LD_PRELOAD should not be used for injecting security-critical code because of glibc fail-open behavior

[thejh]

If you use LD_PRELOAD for injecting security-critical code, that's extremely brittle because glibc will just plow ahead if loading LD_PRELOAD libraries fails:

$ LD_PRELOAD=/foo/bar/baz /bin/echo hello world
ERROR: ld.so: object '/foo/bar/baz' from LD_PRELOAD cannot be preloaded (cannot open shared object file): ignored.
hello world

If the executable runs without CAP_SYS_ADMIN, an attacker could e.g. deliberately trigger such an error condition by temporarily opening so many files that the system-global limit is reached and open() returns -ENFILE.

Therefore, anything that relies on LD_PRELOAD for sandboxing is probably a bad idea, unless you have a safety net that somehow ensures that the LD_PRELOAD operation succeeded.

[ignatk]

Yes, LD_PRELOAD is a bit fragile approach, that's why we propose an option of doing patchelf (or even linking on build time) to make the dependency permanent. In this case the application would refuse to start, if it can't load the library.

Also, both this library as well as seccomp on its own are sandboxing tools, which protect the rest of the system from a vulnerable/exploited application, but not the other way around. If the OS/operating environment is compromised (and if the attacker can exhaust the system limit for open files - it probably is), no application sandboxing can be relied upon.

[thejh]

The README currently reads to me as if the LD_PRELOAD approach is a suggested usage pattern. If that is not the case, it may be a good idea to add a big warning at the top of the "Dynamically linked executables" section.

[ignatk]

It is a suggested usage pattern, at least for now.

[ignatk]

Just to clarify more: while LD_PRELOAD has its downsides, we think it should be offered as an option still, because it helps in some cases, where patchelf is not possible. For example, a system with enabled and enforced Linux IMA might not allow patching executables, so LD_PRELOAD is the only? option.

Also, LD_PRELOAD is easier to configure. Going back to the global file descriptor example: in most well monitored production systems a global file descriptor exhaustion will likely cause many more issues and would not go unnoticed in the first place.

Finally, some other users pointed out that there are similar solutions using similar approaches and LD_PRELOAD specifically, like Google minijail project, so it seems other engineers find value in the approach.

[thejh]

For example, a system with enabled and enforced Linux IMA might not allow patching executables, so LD_PRELOAD is the only? option.

Or you could use a different loader, or let your code signing infrastructure sign the patched executable, or...

in most well monitored production systems a global file descriptor exhaustion will likely cause many more issues and would not go unnoticed in the first place

Even if it is a deliberately targeted, extremely short spike?

Finally, some other users pointed out that there are similar solutions using similar approaches and LD_PRELOAD specifically, like Google minijail project

That's not a reason to repeat their mistakes.

[ignatk]

Even if it is a deliberately targeted, extremely short spike?

This assumes the OS is already compromised. seccomp is as only good as a measure, if the OS is trustworthy. If it is not, any kernel security mechanism does not make sense. In other words, if I'm malicious and can do a deliberately targeted, extremely short spike or resource exhaust in the OS, most likely I will not bother with trying to bypass seccomp by timing exactly when a sandboxed process is starting, but will rather do other damage in an easier way.

[thejh]

In other words, even if an attacker only gains code execution in a process that has the ability to fork and to open /dev/null (and not much else), you consider the OS to be compromised?

[ignatk]

I'm not sure that only with fork and open("/dev/null") it is possible to get a "deliberately targeted, extremely short spike"
especially if you just take defaults: https://github.com/torvalds/linux/blob/7c30b859a947535f2213277e827d7ac7dcff9c84/fs/file_table.c#L400

[kieranclancy]

There are any number of other legitimate purposes for LD_PRELOAD (or other options like LD_AUDIT) that could be defeated if it fails to load the object for this kind of reason. If there's a bug here it can be argued that it's in glibc. If someone is that concerned, maybe a patch should instead be sent to glibc to add an environment variable like LD_EXIT_ON_ERROR=(preload,symbol-resolution,relocation,audit,...,any) so that one can be 100% confident that either a program starts with the LD_PRELOAD having been successful or it doesn't start at all.

[insomniacslk]

I hope I'm not misunderstanding the intention of the comments in defense of LD_PRELOAD, but to me this reads as "yeah LD_PRELOAD has known security issues, you should use patchelf instead, but we still suggest to use it".

At this point the README should at very least document the risks of using LD_PRELOAD, in my opinion, or it can be misleading. Security should always come with sane defaults, but when it doesn't, it should warn the users loudly.

[ignatk]

It seems the more generic problem outlined here is how to make sure the process we're sandboxing is actually sandboxed and the rules were applied. I've added a section to the README describing one of the ways: https://github.com/cloudflare/sandbox#check-the-status

[insomniacslk]

Thank you for adding that section @ignatk ! It's not the big warning that I was hoping for, but it's definitely an improvement