Message-ID: <20241211.154841-core.hand.fragrant.rearview-Ajjgdy5TrwhO@cyphar.com>
Date: Thu, 12 Dec 2024 02:56:59 +1100
From: Aleksa Sarai <cyphar@...har.com>
To: Matthew Garrett <mjg59@...f.ucam.org>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC] Add a prctl to disable ".." traversal in path resolution

On 2024-12-11, Matthew Garrett <mjg59@...f.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@...f.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
> 
> 

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists