proc: reject "." and ".." as filenames
PATCH 1/1 proc: reject "." and ".." as filenames
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.
---
fs/proc/generic.c | 8 ++++++++ 1 file changed, 8 insertions(+)
fs/proc/generic.c +8 -0
@@ -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 ;
Quoted 6 lines · 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.
I'll send a patch for xtables too to reject bogus names coming from userspace (syzbot reports WARN() ).
Quoted 6 lines · 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.
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.
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 11 lines · Pavel Machek
Quoted 6 lines · 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.
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.
Quoted 9 lines · Alexey Dobriyan
Quoted 8 lines · Pavel Machek
Quoted 6 lines · 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.
Quoted 8 lines
Quoted 3 lines
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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 14 lines · On Sat, 10 Mar 2018 03:12:23 +0300 Alexey Dobriyan <[email protected]> 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.
fs/proc/generic.c +1
-0@@ -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 ] =='.' ) {
fs/proc/generic.c static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent, · line 379
Quoted 10 lines
fs/proc/generic.c +7
-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 ;
fs/proc/generic.c renamed from fs/proc/generic.c~proc-reject-and-as-filenames-fix +1 -3
@@ -387,10 +387,8 @@ static struct proc_dir_entry *__proc_creWARN (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.
fs/proc/generic.c static struct proc_dir_entry *__proc_cre · line 393
Quoted 5 lines · Andrew Morton
fs/proc/generic.c renamed from fs/proc/generic.c~proc-reject-and-as-filenames-fix +1
-3-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.