[PATCH] proc: reject "." and ".." as filenames

[PATCH] proc: reject "." and ".." as filenames

[Alexey Dobriyan]

Various subsystems can create files and directories in /proc
with names directly controlled by userspace.

Which means "/", "." and ".." are no-no.

"/" split is already taken care of, do the other 2 prohibited names.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/proc/generic.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -366,6 +366,14 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
 		WARN(1, "name len %u\n", qstr.len);
 		return NULL;
 	}
+	if (qstr.len == 1 && fn[0] == '.') {
+		WARN(1, "name '.'\n");
+		return NULL;
+	}
+	if (qstr.len == 2 && fn[0] == '.' && fn[1] == '.') {
+		WARN(1, "name '..'\n");
+		return NULL;
+	}
 	if (*parent == &proc_root && name_to_int(&qstr) != ~0U) {
 		WARN(1, "create '/proc/%s' by hand\n", qstr.name);
 		return NULL;

Re: [PATCH] proc: reject "." and ".." as filenames

[Florian Westphal]

Alexey Dobriyan <adobriyan@gmail.com> wrote:
> Various subsystems can create files and directories in /proc
> with names directly controlled by userspace.
> 
> Which means "/", "." and ".." are no-no.
> 
> "/" split is already taken care of, do the other 2 prohibited names.

Acked-by: Florian Westphal <fw@strlen.de>

I'll send a patch for xtables too to reject bogus names
coming from userspace (syzbot reports WARN() ).

Re: [PATCH] proc: reject "." and ".." as filenames

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On Sat 2018-03-10 03:12:23, Alexey Dobriyan wrote:
> Various subsystems can create files and directories in /proc
> with names directly controlled by userspace.
> 
> Which means "/", "." and ".." are no-no.
> 
> "/" split is already taken care of, do the other 2 prohibited names.

Hmm, patch is probably good idea, but now it means that userspace can
trigger WARN()s, and can hide objects from root by naming them '.' and
'..'... which is not good.

If you know where this happens, it would be nice to fix them in
addition to this patch.
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] proc: reject "." and ".." as filenames

[Alexey Dobriyan]

On Sun, Mar 11, 2018 at 10:30:58PM +0100, Pavel Machek wrote:
> On Sat 2018-03-10 03:12:23, Alexey Dobriyan wrote:
> > Various subsystems can create files and directories in /proc
> > with names directly controlled by userspace.
> > 
> > Which means "/", "." and ".." are no-no.
> > 
> > "/" split is already taken care of, do the other 2 prohibited names.
> 
> Hmm, patch is probably good idea, but now it means that userspace can
> trigger WARN()s, and can hide objects from root by naming them '.' and
> '..'... which is not good.

Patch rejects creation of such entries.

And they should be harmless as VFS lookup won't find them, only readdir
would. It not clear how they could be useful.

Re: [PATCH] proc: reject "." and ".." as filenames

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

On Mon 2018-03-12 00:35:34, Alexey Dobriyan wrote:
> On Sun, Mar 11, 2018 at 10:30:58PM +0100, Pavel Machek wrote:
> > On Sat 2018-03-10 03:12:23, Alexey Dobriyan wrote:
> > > Various subsystems can create files and directories in /proc
> > > with names directly controlled by userspace.
> > > 
> > > Which means "/", "." and ".." are no-no.
> > > 
> > > "/" split is already taken care of, do the other 2 prohibited names.
> > 
> > Hmm, patch is probably good idea, but now it means that userspace can
> > trigger WARN()s, and can hide objects from root by naming them '.' and
> > '..'... which is not good.
> 
> Patch rejects creation of such entries.
> 
> And they should be harmless as VFS lookup won't find them, only readdir
> would. It not clear how they could be useful.

Yeah, as I said, that's half of problem.

If I can name my object "." and it will be hidden from root, that
sounds like a security hole to be prevented.

So if you know _which_ subsystem allow creating files and directories
in /proc with names directly controlled by userspace, please let us
know, we want to fix that.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] proc: reject "." and ".." as filenames

[Andrew Morton]

On Sat, 10 Mar 2018 03:12:23 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> Various subsystems can create files and directories in /proc
> with names directly controlled by userspace.
> 
> Which means "/", "." and ".." are no-no.
> 
> "/" split is already taken care of, do the other 2 prohibited names.
> 
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -366,6 +366,14 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
>  		WARN(1, "name len %u\n", qstr.len);
>  		return NULL;
>  	}
> +	if (qstr.len == 1 && fn[0] == '.') {
> +		WARN(1, "name '.'\n");
> +		return NULL;
> +	}
> +	if (qstr.len == 2 && fn[0] == '.' && fn[1] == '.') {
> +		WARN(1, "name '..'\n");
> +		return NULL;
> +	}
>  	if (*parent == &proc_root && name_to_int(&qstr) != ~0U) {
>  		WARN(1, "create '/proc/%s' by hand\n", qstr.name);
>  		return NULL;

--- a/fs/proc/generic.c~proc-reject-and-as-filenames-fix
+++ a/fs/proc/generic.c
@@ -387,10 +387,8 @@ static struct proc_dir_entry *__proc_cre
 		WARN(1, "name len %u\n", qstr.len);
 		return NULL;
 	}
-	if (qstr.len == 1 && fn[0] == '.') {
-		WARN(1, "name '.'\n");
+	if (WARN(qstr.len == 1 && fn[0] == '.', "name '.'\n"))
 		return NULL;
-	}
 	if (qstr.len == 2 && fn[0] == '.' && fn[1] == '.') {
 		WARN(1, "name '..'\n");
 		return NULL;

is neater, but the whole function should be thus converted and I'll let
you decide on that.

Re: [PATCH] proc: reject "." and ".." as filenames

[Alexey Dobriyan]

On Mon, Mar 12, 2018 at 04:00:18PM -0700, Andrew Morton wrote:
> -	if (qstr.len == 1 && fn[0] == '.') {
> -		WARN(1, "name '.'\n");
> +	if (WARN(qstr.len == 1 && fn[0] == '.', "name '.'\n"))
>  		return NULL;
> -	}

Oh, I hate this style of WARN.
For one thing it overlaps with comma operator.