Add optional support for extern_item_impls

[bushrat011899]

Objective

• Experiment with using extern_item_impls in getrandom.

Solution

• Added a new opt-in backend, extern_item_impls, which enables the nightly-only extern_item_impls` feature.
• Export the attribute macros provided by #[eii] in a new implementation module.
• Updated the READEME.md to provide basic usage information.

Example

With extern_item_impls enabled, users can provide (or override) the implementations of the 3 backend functions used by getrandom with a simple attribute macro:

use core::mem::MaybeUninit;

#[getrandom::implementation::fill_uninit]
fn my_fill_uninit_implementation(
    dest: &mut [MaybeUninit<u8>]
) -> Result<(), getrandom::Error> {
    // ...
    Ok(())
}

If an implementation is not provided by the user (or an appropriate library) the compiler will provide a helpful error message during compilation informing the user that they must provide an implementation:

error: `#[fill_uninit]` required, but not found
   --> src\backends.rs:27:53
    |
 27 |         #[cfg_attr(getrandom_extern_item_impls, eii(fill_uninit))]
    |                                                     ^^^^^^^^^^^ expected because `#[fill_uninit]` was declared here in crate `getrandom`
...
205 |                 implementation!();
    |                 ----------------- in this macro invocation
    |
    = help: expected at least one implementation in crate `foo` or any of its dependencies
    = note: this error originates in the macro `implementation` (in Nightly builds, run with -Z macro-backtrace for more info)

If two implementations are provided (e.g., a library and the user) then another error message is provided:

error: multiple implementations of `#[fill_uninit]`
  --> src\lib.rs:81:1
   |
