Message ID | 20231003171844.9586-1-volker.weissmann@gmx.de |
---|---|
State | New |
Headers | show |
Series | [v3] Fix FORTIFY_SOURCE false positive | expand |
On 2023-10-03 13:18, Volker Weißmann wrote: > When -D_FORTIFY_SOURCE=2 was given during compilation, > sprintf and similar functions will check if their > first argument is in read-only memory and exit with > *** %n in writable segment detected *** > otherwise. To check if the memory is read-only, glibc > reads frpm the file "/proc/self/maps". If opening this > file fails due to too many open files (EMFILE), glibc > will now ignore this error. > > Fixes [BZ #30932] > > Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de> > --- Thanks! LGTM. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> Adhemerval, could you please add this with your test case patch and send it as a series? I can then review that too. Thanks, Sid > sysdeps/unix/sysv/linux/readonly-area.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c > index edc68873f6..ba32372ebb 100644 > --- a/sysdeps/unix/sysv/linux/readonly-area.c > +++ b/sysdeps/unix/sysv/linux/readonly-area.c > @@ -42,7 +42,9 @@ __readonly_area (const char *ptr, size_t size) > to the /proc filesystem if it is set[ug]id. There has > been no willingness to change this in the kernel so > far. */ > - || errno == EACCES) > + || errno == EACCES > + /* Process has reached the maximum number of open files. */ > + || errno == EMFILE) > return 1; > return -1; > } > -- > 2.42.0 >
I thought about my patch again... If an attacker can make the victim-program leak file descriptors, this can be used to defeat string fortification. Since leaking file-descriptors is normally not that bad (normally, it cannot lead to anything worse than a DOS), programmers/security auditors might be less careful in ensuring that no fd leaks. It doesn't even have to be a true leak, image if e.g. the attacker controls python code that runs inside an interpreter that does some sandboxing. Then the attacker could do something like: with open("/dev/zero") as file1: with open("/dev/zero") as file2: ... with open("/dev/zero") as file1023: trigger_formatstring_bug_in_the_python_interpreter() to break out of the sandbox. I know I'm being a bit paranoid, but glibc is used *everywhere*. I think instead of "return 1;" we should do __libc_fatal ("*** too many open file descriptors ***\n"); instead. On 03.10.23 19:25, Siddhesh Poyarekar wrote: > On 2023-10-03 13:18, Volker Weißmann wrote: >> When -D_FORTIFY_SOURCE=2 was given during compilation, >> sprintf and similar functions will check if their >> first argument is in read-only memory and exit with >> *** %n in writable segment detected *** >> otherwise. To check if the memory is read-only, glibc >> reads frpm the file "/proc/self/maps". If opening this >> file fails due to too many open files (EMFILE), glibc >> will now ignore this error. >> >> Fixes [BZ #30932] >> >> Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de> >> --- > > Thanks! LGTM. > > Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > > Adhemerval, could you please add this with your test case patch and > send it as a series? I can then review that too. > > Thanks, > Sid > >> sysdeps/unix/sysv/linux/readonly-area.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c >> b/sysdeps/unix/sysv/linux/readonly-area.c >> index edc68873f6..ba32372ebb 100644 >> --- a/sysdeps/unix/sysv/linux/readonly-area.c >> +++ b/sysdeps/unix/sysv/linux/readonly-area.c >> @@ -42,7 +42,9 @@ __readonly_area (const char *ptr, size_t size) >> to the /proc filesystem if it is set[ug]id. There has >> been no willingness to change this in the kernel so >> far. */ >> - || errno == EACCES) >> + || errno == EACCES >> + /* Process has reached the maximum number of open files. */ >> + || errno == EMFILE) >> return 1; >> return -1; >> } >> -- >> 2.42.0 >>
On Okt 03 2023, Volker Weißmann wrote: > diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c > index edc68873f6..ba32372ebb 100644 > --- a/sysdeps/unix/sysv/linux/readonly-area.c > +++ b/sysdeps/unix/sysv/linux/readonly-area.c > @@ -42,7 +42,9 @@ __readonly_area (const char *ptr, size_t size) > to the /proc filesystem if it is set[ug]id. There has > been no willingness to change this in the kernel so > far. */ > - || errno == EACCES) > + || errno == EACCES > + /* Process has reached the maximum number of open files. */ > + || errno == EMFILE) Should this also handle ENFILE?
Yes, you are right. I will write a patch once we decided whether we want to "return 1" or __libc_fatal ("*** too many open file descriptors ***\n") On 04.10.23 16:51, Andreas Schwab wrote: > On Okt 03 2023, Volker Weißmann wrote: > >> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c >> index edc68873f6..ba32372ebb 100644 >> --- a/sysdeps/unix/sysv/linux/readonly-area.c >> +++ b/sysdeps/unix/sysv/linux/readonly-area.c >> @@ -42,7 +42,9 @@ __readonly_area (const char *ptr, size_t size) >> to the /proc filesystem if it is set[ug]id. There has >> been no willingness to change this in the kernel so >> far. */ >> - || errno == EACCES) >> + || errno == EACCES >> + /* Process has reached the maximum number of open files. */ >> + || errno == EMFILE) > Should this also handle ENFILE? >
On 04/10/23 11:43, Volker Weißmann wrote: > I thought about my patch again... > > If an attacker can make the victim-program leak file descriptors, this > can be used to defeat string fortification. > > Since leaking file-descriptors is normally not that bad (normally, it > cannot lead to anything worse than a DOS), programmers/security auditors > might be less careful in ensuring that no fd leaks. > > It doesn't even have to be a true leak, image if e.g. the attacker > controls python code that runs inside an interpreter that does some > sandboxing. Then the attacker could do something like: > > with open("/dev/zero") as file1: > with open("/dev/zero") as file2: > ... > with open("/dev/zero") as file1023: > trigger_formatstring_bug_in_the_python_interpreter() > > to break out of the sandbox. > > I know I'm being a bit paranoid, but glibc is used *everywhere*. > > I think instead of "return 1;" we should do > > __libc_fatal ("*** too many open file descriptors ***\n"); > > instead. The same if also check for procfs support, meaning that if it is not accessible the process will start to abort execution. Not sure about what kind of breakage it would incur, but it should reasonable to abort on both cases since this is done iff fortify is enabled. > > > On 03.10.23 19:25, Siddhesh Poyarekar wrote: >> On 2023-10-03 13:18, Volker Weißmann wrote: >>> When -D_FORTIFY_SOURCE=2 was given during compilation, >>> sprintf and similar functions will check if their >>> first argument is in read-only memory and exit with >>> *** %n in writable segment detected *** >>> otherwise. To check if the memory is read-only, glibc >>> reads frpm the file "/proc/self/maps". If opening this >>> file fails due to too many open files (EMFILE), glibc >>> will now ignore this error. >>> >>> Fixes [BZ #30932] >>> >>> Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de> >>> --- >> >> Thanks! LGTM. >> >> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> >> >> Adhemerval, could you please add this with your test case patch and >> send it as a series? I can then review that too. >> >> Thanks, >> Sid >> >>> sysdeps/unix/sysv/linux/readonly-area.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c >>> b/sysdeps/unix/sysv/linux/readonly-area.c >>> index edc68873f6..ba32372ebb 100644 >>> --- a/sysdeps/unix/sysv/linux/readonly-area.c >>> +++ b/sysdeps/unix/sysv/linux/readonly-area.c >>> @@ -42,7 +42,9 @@ __readonly_area (const char *ptr, size_t size) >>> to the /proc filesystem if it is set[ug]id. There has >>> been no willingness to change this in the kernel so >>> far. */ >>> - || errno == EACCES) >>> + || errno == EACCES >>> + /* Process has reached the maximum number of open files. */ >>> + || errno == EMFILE) >>> return 1; >>> return -1; >>> } >>> -- >>> 2.42.0 >>>
On 2023-10-04 12:57, Adhemerval Zanella Netto wrote: > > > On 04/10/23 11:43, Volker Weißmann wrote: >> I thought about my patch again... >> >> If an attacker can make the victim-program leak file descriptors, this >> can be used to defeat string fortification. >> >> Since leaking file-descriptors is normally not that bad (normally, it >> cannot lead to anything worse than a DOS), programmers/security auditors >> might be less careful in ensuring that no fd leaks. >> >> It doesn't even have to be a true leak, image if e.g. the attacker >> controls python code that runs inside an interpreter that does some >> sandboxing. Then the attacker could do something like: >> >> with open("/dev/zero") as file1: >> with open("/dev/zero") as file2: >> ... >> with open("/dev/zero") as file1023: >> trigger_formatstring_bug_in_the_python_interpreter() >> >> to break out of the sandbox. >> >> I know I'm being a bit paranoid, but glibc is used *everywhere*. >> >> I think instead of "return 1;" we should do >> >> __libc_fatal ("*** too many open file descriptors ***\n"); >> >> instead. > > The same if also check for procfs support, meaning that if it is not accessible > the process will start to abort execution. Not sure about what kind of breakage > it would incur, but it should reasonable to abort on both cases since this is > done iff fortify is enabled. I'm worried that this would result in spurious reports and may discourage usage of fortification, something that we do distribution-wide right now. Sid
* Volker Weißmann: > When -D_FORTIFY_SOURCE=2 was given during compilation, > sprintf and similar functions will check if their > first argument is in read-only memory and exit with > *** %n in writable segment detected *** > otherwise. To check if the memory is read-only, glibc > reads frpm the file "/proc/self/maps". If opening this > file fails due to too many open files (EMFILE), glibc > will now ignore this error. > > Fixes [BZ #30932] > > Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de> > --- > sysdeps/unix/sysv/linux/readonly-area.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c > index edc68873f6..ba32372ebb 100644 > --- a/sysdeps/unix/sysv/linux/readonly-area.c > +++ b/sysdeps/unix/sysv/linux/readonly-area.c > @@ -42,7 +42,9 @@ __readonly_area (const char *ptr, size_t size) > to the /proc filesystem if it is set[ug]id. There has > been no willingness to change this in the kernel so > far. */ > - || errno == EACCES) > + || errno == EACCES > + /* Process has reached the maximum number of open files. */ > + || errno == EMFILE) > return 1; > return -1; > } This whole thing is rather questionable. First of all, the compiler should detect the fact that a format argument to printf is a string literal and record that in the flags argument (which already exists for __printf_chk). Then we wouldn't have to do any %n security checks for most uses of %n. (The flags argument cannot be spoofed just by altering the string.) Siddhesh, is that something you could be working on? Even without that, if we are willing to trust the ld.so data structures, we can do the permission check in userspace for strings that come from .rodata. We an find the ELF object that contains them and check if the loadable segment has the right permissions (or we are in the RELRO area). After these changes, I think we can fail hard on /proc-related errors because they are very unlikely to happen. Thanks, Florian
On 2023-10-04 13:36, Florian Weimer wrote: > This whole thing is rather questionable. > > First of all, the compiler should detect the fact that a format argument > to printf is a string literal and record that in the flags argument > (which already exists for __printf_chk). Then we wouldn't have to do > any %n security checks for most uses of %n. (The flags argument cannot > be spoofed just by altering the string.) > > Siddhesh, is that something you could be working on? Hmm, I thought the compiler already did this. I can take a look. > Even without that, if we are willing to trust the ld.so data structures, > we can do the permission check in userspace for strings that come from > .rodata. We an find the ELF object that contains them and check if the > loadable segment has the right permissions (or we are in the RELRO > area). > > After these changes, I think we can fail hard on /proc-related errors > because they are very unlikely to happen. We'd have to figure out a way for static binaries too. Sid
diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c index edc68873f6..ba32372ebb 100644 --- a/sysdeps/unix/sysv/linux/readonly-area.c +++ b/sysdeps/unix/sysv/linux/readonly-area.c @@ -42,7 +42,9 @@ __readonly_area (const char *ptr, size_t size) to the /proc filesystem if it is set[ug]id. There has been no willingness to change this in the kernel so far. */ - || errno == EACCES) + || errno == EACCES + /* Process has reached the maximum number of open files. */ + || errno == EMFILE) return 1; return -1; }
When -D_FORTIFY_SOURCE=2 was given during compilation, sprintf and similar functions will check if their first argument is in read-only memory and exit with *** %n in writable segment detected *** otherwise. To check if the memory is read-only, glibc reads frpm the file "/proc/self/maps". If opening this file fails due to too many open files (EMFILE), glibc will now ignore this error. Fixes [BZ #30932] Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de> --- sysdeps/unix/sysv/linux/readonly-area.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) -- 2.42.0