rust-random / getrandom

A small cross-platform library for retrieving random data from (operating) system source
Apache License 2.0
264 stars 166 forks source link

Implementation of `dlsym`-based `Weak` is not sandbox-friendly #428

Closed briansmith closed 1 month ago

briansmith commented 1 month ago

The comment says:

    // However, if by chance libc::dlsym does return UNINIT, there will not
    // be undefined behavior. libc::dlsym will just be called each time ptr()
    // is called. This would be inefficient, but correct.

It seems like we're converging on the idea that before a sandbox is enabled, getrandom::get_random[_uninit]() must be called once. Is it also required that it return Ok(_) before the sandbox can be enabled? That might be too strict of a requirement.

But, if we don't require it to return Ok(_) at least once, then the application may enable its sandbox, in which case calling dlsym would not be "inefficient, but correct" as it will instead likely kill the process.

briansmith commented 1 month ago

The above is an exaggeration because in almost every target architecture, a function pointer will never be 1, and we have const UNINIT: *mut c_void = 1 as *mut c_void; since functions are almost often required to be aligned on even addresses, and also the lowest word of the address space is often reserved for allowing such things.

newpavlov commented 1 month ago

Note that #427 changes UNINIT from 1 to usize::MAX. It should make the "inefficient" scenario even less likelier.

josephlr commented 1 month ago

One other thing to note: calling libc::dlsym(RTLD_DEFAULT, ...) (or GetProcAddress on Windows) inside a sandbox probably works just fine, provided the symbol you are looking up has already had its dynamic library loaded. Generally, it's things like libc::dlopen (or LoadLibrary on Windows) which cause issues as they load new code into a process. Things like lazy loading can complicate this, but that generally doesn't apply to libc.

So I think the current implementation is fine, even if the first call to getrandom was within a sandbox. In order to even call dlsym, libc.so will already have to be loaded, so the call to dlsym will either succeed if the symbol is present in libc, or fail otherwise.

newpavlov commented 1 month ago

I agree with @josephlr and think that we can close this issue.

briansmith commented 2 weeks ago

This will actually be fixed with https://github.com/rust-random/getrandom/pull/487, which requires also https://github.com/rust-random/getrandom/pull/484.

briansmith commented 2 weeks ago

In order to even call dlsym, libc.so will already have to be loaded, so the call to dlsym will either succeed if the symbol is present in libc, or fail otherwise.

This is assuming dlsym is provided by libc. It may have been replaced by antivirus or a sandbox implementation. Some projects I have worked on were hoping to eventually disable dlsym in their sandbox, but I'm not sure if they ever succeeded. I imagine somebody has.