81 | / fn my_first_fill_uninit_implementation(
82 | |     _dest: &mut [MaybeUninit<u8>]
83 | | ) -> Result<(), Error> {
   | |______________________^ first implemented here in crate `foo`
...
88 | / fn my_second_fill_uninit_implementation(
89 | |     _dest: &mut [MaybeUninit<u8>]
90 | | ) -> Result<(), Error> {
   | |______________________- also implemented here in crate `bar`
   |
   = help: an "externally implementable item" can only have a single implementation in the final artifact. When multiple implementations are found, also in different crates, they conflict

Open Questions

• Should this be merged even in a nightly-only fashion? No new syntax is introduced by the feature, so even if the feature is removed from nightly entirely, getrandom will continue to function correctly as long as the Rust flag isn't used. Noting as well that the efi_rng backend also relies on a nightly feature, uefi_std. Generally agreeable.
• If this was stable, should it be the default? Probably
• If there wasn't a Rust flag required to enable this functionality (e.g., on by default or through a Cargo feature), should there be a way for the user to defensively disable it for security reasons? For example, to prevent a malicious dependency changing the source of randomness.

---

Notes

• Offering this PR largely as a hypothetical, since I'm uncertain if/when extern_item_impls will be stabilised. I hope relatively soon, since even the current implementation seems vastly cleaner than #[unsafe(no_mangle)] extern "Rust" fn ....
• I personally have very little availability to maintain this feature (our daughter just hit 6 months old and work is a slog!), so if this is to be merged, please keep that in mind. I don't want to push a feature onto a project if the current maintainers don't want to maintain it.

[newpavlov]

extern_item_impls certainly looks very interesting! But I would prefer if it was implemented as a new opt-in backend.

Should this be merged even in a nightly-only fashion?

Yes, it's fine.

If this was stable, should it be the default?

I think yes, assuming it satisfies all our needs. But we probably should introduce such change only in a new breaking release.

If there wasn't a Rust flag required to enable this functionality, should there be a way for the user to defensively disable it for security reasons?

I am not sure... On one hand, I would prefer if it was impossible to silently overwrite default source without downstream user knowledge, but on the other hand we have #[global_allocator] which allows a similar thing...

[dhardy]

This would indeed be a nice feature to have.

I would suggest using an externally-implementable trait. Certainly the default impls for u32 and u64 should be implemented over fill_inner and not over the default backend. Also, we shouldn't allow crate A to provide fill_inner and crate B to provide inner_u32; at most one crate should supply the custom backend.

Yes, it's fine.

Do we have a real use-case for this yet? Once stable it should likely replace the current custom backend system; in the mean-time is there any actual reason to support two methods?

but on the other hand we have #[global_allocator] which allows a similar thing

I'm not sure that has the same security implications. Personally I prefer that custom backends be off-by-default with only the binary being able to enable the functionality (which is approximately what rust flags achieve).

[newpavlov]

I would suggest using an externally-implementable trait.

Can it work with the rand_core::TryRng trait? We could make rand_core non-optional dependency in a future breaking release.

in the mean-time is there any actual reason to support two methods?

I think it's worth to test extern_item_impls as an opt-in backend to find any potential issues. Additionally, for some users it may be a worthwhile alternative to the custom opt-in backend.

[dhardy]

Can it work with the rand_core::TryRng trait?

I don't think so... in any case, the Error type is fixed here and next_u32 / next_u64 should have default impls (assuming most custom backends must implement fill_bytes).

[bushrat011899]

extern_item_impls certainly looks very interesting! But I would prefer if it was implemented as a new opt-in backend.

Basically a copy of how the custom backend, just using the nightly feature instead of an extern function?

Should this be merged even in a nightly-only fashion?

Yes, it's fine.

I'll mark this PR as ready for review then!

If this was stable, should it be the default?

I think yes, assuming it satisfies all our needs. But we probably should introduce such change only in a new breaking release.

I think so too. This feels like a big enough change to warrant a new breaking release, especially since it makes the custom backend redundant (if users want the same functionality they can just define an extern function and call it in the eii method themselves).

If there wasn't a Rust flag required to enable this functionality, should there be a way for the user to defensively disable it for security reasons?

I am not sure... On one hand, I would prefer if it was impossible to silently overwrite default source without downstream user knowledge, but on the other hand we have #[global_allocator] which allows a similar thing...

Another option is we could publicly export whatever the default backend is. So users who want to ensure the default backend is used can just call the default in their own eii function.

[newpavlov]

Basically a copy of how the custom backend, just using the nightly feature instead of an extern function?

Yes. It also may require all 3 functions instead of just fill_bytes.

[bushrat011899]

I would suggest using an externally-implementable trait.

I do think that's a better fit, but I'm not sure how far along that feature is? It appears to me at least that externally implementable functions will land relatively soon.

Also, we shouldn't allow crate A to provide fill_inner and crate B to provide inner_u32; at most one crate should supply the custom backend.

I'm not sure if we can prevent that with the nightly feature as-is. We definitely could with extern traits though. We could also just advise libraries/users to override all 3 as best practice (maybe export inner_u32/64 so they can explicitly use the defaults).

Yes, it's fine.

Do we have a real use-case for this yet? Once stable it should likely replace the current custom backend system; in the mean-time is there any actual reason to support two methods?

My thinking is it's be good to allow nightly users to battle test the feature before eventually (maybe) relying on it more heavily once stable. It does have the benefit of allowing the override of the u32 and u64 methods, which the current custom backend cannot though.

but on the other hand we have #[global_allocator] which allows a similar thing

I'm not sure that has the same security implications. Personally I prefer that custom backends be off-by-default with only the binary being able to enable the functionality (which is approximately what rust flags achieve).

Thankfully this question can be kicked down the road for now, since it's a nightly feature and will require a flag until stabilised. Personally, I think this should be on by default in a future breaking release. If it was, the experience of using a 3rd party library as a backend would be as simple as including it as a dependency; no RUSTFLAGS shenanigans, and no confusing linker failure like with the current custom backend.

[bushrat011899]

I've updated this PR based on the provided feedback. The main difference now is I'm not checking for a default implementation of fill_inner based on the target. Instead, the extern_item_impls backend will always require a provided implementation of fill_uninit. Default implementations are provided for u32_inner and u64_inner, which can be overridden though. The main reason for this is to avoid duplicating the backend selection logic within the extern_item_impls module, or substantially modifying the layout of backends.rs. I do think in a future where EII is stable there is room for a pretty substantial refactor though.

[newpavlov]

It also would be nice to add a test and CI job for this opt-in backend, but we can leave it for a later PR if you don't want to work on it right now.

[tarcieri]

I'm not sure that has the same security implications. Personally I prefer that custom backends be off-by-default with only the binary being able to enable the functionality (which is approximately what rust flags achieve).

@bushrat011899 not to delay the feature, but perhaps in the future extern_item_impls could have some way for the "provider" crate to flag/signal that the external item imps they're providing are security-sensitive, and require explicit delegation of which crates are allowed to implement them somehow?

I could imagine that happening in Cargo.toml maybe?

[dependencies]
getrandom = { version = "...", allow-eii = { fill_uninit = "my-cool-rng-package" } }

[bushrat011899]

I imagine for security conscious applications one solution here would be to register the EII overrides yourself:

#[getrandom::implementations::fill_uninit]
fn my_fill_uninit(dest: &mut [MaybeUninit<u8>]) -> Result<(), getrandom::Error> {
    my_cool_rng_package::fill_uninit(dest)
}

#[getrandom::implementations::u32]
fn my_u32() -> Result<u32, getrandom::Error> {
    my_cool_rng_package::u32()
}

#[getrandom::implementations::u64]
fn my_u64() -> Result<u64, getrandom::Error> {
    my_cool_rng_package::u64()
}

Since this would turn any attempts by a malicious 3rd party library to override the implementations into a compilation error.

But it's maybe unnecessary anyway since, presumably, my-cool-rng-package could itself register its implementations. So any other malicious libraries would cause a compilation error anyway. The one area I could see a malicious crate sneaking in would be if getrandom added a new backend method without a major version bump. Since a malicious library could override just the new methods which my-cool-rng-package hadn't added implementations for yet.

[tarcieri]

@bushrat011899 I'm not worried about my-cool-rng-package.

I'm worried about the case where a user is expecting to be using the system RNG, and some other package with non-RNG related purposes suddenly gets a little bit of malware that takes over the RNG. It is a somewhat scary vector for breaking cryptography in a program (or being to exfiltrate the RNG secret or state so a third party can potentially decrypt things the program encrypts).

But at the end of the day letting malware slip into your build is catastrophic and it could also take over your RNG by munging your Cargo.toml and replacing getrandom with evilgetrandom or a dozen other vectors

[bushrat011899]

@tarcieri what I meant was if a user intends to use my-cool-rng-package but somehow evil-rng ends up in the build-pipeline, both crates will call the EII override, so that becomes a compilation error. So the security concern is limited to cases where a user intends for the getrandom default backend to be used, but evil-rng overrides it. In that case, a user could manually override the implementations themselves with getrandom's defaults (they'd need to be publicly exported to support this), since that would also turn the existence of evil-rng into a compilation error.

There are other options too though. EII could be behind a simple Cargo feature flag, so security conscious users could then use something like cargo-deny to fail CI if EII gets enabled when it's not supposed to be. A Rust flag could be added to getrandom to disable EII. Lots of potential ways to harden the dependency graph here.

[tarcieri]

If there were even just a way to turn it off globally and turn it on at an individual crate level, that would probably do the trick.

Anyway, seems like a cool feature!