Message ID | 20210719193343.208149-1-ajordanr@google.com |
---|---|
State | New |
Headers | show |
Series | Deny preload of files on NOEXEC mounts | expand |
On 19/07/2021 16:33, Jordan R Abrahams via Libc-alpha wrote: > I'm from Google's ChromeOS Toolchain team, and I have a security > hardening patch to port upstream. I've confirmed with our > security team that this patch can be public, and I'm looking for > review and feedback. > > I'm a new contributor, but Google already has a CLA on file for > the FSF, so it should not be an issue. If I'm missing any > contribution info, please do let me know. > > Best regards, > ~Jordan R Abrahams > > -- >8 -- > This commit hardens a security flaw in dl-load.c. > > At present, one could dynamically load any shared object files even if > they resided on a NOEXEC mount partition. This introduces an exploit > where an attacker may get around this NOEXEC requirement by preloading > a malicious shared library. > > For example, from shell this can look like: > > $ LD_PRELOAD='/tmp/malicious_lib.so' id > Hello world from malicious_lib! > > Which will work even if /tmp/ is a noexec mount. > > A proof of concept of this exploit can be found here at > https://bugs.chromium.org/p/chromium/issues/detail?id=1182687 > The bug thread is restricted, but we are willing to add > interested parties with the approval of our security team. > > This patch hardens against the exploit by checking the file descriptor > if the file lies in a NOEXEC mount partition via an fstatvfs call. > The error string is set, and we jump to the `lose` cleanup code. > > With this patch, the above example instead becomes: > > $ LD_PRELOAD='/tmp/malicious_lib.so' id > /usr/bin/id: error while loading shared libraries: /tmp/malicious_lib.so: file not located on exec mount > # <... normal stdout for id ...> > > Signed-off-by: Jordan R Abrahams <ajordanr@google.com> Don't noexec already prevents that? An example below with the glibc 2.33 on 5.11.0-25. $ cat test.c int main (int argc, char *argv[]) { return 0; } $ cat libpreload.c #include <stdio.h> static void __attribute__ ((constructor)) lib_init (void) { printf ("%s\n", __func__); } $ gcc -Wall test.c -o test $ gcc -Wall -shared libpreload.c -o libpreload.so $ LD_PRELOAD=./libpreload.so ./test lib_init $ dd if=/dev/zero of=loopbackfile.img bs=1M count=1 $ sudo losetup -fP loopbackfile.img $ losetup -a | grep loopbackfile.img /dev/loop16: []: (/tmp/test/loopbackfile.img) $ mkfs.ext4 loopbackfile.img $ mkdir loopfs $ sudo mount -o loop,noexec /dev/loop16 loopfs $ mount | grep loopfs /dev/loop16 on /tmp/test/loopfs type ext4 (rw,noexec,relatime) $ mv libpreload.so loopfs/ $ LD_PRELOAD=loopfs/libpreload.so ./test ERROR: ld.so: object 'loopfs/libpreload.so' from LD_PRELOAD cannot be preloaded (failed to map segment from shared object): ignored. I am not sure if this is an Ubuntu hardnening, if this is only enabled on recent kernels, or only enabled for some filesystems. It seems that with CONFIG_SECURITY the code on security/security.c does filter out the PROT_EXEC on mmap_prot() (but you will need to check how all the pieces does work). This kind of hardening does sound it would be better implemented by the kernel itself, although I don't have a strong opinion if the idea is to hardened against possible kernel version or FS that does not fully support it. Some comments below regarding the code. Did you actually ran the make check? I am asking because first the code does not build (there is a wrong label usage) and you would see a lot of linknamespace issues (I explained better below). > --- > elf/dl-load.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/elf/dl-load.c b/elf/dl-load.c > index 650e4edc35..81ca08c38f 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -29,6 +29,7 @@ > #include <sys/mman.h> > #include <sys/param.h> > #include <sys/stat.h> > +#include <sys/statvfs.h> > #include <sys/types.h> > #include <gnu/lib-names.h> > > @@ -1572,6 +1573,20 @@ print_search_path (struct r_search_path_elem **list, > _dl_debug_printf_c ("\t\t(%s)\n", what); > } > > +/* Check if a the passed in file descriptor points to file on an executable mount. */ > +static int > +check_exec (int fd) > +{ > + struct statvfs buf; > + int stated = fstatvfs (fd, &buf); > + if (stated == 0) > + { > + return !(buf.f_flag & ST_NOEXEC); > + } > + /* Could not fstat the file. */ > + return false; > +} > + Wrong indentation and besides using fstatvfs that creates a linknamespace polution, it might defeats the hardening (since one can interpose fstatvfs). You will need to add a hidden_def (__fstatvfs) and use __fstatvfs() instead. > /* Open a file and verify it is an ELF file for this architecture. We > ignore only ELF files for other architectures. Non-ELF files and > ELF files with different header information cause fatal errors since > @@ -1650,8 +1665,8 @@ open_verify (const char *name, int fd, > #endif > > if (fd == -1) > - /* Open the file. We always open files read-only. */ > - fd = __open64_nocancel (name, O_RDONLY | O_CLOEXEC); > + /* Open the file. We always open files read-only. */ > + fd = __open64_nocancel (name, O_RDONLY | O_CLOEXEC); > > if (fd != -1) > { Superfluous change here. > @@ -1667,6 +1682,14 @@ open_verify (const char *name, int fd, > __set_errno (0); > fbp->len = 0; > assert (sizeof (fbp->buf) > sizeof (ElfW(Ehdr))); > + > + /* Before we read in the file, check if the file is in an exec mount */ > + if (__glibc_unlikely (!check_exec(fd))) Space before '(': "check_exext (fd)". > + { > + errstring = N_("file not located on exec mount"); > + goto call_lose; > + } > + This should 'lose' (there is no label call_lose). > /* Read in the header. */ > do > { >
* Adhemerval Zanella via Libc-alpha: > I am not sure if this is an Ubuntu hardnening, if this is only enabled > on recent kernels, or only enabled for some filesystems. It seems that > with CONFIG_SECURITY the code on security/security.c does filter out > the PROT_EXEC on mmap_prot() (but you will need to check how all the > pieces does work). > > This kind of hardening does sound it would be better implemented by the > kernel itself, although I don't have a strong opinion if the idea is > to hardened against possible kernel version or FS that does not fully > support it. Yes, this looks like policy that the kernel should implement. We could help the kernel by specifying an O_NEEDEXEC or O_MAYEXEC flag, but the kernel patches in this area do not seem to have made much progress I think. Trying to implement a noexec policy in userspace looks wrong to me. Thanks, Florian
On 23/07/2021 16:22, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> I am not sure if this is an Ubuntu hardnening, if this is only enabled >> on recent kernels, or only enabled for some filesystems. It seems that >> with CONFIG_SECURITY the code on security/security.c does filter out >> the PROT_EXEC on mmap_prot() (but you will need to check how all the >> pieces does work). >> >> This kind of hardening does sound it would be better implemented by the >> kernel itself, although I don't have a strong opinion if the idea is >> to hardened against possible kernel version or FS that does not fully >> support it. > > Yes, this looks like policy that the kernel should implement. We could > help the kernel by specifying an O_NEEDEXEC or O_MAYEXEC flag, but the > kernel patches in this area do not seem to have made much progress I > think. So the noexec not allowing PROT_EXEC is not really upstream? > > Trying to implement a noexec policy in userspace looks wrong to me. I am not sure exactly what kind of issue they are trying to prevent, since LD_PRELOAD won't work on setuid/seguid. My guess is on chromeos all user-accessible mount point are set with noexec, so kernel will prevent the direct execution of static binaries and a possible loader.
* Adhemerval Zanella: > On 23/07/2021 16:22, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> I am not sure if this is an Ubuntu hardnening, if this is only enabled >>> on recent kernels, or only enabled for some filesystems. It seems that >>> with CONFIG_SECURITY the code on security/security.c does filter out >>> the PROT_EXEC on mmap_prot() (but you will need to check how all the >>> pieces does work). >>> >>> This kind of hardening does sound it would be better implemented by the >>> kernel itself, although I don't have a strong opinion if the idea is >>> to hardened against possible kernel version or FS that does not fully >>> support it. >> >> Yes, this looks like policy that the kernel should implement. We could >> help the kernel by specifying an O_NEEDEXEC or O_MAYEXEC flag, but the >> kernel patches in this area do not seem to have made much progress I >> think. > > So the noexec not allowing PROT_EXEC is not really upstream? $ chmod 644 libc.so $ bash testrun.sh nss/getent nss/getent: wrong number of arguments Try `getent --help' or `getent --usage' for more information. So I would say: no, it's not the default. >> Trying to implement a noexec policy in userspace looks wrong to me. > > I am not sure exactly what kind of issue they are trying to prevent, > since LD_PRELOAD won't work on setuid/seguid. My guess is on chromeos > all user-accessible mount point are set with noexec, so kernel will > prevent the direct execution of static binaries and a possible loader. It's a frequently requested feature. Some security policies require that all writable mounts must also be noexec. That doesn't do much good if the bypass is trivial. Of course other bypasses remain (script interpreters and suchlike, hence the need for O_MAYEXEC). Thanks, Florian
On 23/07/2021 17:50, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 23/07/2021 16:22, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> I am not sure if this is an Ubuntu hardnening, if this is only enabled >>>> on recent kernels, or only enabled for some filesystems. It seems that >>>> with CONFIG_SECURITY the code on security/security.c does filter out >>>> the PROT_EXEC on mmap_prot() (but you will need to check how all the >>>> pieces does work). >>>> >>>> This kind of hardening does sound it would be better implemented by the >>>> kernel itself, although I don't have a strong opinion if the idea is >>>> to hardened against possible kernel version or FS that does not fully >>>> support it. >>> >>> Yes, this looks like policy that the kernel should implement. We could >>> help the kernel by specifying an O_NEEDEXEC or O_MAYEXEC flag, but the >>> kernel patches in this area do not seem to have made much progress I >>> think. >> >> So the noexec not allowing PROT_EXEC is not really upstream? > > $ chmod 644 libc.so > $ bash testrun.sh nss/getent > nss/getent: wrong number of arguments > Try `getent --help' or `getent --usage' for more information. > > So I would say: no, it's not the default. I am getting the same error on a centos7 with kernel 3.10.0-1160.31.1.el7.x86_64 and glibc 2.17: $ mount | grep loopfs /dev/loop0 on /home/azanella/loopfs type ext4 (rw,*noexec*,relatime,seclabel) $ LD_PRELOAD=loopfs/libpreload.so ./test ERROR: ld.so: object 'loopfs/libpreload.so' from LD_PRELOAD cannot be preloaded: ignored. $ strace env LD_PRELOAD=loopfs/libpreload.so ./test [...] mmap(NULL, 2101304, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = -1 EPERM (Operation not permitted) [...] I haven't tested a vanilla kernel yet. > >>> Trying to implement a noexec policy in userspace looks wrong to me. >> >> I am not sure exactly what kind of issue they are trying to prevent, >> since LD_PRELOAD won't work on setuid/seguid. My guess is on chromeos >> all user-accessible mount point are set with noexec, so kernel will >> prevent the direct execution of static binaries and a possible loader. > > It's a frequently requested feature. Some security policies require > that all writable mounts must also be noexec. That doesn't do much good > if the bypass is trivial. Of course other bypasses remain (script > interpreters and suchlike, hence the need for O_MAYEXEC). Interesting, I was not aware of the O_MAYEXEC proposal. I think we might try to use on loader/dlfcn interfaces, but since we already mmap PROT_EXEC I think noexec that proper fails should be suffice.
On Fri, 23 Jul 2021, Adhemerval Zanella via Libc-alpha wrote: > > > $ mount | grep loopfs > /dev/loop0 on /home/azanella/loopfs type ext4 (rw,*noexec*,relatime,seclabel) > $ LD_PRELOAD=loopfs/libpreload.so ./test > ERROR: ld.so: object 'loopfs/libpreload.so' from LD_PRELOAD cannot be preloaded: ignored. > $ strace env LD_PRELOAD=loopfs/libpreload.so ./test > [...] > mmap(NULL, 2101304, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = -1 EPERM (Operation not permitted) > [...] > > I haven't tested a vanilla kernel yet. Vanilla kernel also does that, but I think you might be missing Jordan's point: this is not about refusing mmap from a noexec mount, this is a counter-measure against specially crafted ELF files taking over the dynamic loader even before it attempts a PROT_EXEC mmap, as demonstrated in glibc bug #21718: https://sourceware.org/bugzilla/show_bug.cgi?id=21718 Alexander
Hi all, Thanks for the wonderful feedback and discussion! I did want to hop in to clarify a few things and give an update on the code quality. >>>> Trying to implement a noexec policy in userspace looks wrong to me. This is a fair point, and I've forwarded this concern to our security team. A kernel-level patch already exists for similar behaviour here: https://lore.kernel.org/kernel-hardening/20201203173118.379271-1-mic@digikod.net/T/ This proposal is for the trusted_for syscall, though it's stalled for a bit it seems. I'll have to discuss with my team on our priorities if we want to move forward with that. Ultimately, a change will need to be done on the glibc side of things even with a kernel patch. Perhaps not with an fstatfs check, but rather with a new syscall with more fine grained control. Our security folk seem to believe it could be cleaner and more robust with a kernel+glibc change. That being said, according to them, a change solely in glibc is not out-of-line. To quote Jann Horn from our security team on this: > Either way, enforcing this kind of policy will require _some_ kind of > change to glibc, the kernel can't do it on its own. But regarding > whether it just should be a glibc change or a glibc+kernel change, I > think you could argue both ways: You could argue that this is a > system-wide policy decision that would ideally be directly steered by > the kernel somehow, or you could argue that this is already a fixed > policy documented in manpages (see > https://man7.org/linux/man-pages/man2/mmap.2.html: "EPERM The prot > argument asks for PROT_EXEC but the mapped area belongs to a file on a > filesystem that was mounted no-exec") and therefore it's fine for > glibc to mirror that policy in cases where the kernel doesn't have > enough information to do it on its own. The intent of the patch was meant as a way for glibc to mirror the safety policy when the kernel cannot know. Perhaps the solution should be to instead fix the kernel (i.e. trusted_for()). In response to Alexander, > this is not about refusing mmap from a noexec mount, this is a counter-measure > against specially crafted ELF files taking over the dynamic loader even before > it attempts a PROT_EXEC mmap, as demonstrated in glibc bug #21718 Indeed, it's not about refusing an mmap from a noexec mount. These are handcrafted ELF files which take over the loader. If you would like to see the proof of concept of the attack, we can add you to the allowed listings for the linked bug thread (https://bugs.chromium.org/p/chromium/issues/detail?id=1182687). We have an ELF for x86_64 devices which shows the above behaviour on our current kernel of ChromeOS. I'm uncertain of the status of other OSes (I would need to reach out again to our security team), but I doubt it would be restricted to just ChromeOS. On the actual code: Oops! It looks like a rebase messed up with the patch. It was originally written for glibc 2.32, and porting it to ToT glibc has led to mistakes. Doesn't look like I ran through testing with the rebase! Apologies for this. I'll work on correcting and testing the code once I've gotten confirmation that this is the correct approach on your end. Thankfully it doesn't seem like there's much mechanically wrong. It's just a question as to whether this is a sane approach to the problem at all. Thanks, ~Jordan On Fri, Jul 23, 2021 at 2:27 PM Alexander Monakov <amonakov@ispras.ru> wrote: > > On Fri, 23 Jul 2021, Adhemerval Zanella via Libc-alpha wrote: > > > > > > > $ mount | grep loopfs > > /dev/loop0 on /home/azanella/loopfs type ext4 (rw,*noexec*,relatime,seclabel) > > $ LD_PRELOAD=loopfs/libpreload.so ./test > > ERROR: ld.so: object 'loopfs/libpreload.so' from LD_PRELOAD cannot be preloaded: ignored. > > $ strace env LD_PRELOAD=loopfs/libpreload.so ./test > > [...] > > mmap(NULL, 2101304, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = -1 EPERM (Operation not permitted) > > [...] > > > > I haven't tested a vanilla kernel yet. > > Vanilla kernel also does that, but I think you might be missing Jordan's point: > this is not about refusing mmap from a noexec mount, this is a counter-measure > against specially crafted ELF files taking over the dynamic loader even before > it attempts a PROT_EXEC mmap, as demonstrated in glibc bug #21718: > https://sourceware.org/bugzilla/show_bug.cgi?id=21718 > > Alexander
On 23/07/2021 18:27, Alexander Monakov wrote: > On Fri, 23 Jul 2021, Adhemerval Zanella via Libc-alpha wrote: > >> >> >> $ mount | grep loopfs >> /dev/loop0 on /home/azanella/loopfs type ext4 (rw,*noexec*,relatime,seclabel) >> $ LD_PRELOAD=loopfs/libpreload.so ./test >> ERROR: ld.so: object 'loopfs/libpreload.so' from LD_PRELOAD cannot be preloaded: ignored. >> $ strace env LD_PRELOAD=loopfs/libpreload.so ./test >> [...] >> mmap(NULL, 2101304, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = -1 EPERM (Operation not permitted) >> [...] >> >> I haven't tested a vanilla kernel yet. > > Vanilla kernel also does that, but I think you might be missing Jordan's point: > this is not about refusing mmap from a noexec mount, this is a counter-measure > against specially crafted ELF files taking over the dynamic loader even before > it attempts a PROT_EXEC mmap, as demonstrated in glibc bug #21718: > https://sourceware.org/bugzilla/show_bug.cgi?id=21718 > > Alexander > Yes I am fully aware of that, but this is not what was the idea for the loader laid out some years ago [1]. So maybe we should discuss again the design goal and reevaluate some ideas. For instance, should we discuss to remove the multiple features that are really a security hazards that we still support due compatibility? What about somewhat useful features that contains multiple issue and possible security issue like rtld-audit? I tend to agree with Siddhesh, but at same time trying to parse ill-formed ELF files are not easy tasks and might clash with current idea of a somewhat simple and fast loader. We already ave multiple layers or security and filtering in current modern OS, will adding another complexity on the loader really pay off? If we really want to focus on loader hardening, I think we really should use Carlos's approach along with the idea of starting *deprecate* and remove some features, even if it will result in incompatibilities (yes, some may frown upon it, but it past time to really start to stop support things like exec-stack, text-rels, and ldd.bash.in). [1] https://sourceware.org/pipermail/libc-alpha/2015-July/062464.html
* Jordan Abrahams: > In response to Alexander, > >> this is not about refusing mmap from a noexec mount, this is a counter-measure >> against specially crafted ELF files taking over the dynamic loader even before >> it attempts a PROT_EXEC mmap, as demonstrated in glibc bug #21718 > > Indeed, it's not about refusing an mmap from a noexec mount. These are > handcrafted ELF files which take over the loader. If you would like to > see the proof of concept of the attack, we can add you to the allowed > listings for the linked bug thread > (https://bugs.chromium.org/p/chromium/issues/detail?id=1182687). We > have an ELF for x86_64 devices which shows the above behaviour on our > current kernel of ChromeOS. I'm uncertain of the status of other OSes > (I would need to reach out again to our security team), but I doubt it > would be restricted to just ChromeOS. There have been public discussions about such bugs before, not sure why you are restricting it. I expect that it will always be possible to take over the dynamic loader until we are more restrictive when it comes to use of MAP_FIXED in the loader, probably by using MAP_FIXED_NOREPLACE. The challenge is to make the change so conservative that existing (slightly broken, or ia64) binaries are not impacted. Thanks, Florian
Hi Florian, > There have been public discussions about such bugs before, not sure why > you are restricting it. I apologise. We can add any interested individuals to view the bug thread (including you! Just let me know). However it's not in my ability to make that thread public. Our security team originally wished access to it to be restricted, and I'm obligated to abide by that. I assume it's restricted because it has an example program which bypasses noexec on ChromeOS devices, and Google doesn't want to risk anything. I _can_ forward a request along to make it public, but I'm afraid it's not my decision or in my power to do so. > I expect that it will always be possible to take over the dynamic loader > until we are more restrictive when it comes to use of MAP_FIXED in the > loader, probably by using MAP_FIXED_NOREPLACE. The challenge is to make > the change so conservative that existing (slightly broken, or ia64) > binaries are not impacted. Given this, what are our options on getting dynamic loader attack preventions upstream? For example, we don't have ia64 testing ourselves, so we can't verify no impact even if we attempted to patch the loader on our side. We'd obviously like to have a more permanent upstream fix. O_NEEDEXEC / O_MAYEXEC seems like a fine long term solution in the kernel, and we're trying to weigh the benefits of that versus maintaining this glibc noexec patch locally. We don't know how long that would take however, and our toolchain team certainly doesn't have the needed expertise or cycles at this time. Plus we'd still also need an interim solution while that is being worked on. Thanks, Jordan
On Mon, Aug 23, 2021 at 1:30 PM Florian Weimer <fweimer@redhat.com> wrote: > > * Jordan Abrahams: > > > In response to Alexander, > > > >> this is not about refusing mmap from a noexec mount, this is a counter-measure > >> against specially crafted ELF files taking over the dynamic loader even before > >> it attempts a PROT_EXEC mmap, as demonstrated in glibc bug #21718 > > > > Indeed, it's not about refusing an mmap from a noexec mount. These are > > handcrafted ELF files which take over the loader. If you would like to > > see the proof of concept of the attack, we can add you to the allowed > > listings for the linked bug thread > > (https://bugs.chromium.org/p/chromium/issues/detail?id=1182687). We > > have an ELF for x86_64 devices which shows the above behaviour on our > > current kernel of ChromeOS. I'm uncertain of the status of other OSes > > (I would need to reach out again to our security team), but I doubt it > > would be restricted to just ChromeOS. > > There have been public discussions about such bugs before, not sure why > you are restricting it. > > I expect that it will always be possible to take over the dynamic loader > until we are more restrictive when it comes to use of MAP_FIXED in the > loader, probably by using MAP_FIXED_NOREPLACE. The challenge is to make > the change so conservative that existing (slightly broken, or ia64) > binaries are not impacted. My understanding is that the ChromeOS security folks (I'm not on that team) would like to ensure that, as a security hardening measure, attempting to load an attacker-controlled library from a noexec mount doesn't give the attacker the ability to run code in the context of the loader. As far as I understand, the kind of ELF file I used in the linked bug (sorry about the access restriction) looks fairly legitimate: It doesn't clobber any existing mappings, instead it uses a ".init_array" section containing a relocation into glibc, like this: .section .init_array,"aw" .align 8 .quad rmdir+0x774 By picking an appropriate gadget in glibc, and making use of the register contents at the indirect call to the fake constructor function, it is possible to use that to e.g. start a ROP chain. I think there are also more fundamental things that a library without any executable segments could do, by design, to compromise the integrity of the process - e.g. depending on when the evil library is loaded, it might be able to override a pointer symbol that was supposed to be located in a ".bss" section to point at the evil library's ".data" section instead and use that to effectively clobber a NULL-initialized pointer variable with an arbitrary value, or something like that? This is why I think that as long as you accept the premise that NOEXEC should robustly prevent library loading, the loader (possibly in collaboration with the kernel) has to ensure that even libraries without executable segments cannot be loaded from non-executable files. (Btw, this also means that IMA-based auditing of library loading based on executable mappings is currently bypassable. I don't think anyone really cares about that though - I brought it up with the IMA maintainers back in 2018 and IIRC they didn't see it as a problem.)
On Mon, Aug 23, 2021 at 7:30 AM Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> wrote: > The challenge is to make > the change so conservative that existing (slightly broken, or ia64) > binaries are not impacted. I don't see why a deceased arch has to block any other work.. that would be very weird.. something that is no longer shipping and its manufacturer has given up on shouldn't be stopping free software....
On 23/08/2021 20:40, Cristian RodrÃguez wrote: > On Mon, Aug 23, 2021 at 7:30 AM Florian Weimer via Libc-alpha > <libc-alpha@sourceware.org> wrote: >> The challenge is to make >> the change so conservative that existing (slightly broken, or ia64) >> binaries are not impacted. > > I don't see why a deceased arch has to block any other work.. that > would be very weird.. something that is no longer shipping and its > manufacturer has given up on shouldn't be stopping free software.... > It should not indeed, if an architecture does not support the best way forward is to just disable the feature on the affected ones. I think what Florian is saying it how to properly enable it without breaking other architectures.
* Jann Horn: > My understanding is that the ChromeOS security folks (I'm not on that > team) would like to ensure that, as a security hardening measure, > attempting to load an attacker-controlled library from a noexec mount > doesn't give the attacker the ability to run code in the context of > the loader. > > As far as I understand, the kind of ELF file I used in the linked bug > (sorry about the access restriction) looks fairly legitimate: It > doesn't clobber any existing mappings, instead it uses a ".init_array" > section containing a relocation into glibc, like this: > > .section .init_array,"aw" > .align 8 > .quad rmdir+0x774 > > By picking an appropriate gadget in glibc, and making use of the > register contents at the indirect call to the fake constructor > function, it is possible to use that to e.g. start a ROP chain. Ah, that really needs trusted_for to fix. Many distributions do not even mark shared objects as executable, so executable permission is not something we can enforce in the dynamic loader. > I think there are also more fundamental things that a library without > any executable segments could do, by design, to compromise the > integrity of the process - e.g. depending on when the evil library is > loaded, it might be able to override a pointer symbol that was > supposed to be located in a ".bss" section to point at the evil > library's ".data" section instead and use that to effectively clobber > a NULL-initialized pointer variable with an arbitrary value, or > something like that? A symbol could have a value that is not within the current shared object, pointing into some other shared object's executable segment. I don't think ELF has a generic redirection mechanism that isn't based on code execution, though. There is something limited for canonical function addresses, but I'm not sure if it can be used this way. > (Btw, this also means that IMA-based auditing of library loading based > on executable mappings is currently bypassable. I don't think anyone > really cares about that though - I brought it up with the IMA > maintainers back in 2018 and IIRC they didn't see it as a problem.) IMA only covers the contents, I think. Neither paths nor even the most basic file attributes are covered by the digest. Thanks, Florian
On 06 Sep 2021 17:10, Florian Weimer via Libc-alpha wrote: > * Jann Horn: > > My understanding is that the ChromeOS security folks (I'm not on that > > team) would like to ensure that, as a security hardening measure, > > attempting to load an attacker-controlled library from a noexec mount > > doesn't give the attacker the ability to run code in the context of > > the loader. > > > > As far as I understand, the kind of ELF file I used in the linked bug > > (sorry about the access restriction) looks fairly legitimate: It > > doesn't clobber any existing mappings, instead it uses a ".init_array" > > section containing a relocation into glibc, like this: > > > > .section .init_array,"aw" > > .align 8 > > .quad rmdir+0x774 > > > > By picking an appropriate gadget in glibc, and making use of the > > register contents at the indirect call to the fake constructor > > function, it is possible to use that to e.g. start a ROP chain. > > Ah, that really needs trusted_for to fix. > > Many distributions do not even mark shared objects as executable, so > executable permission is not something we can enforce in the dynamic > loader. we shouldn't let bad distro decisions constrain possibilities. we have the ability to move the ecosystem. we can add new features that are disabled by default via configure options with warnings (including in NEWS) that these will change in the future. distros will migrate if they want newer glibc. -mike
diff --git a/elf/dl-load.c b/elf/dl-load.c index 650e4edc35..81ca08c38f 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -29,6 +29,7 @@ #include <sys/mman.h> #include <sys/param.h> #include <sys/stat.h> +#include <sys/statvfs.h> #include <sys/types.h> #include <gnu/lib-names.h> @@ -1572,6 +1573,20 @@ print_search_path (struct r_search_path_elem **list, _dl_debug_printf_c ("\t\t(%s)\n", what); } +/* Check if a the passed in file descriptor points to file on an executable mount. */ +static int +check_exec (int fd) +{ + struct statvfs buf; + int stated = fstatvfs (fd, &buf); + if (stated == 0) + { + return !(buf.f_flag & ST_NOEXEC); + } + /* Could not fstat the file. */ + return false; +} + /* Open a file and verify it is an ELF file for this architecture. We ignore only ELF files for other architectures. Non-ELF files and ELF files with different header information cause fatal errors since @@ -1650,8 +1665,8 @@ open_verify (const char *name, int fd, #endif if (fd == -1) - /* Open the file. We always open files read-only. */ - fd = __open64_nocancel (name, O_RDONLY | O_CLOEXEC); + /* Open the file. We always open files read-only. */ + fd = __open64_nocancel (name, O_RDONLY | O_CLOEXEC); if (fd != -1) { @@ -1667,6 +1682,14 @@ open_verify (const char *name, int fd, __set_errno (0); fbp->len = 0; assert (sizeof (fbp->buf) > sizeof (ElfW(Ehdr))); + + /* Before we read in the file, check if the file is in an exec mount */ + if (__glibc_unlikely (!check_exec(fd))) + { + errstring = N_("file not located on exec mount"); + goto call_lose; + } + /* Read in the header. */ do {
I'm from Google's ChromeOS Toolchain team, and I have a security hardening patch to port upstream. I've confirmed with our security team that this patch can be public, and I'm looking for review and feedback. I'm a new contributor, but Google already has a CLA on file for the FSF, so it should not be an issue. If I'm missing any contribution info, please do let me know. Best regards, ~Jordan R Abrahams -- >8 -- This commit hardens a security flaw in dl-load.c. At present, one could dynamically load any shared object files even if they resided on a NOEXEC mount partition. This introduces an exploit where an attacker may get around this NOEXEC requirement by preloading a malicious shared library. For example, from shell this can look like: $ LD_PRELOAD='/tmp/malicious_lib.so' id Hello world from malicious_lib! Which will work even if /tmp/ is a noexec mount. A proof of concept of this exploit can be found here at https://bugs.chromium.org/p/chromium/issues/detail?id=1182687 The bug thread is restricted, but we are willing to add interested parties with the approval of our security team. This patch hardens against the exploit by checking the file descriptor if the file lies in a NOEXEC mount partition via an fstatvfs call. The error string is set, and we jump to the `lose` cleanup code. With this patch, the above example instead becomes: $ LD_PRELOAD='/tmp/malicious_lib.so' id /usr/bin/id: error while loading shared libraries: /tmp/malicious_lib.so: file not located on exec mount # <... normal stdout for id ...> Signed-off-by: Jordan R Abrahams <ajordanr@google.com> --- elf/dl-load.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)