| Message ID | 20241211142929.247692-1-mjg59@srcf.ucam.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] Add a prctl to disable ".." traversal in path resolution | expand |
On Mittwoch, 11. Dezember 2024 15:29:29 Mitteleuropäische Normalzeit Matthew Garrett wrote: > Path traversal attacks remain a common security vulnerability > (https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=%22path+traversal%22) > and many are due to either failing to filter out ".." when validating a > path or incorrectly collapsing other sequences of "."s into ".." . > Evidence suggests that improving education isn't fixing the problem. Hi Matthew, I get the motivation here, but I think you've just turned "educate about handling .. properly" into "educate about this specific mitigation". Linux already offers a multitude of ways in which people who deploy potentially buggy applications can restrict which files these applications have access to, from old fashioned UNIX permissions over SELinux and other LSMs to namespaces. (Some software, like postfix, even thinks chroots are appropriate for this, but I think modern understanding of security would disagree with that.) People who fall victim to path traversal vulnerabilities in actual deployments will not have deployed any of those mitigations, and therefore I don't see why they would deploy this one. As sad as it is to see that supposed enterprise security network appliances still fall victim to the same mistakes people's CGI perl scripts did in the 90s, I don't think the ones who would opt into this mechanism are the ones getting got through path traversal. > The majority of internet-facing applications are unlikely to require the > ability to handle ".." in a path after startup, and many are unlikely to > require it at all. I would like to see some evidence of that. Do webservers already realname the path when a client asks for example.com/foo/../css/bar.css? If so, there's no path traversal vulnerability, if not, then they do require it. Cheers, Nicolas Frattaroli
On 2024-12-11, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > Path traversal attacks remain a common security vulnerability > (https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=%22path+traversal%22) > and many are due to either failing to filter out ".." when validating a > path or incorrectly collapsing other sequences of "."s into ".." . > Evidence suggests that improving education isn't fixing the problem. I was thinking about adding a RESOLVE_NO_DOTDOT which would do something like this but on a per-openat2-call basis. The main problem with making this global for the entire process is that most tools would not be able to practically enable this for themselves as it would require auditing the entire execution environment as well as all dependencies that might dare to use ".." in a path anywhere in their codebase. Given that "nosymfollow" was added by Google because of the impossibility of a similar kind of audit, I feel enabling this would face a pretty similar issue. I get that returning an error (even if it kills the program) could be seen as preferable in some cases, but as a general-purpose tool I don't really see how any program could enable this without fear of issues. Even if the application itself handles errors gracefully, there's no way of knowing whether some dependency (or even the stdlib / runtime) will abort the program if it gets an error. > The majority of internet-facing applications are unlikely to require the > ability to handle ".." in a path after startup, and many are unlikely to > require it at all. This patch adds a prctl() to simply request that the > VFS path resolution code return -EPERM if it hits a ".." in the process. > Applications can either call this themselves, or a service manager can > do this on their behalf before execing them. I would suggest making this -EXDEV to match the rest of the RESOLVE_* flags (in particular, RESOLVE_BENEATH). > Note that this does break resolution of symlinks with ".." in them, > which means it breaks the common case of /etc/whatever/sites-available.d > containing site-specific configuration, with > /etc/whatever/sites-enabled.d containing a set of relative symlinks to > ../sites-available.d/ entries. In this case either configuration would > need to be updated before deployment, or the process would call prctl() > itself after parsing configuration (and then disable and re-enable the > feature whenever re-reading configuration). Getting this right for all > scenarios involving symlinks seems awkward and I'm not sure it's worth > it, but also I don't even play a VFS expert on TV so if someone has > clever ideas here we can extend this to support that case. I think RESOLVE_BENEATH is usually more along the lines of what programs that are trying to restrict themselves would want (RESOLVE_IN_ROOT is what extraction tools want, on the other hand) as it only blocks ".." components that move you out of the directory you expect. It also blocks absolute symlinks, which this proposal does nothing about (it even blocks magic-links, which can be an even bigger issue depending on what kind of program we are talking about). Alas, RESOLVE_BENEATH requires education... > Signed-off-by: Matthew Garrett <mjg59@srcf.ucam.org> > --- > fs/namei.c | 7 +++++++ > include/linux/sched.h | 1 + > include/uapi/linux/prctl.h | 3 +++ > kernel/sys.c | 5 +++++ > 4 files changed, 16 insertions(+) > > diff --git a/fs/namei.c b/fs/namei.c > index 9d30c7aa9aa6..01d0fa415b64 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -2431,6 +2431,13 @@ static int link_path_walk(const char *name, struct nameidata *nd) > > switch(lastword) { > case LAST_WORD_IS_DOTDOT: > + /* > + * Deny .. in resolution if the process has indicated > + * it wants to protect against path traversal > + * vulnerabilities > + */ > + if (unlikely(current->deny_path_traversal)) > + return -EPERM; > nd->last_type = LAST_DOTDOT; > nd->state |= ND_JUMPED; > break; > diff --git a/include/linux/sched.h b/include/linux/sched.h > index d380bffee2ef..9fc7f4c11645 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -1008,6 +1008,7 @@ struct task_struct { > /* delay due to memory thrashing */ > unsigned in_thrashing:1; > #endif > + unsigned deny_path_traversal:1; > #ifdef CONFIG_PREEMPT_RT > struct netdev_xmit net_xmit; > #endif > diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h > index 5c6080680cb2..d289acecef6c 100644 > --- a/include/uapi/linux/prctl.h > +++ b/include/uapi/linux/prctl.h > @@ -353,4 +353,7 @@ struct prctl_mm_map { > */ > #define PR_LOCK_SHADOW_STACK_STATUS 76 > > +/* Block resolution of "../" in paths, returning -EPERM instead */ > +#define PR_SET_PATH_TRAVERSAL_BLOCK 77 > + > #endif /* _LINUX_PRCTL_H */ > diff --git a/kernel/sys.c b/kernel/sys.c > index c4c701c6f0b4..204ea88d5597 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -2809,6 +2809,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > return -EINVAL; > error = arch_lock_shadow_stack_status(me, arg2); > break; > + case PR_SET_PATH_TRAVERSAL_BLOCK: > + if ((arg2 > 1) || arg3 || arg4 || arg5) > + return -EINVAL; > + current->deny_path_traversal = !!arg2; > + break; > default: > error = -EINVAL; > break; > -- > 2.47.0 > >
On Thu, Dec 12, 2024 at 02:56:59AM +1100, Aleksa Sarai wrote: > I think RESOLVE_BENEATH is usually more along the lines of what programs > that are trying to restrict themselves would want (RESOLVE_IN_ROOT is > what extraction tools want, on the other hand) as it only blocks ".." > components that move you out of the directory you expect. > > It also blocks absolute symlinks, which this proposal does nothing about > (it even blocks magic-links, which can be an even bigger issue depending > on what kind of program we are talking about). Alas, RESOLVE_BENEATH > requires education... So does this prctl, when you get to that - any references to "service manager" that might turn it on are contradicted by the "after startup" bit in the original posting. IOW, I very much doubt that this problem is amenable to cargo-culting. _If_ somebody wants to collect actual information about the use patterns, something like prctl that would spew a stack trace when running into .. would be an obvious approach, but I would strongly object to even inserting a tracepoint of that sort into the mainline kernel.
On Thu, Dec 12, 2024 at 02:56:59AM +1100, Aleksa Sarai wrote: > On 2024-12-11, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > Path traversal attacks remain a common security vulnerability > > (https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=%22path+traversal%22) > > and many are due to either failing to filter out ".." when validating a > > path or incorrectly collapsing other sequences of "."s into ".." . > > Evidence suggests that improving education isn't fixing the problem. > > I was thinking about adding a RESOLVE_NO_DOTDOT which would do something > like this but on a per-openat2-call basis. That's what I was thinking a while ago. I discussed that with Linus in connection to a change by Jann for looking up module paths. https://lore.kernel.org/r/CAADWXX_zpqzYdCpmQGF3JgsN4+wk3AsuQLCKREkDC1ScxSfDjQ@mail.gmail.com > > The main problem with making this global for the entire process is that > most tools would not be able to practically enable this for themselves > as it would require auditing the entire execution environment as well as > all dependencies that might dare to use ".." in a path anywhere in their > codebase. I agree.
On 2024-12-11, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Thu, Dec 12, 2024 at 02:56:59AM +1100, Aleksa Sarai wrote: > > > I think RESOLVE_BENEATH is usually more along the lines of what programs > > that are trying to restrict themselves would want (RESOLVE_IN_ROOT is > > what extraction tools want, on the other hand) as it only blocks ".." > > components that move you out of the directory you expect. > > > > It also blocks absolute symlinks, which this proposal does nothing about > > (it even blocks magic-links, which can be an even bigger issue depending > > on what kind of program we are talking about). Alas, RESOLVE_BENEATH > > requires education... > > So does this prctl, when you get to that - any references to "service manager" > that might turn it on are contradicted by the "after startup" bit in the > original posting. > > IOW, I very much doubt that this problem is amenable to cargo-culting. I'm not sure I understand what you're saying -- was this comment intended for me or Matthew? I was just trying to say that: 1. Most programs that want to access config files or static files to serve as a web server where there might be ".." symlinks would probably want RESOLVE_BENEATH behaviour (using the config directory as the root) because that will only block escaping ".."s (and will also block absolute symlinks as well as magic-links that can escape too -- which are also issues that you need to deal with anyway). 2. Blocking this on a process-wide level could cause issues for any given program because the language's stdlib or runtime could internally do a ".." open operation, and if that fails the runtime might decide to crash the program (we've run into issues like this in Go -- though those were related to errors from pthread_* functions). If the service manager sets the prctl then if the link loader or glibc on startup have to resolve ".." due to the way the system was set up, you would also get errors (and probably a crash). Obviously, some users would prefer the application crash rather than resolving a "..", but most would not which is why I'm unsure of how much this will help solve the problem in practice. As a devils-advocate proposal, I wonder if we could instead do something like nosymfollow (so nodotdot?) where you can mark particular mounts to have this restriction. This could be used to limit all ".."s globally for a process using a mntns, but it could also be scoped to application data directories (or to all untrusted directories) without requiring program changes (the same argument was used for adding nosymfollow when RESOLVE_NO_SYMLINKS existed). Obviously there would be problems with this proposal -- unlike nosymfollow (which is more like nodev or noexec in concept, and so is more about conceptually restricting access modes of inodes -- even though nosymfollow is piped through namei), this is would be nd->mnt impacting how lookups work, which seems kinda ugly and would burn another MS_ flag (the final one aside from internal ones AFAICS)... Just food for thought. (I think LOOKUP_NO_DOTDOT / RESOLVE_NO_DOTDOT is the most usable solution for most programs that really need this, fwiw.) > _If_ somebody wants to collect actual information about the use patterns, > something like prctl that would spew a stack trace when running into > .. would be an obvious approach, but I would strongly object to even > inserting a tracepoint of that sort into the mainline kernel. You probably don't need a custom tracepoint, I expect they can get most of the information they'd need with bpftrace.
diff --git a/fs/namei.c b/fs/namei.c index 9d30c7aa9aa6..01d0fa415b64 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2431,6 +2431,13 @@ static int link_path_walk(const char *name, struct nameidata *nd) switch(lastword) { case LAST_WORD_IS_DOTDOT: + /* + * Deny .. in resolution if the process has indicated + * it wants to protect against path traversal + * vulnerabilities + */ + if (unlikely(current->deny_path_traversal)) + return -EPERM; nd->last_type = LAST_DOTDOT; nd->state |= ND_JUMPED; break; diff --git a/include/linux/sched.h b/include/linux/sched.h index d380bffee2ef..9fc7f4c11645 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1008,6 +1008,7 @@ struct task_struct { /* delay due to memory thrashing */ unsigned in_thrashing:1; #endif + unsigned deny_path_traversal:1; #ifdef CONFIG_PREEMPT_RT struct netdev_xmit net_xmit; #endif diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h index 5c6080680cb2..d289acecef6c 100644 --- a/include/uapi/linux/prctl.h +++ b/include/uapi/linux/prctl.h @@ -353,4 +353,7 @@ struct prctl_mm_map { */ #define PR_LOCK_SHADOW_STACK_STATUS 76 +/* Block resolution of "../" in paths, returning -EPERM instead */ +#define PR_SET_PATH_TRAVERSAL_BLOCK 77 + #endif /* _LINUX_PRCTL_H */ diff --git a/kernel/sys.c b/kernel/sys.c index c4c701c6f0b4..204ea88d5597 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2809,6 +2809,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, return -EINVAL; error = arch_lock_shadow_stack_status(me, arg2); break; + case PR_SET_PATH_TRAVERSAL_BLOCK: + if ((arg2 > 1) || arg3 || arg4 || arg5) + return -EINVAL; + current->deny_path_traversal = !!arg2; + break; default: error = -EINVAL; break;
Path traversal attacks remain a common security vulnerability (https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=%22path+traversal%22) and many are due to either failing to filter out ".." when validating a path or incorrectly collapsing other sequences of "."s into ".." . Evidence suggests that improving education isn't fixing the problem. The majority of internet-facing applications are unlikely to require the ability to handle ".." in a path after startup, and many are unlikely to require it at all. This patch adds a prctl() to simply request that the VFS path resolution code return -EPERM if it hits a ".." in the process. Applications can either call this themselves, or a service manager can do this on their behalf before execing them. Note that this does break resolution of symlinks with ".." in them, which means it breaks the common case of /etc/whatever/sites-available.d containing site-specific configuration, with /etc/whatever/sites-enabled.d containing a set of relative symlinks to ../sites-available.d/ entries. In this case either configuration would need to be updated before deployment, or the process would call prctl() itself after parsing configuration (and then disable and re-enable the feature whenever re-reading configuration). Getting this right for all scenarios involving symlinks seems awkward and I'm not sure it's worth it, but also I don't even play a VFS expert on TV so if someone has clever ideas here we can extend this to support that case. Signed-off-by: Matthew Garrett <mjg59@srcf.ucam.org> --- fs/namei.c | 7 +++++++ include/linux/sched.h | 1 + include/uapi/linux/prctl.h | 3 +++ kernel/sys.c | 5 +++++ 4 files changed, 16 insertions(+)