Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
briansmith opened this issue May 21, 2024 · 6 comments
Closed

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

briansmith opened this issue May 21, 2024 · 6 comments

Comments

@briansmith
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

newpavlov commented May 21, 2024

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

@josephlr
Copy link
Member

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
Copy link
Member

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

@briansmith
Copy link
Contributor Author

This will actually be fixed with #487, which requires also #484.

@briansmith
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants