Safe Library::new() can call any unsafe global constructors

[ghost]

This is unsound. It should be an unsafe function instead.

For example, this on x86_64-unknown-linux-gnu:

Cargo.toml:

[workspace]
members = ["evil", "loader"]

evil/Cargo.toml:

[package]
name = "evil"
version = "0.0.0"
edition = "2018"

[lib]
crate-type = ["cdylib"]

evil/src/lib.rs:

unsafe extern "C" fn evil() -> ! {
    std::hint::unreachable_unchecked()
}

#[link_section = ".init_array"]
#[used]
static EVIL: unsafe extern "C" fn() -> ! = evil;

loader/Cargo.toml:

[package]
name = "loader"
version = "0.0.0"
edition = "2018"

[dependencies]
libloading.git = "https://github.com/nagisa/rust_libloading.git"

loader/src/main.rs:

#![forbid(unsafe_code)]

use libloading::Library;

fn main() {
    let mut path = std::env::current_exe().unwrap();
    assert!(path.pop());
    path.push("libevil.so");
    Library::new(path).unwrap();
}

Triggers SIGILL (in debug mode) because std::hint::unreachable_unchecked() is called.

Backtrace

#0  core::hint::unreachable_unchecked ()
#1  0x00007ffff7fc2119 in evil::evil ()
#2  0x00007ffff7fe0b8a in ?? () from /lib64/ld-linux-x86-64.so.2
#3  0x00007ffff7fe0c91 in ?? () from /lib64/ld-linux-x86-64.so.2
#4  0x00007ffff7edb915 in __GI__dl_catch_exception (exception=<optimized out>, operate=<optimized out>, args=<optimized out>)
#5  0x00007ffff7fe50bf in ?? () from /lib64/ld-linux-x86-64.so.2
#6  0x00007ffff7edb8b8 in __GI__dl_catch_exception (exception=<optimized out>, operate=<optimized out>, args=<optimized out>)
#7  0x00007ffff7fe45fa in ?? () from /lib64/ld-linux-x86-64.so.2
#8  0x00007ffff7fa934c in dlopen_doit (a=a@entry=0x7fffffffd380)
#9  0x00007ffff7edb8b8 in __GI__dl_catch_exception (exception=exception@entry=0x7fffffffd320, operate=<optimized out>, args=<optimized out>)
#10 0x00007ffff7edb983 in __GI__dl_catch_error (objname=0x5555555a3ca0, errstring=0x5555555a3ca8, mallocedp=0x5555555a3c98, operate=<optimized out>, args=<optimized out>)
#11 0x00007ffff7fa9b59 in _dlerror_run (operate=operate@entry=0x7ffff7fa92f0 <dlopen_doit>, args=args@entry=0x7fffffffd380)
#12 0x00007ffff7fa93da in __dlopen (file=<optimized out>, mode=<optimized out>)
#13 0x000055555555a7f4 in libloading::os::unix::Library::open::{{closure}} ()
#14 0x000055555555a276 in libloading::os::unix::with_dlerror (
    wrap=0x55555555b2a0 <core::ops::function::FnOnce::call_once>, closure=...)
#15 0x000055555555a64f in libloading::os::unix::Library::open (filename=..., flags=2)
#16 0x000055555555a402 in libloading::os::unix::Library::new (filename=...)
#17 0x000055555555b52c in libloading::Library::new (filename=...)
#18 0x000055555555b618 in loader::main ()

Similarly, Library::new() calls the unsafe DllMain() on Windows.

[nagisa]

Thank you for the report. That's indeed a pretty nasty one!

I think there are a number of tradeoffs and prior arts to consider here:

Firstly, I believe the same kind of issue present when linking to a dynamic library through regular means (#[link(name="...") or -l argument or cargo:rustc-link-lib=[KIND=]NAME). Those are also considered “safe” in that they don't require users to acknowledge this could cause the application to run unsound code. Linking to an evil library like this will fault all the same, except earlier on.

On the other hand, the lack of unsafe annotation on Library::new may lead to false sense of security and users of the library thinking this cannot crash or be unsound, ever. This is also not true, as your demonstration already shows.

This problem is significantly more interesting in the context of it being extremely easy to write a library that has unsound initializers in C++ or other such languages.

---

I don't think there is much problem with making Library::new unsafe, given that utilizing the loaded library requires use of an unsafe Library::get anyway. But it is also a comparatively huge breaking change to the API that will affect literally every user of this library trying to upgrade.

I wonder if there's anything in the way of requirements or invariants that the code calling Library::new would need to verify and satisfy in order to ensure that the call cannot be unsound. I suspect the answer will be "generally not". In that case what would libloading's documentation say about the (un)safety of Library::new?

# Safety

This call will execute initializers present in the loaded libraries.
These may be unsound and there's nothing you can do about it?

And then the users trying to explain why the code they're calling is actually safe are forced to...

let library = unsafe {
    // SAFE: uh… lets just hope this solib does not have evil initializers?
    Library::new("libpossiblyevil.so")
};

Welp.

[ghost]

Firstly, I believe the same kind of issue present when linking to a dynamic library through regular means (#[link(name="...") or -l argument or cargo:rustc-link-lib=[KIND=]NAME). Those are also considered “safe” in that they don't require users to acknowledge this could cause the application to run unsound code. Linking to an evil library like this will fault all the same, except earlier on.

Indeed. However, in that case, the dynamic library is typically a build dependency, maybe (at least I hope) the user is more likely to acknowledge that it may be evil (just like they're statically linking an unsound Rust crate) or may be replaced with an evil one at runtime.

I wonder if there's anything in the way of requirements or invariants that the code calling Library::new would need to verify and satisfy in order to ensure that the call cannot be unsound. I suspect the answer will be "generally not". In that case what would libloading's documentation say about the (un)safety of Library::new?

I think Library::get() has the same issue. Its documentation says:

Using a value with wrong type is undefined.

But I think in most cases the user actually can't verify the type of the symbol.
(So "there's nothing you can do about it" may be fine...)

[nagisa]

I went ahead and prepared a fix for #87. I will let it bake for a week and then release the new version the next weekend.

(I also want to look into whether there might be things like… destructors. I know that they exist for TLS, but that is not strictly coupled to loading/unloading the library and could be attributed more to the TLS handling code instead)

Thanks for the report!

[ghost]

(I also want to look into whether there might be things like… destructors. I know that they exist for TLS, but that is not strictly coupled to loading/unloading the library and could be attributed more to the TLS handling code instead)

Not all destructors could be "attributed more to the TLS handling code":

evil/src/lib.rs (just reusing the library name, actually not evil in this case):

extern "C" fn print() {
    eprintln!("global destructor called");
}

#[link_section = ".fini_array"]
#[used]
static PRINT: extern "C" fn() = print;

loader/src/main.rs:

#![forbid(unsafe_code)]

use libloading::Library;

fn main() {
    let mut path = std::env::current_exe().unwrap();
    assert!(path.pop());
    path.push("libevil.so");
    Library::new(path).unwrap();
    eprintln!("library dropped");
}

That prints:

global destructor called
library dropped

Maybe they worth mentioning?

[nagisa]

Yeah, I suspected they'd exist, just didn't have the time to look into them. Thanks for bringing these up!

For these there are two distinct options I guess: could make Library::close unsafe (and possibly remove the Drop implementation) as well; or, as you suggest, could roll up this into safety invariants of Library::new as once the library loaded there's little in the way (e.g. mem::forget) that can prevent destructors from running. I'm perhaps partial to the latter option, but I'll take my time pondering this.