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.

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.

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

[Andrew Morton]

On Sat, 10 Mar 2018 03:12:23 +0300 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.
> 
> --- 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 == _root && name_to_int() != ~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

[Andrew Morton]

On Sat, 10 Mar 2018 03:12:23 +0300 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.
> 
> --- 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 == _root && name_to_int() != ~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

[Pavel Machek]

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


signature.asc
Description: Digital signature

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

[Pavel Machek]

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


signature.asc
Description: Digital signature

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

[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]

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


signature.asc
Description: Digital signature

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

[Pavel Machek]

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


signature.asc
Description: Digital signature

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

[Florian Westphal]

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.

Acked-by: Florian Westphal 

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

[Florian Westphal]

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.

Acked-by: Florian Westphal 

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