Windows: Switch to using ProcessPRNG by default.

ProcessPRNG was introduced in Windows 10, which is the minimum supported Windows for most Rust targets. Upstream change: rust-lang/rust#121337

BoringSSL now uses ProcessPRNG: https://github.com/google/boringssl/blob/2db0eb3f96a5756298dcd7f9319e56a98585bd10/crypto/rand_extra/windows.c#L64
Golang also switched to ProcessPRNG: https://go-review.googlesource.com/c/go/+/536235

Should also help with the Chromium Sandbox: https://issues.chromium.org/issues/40277768

It looks like this won't be quite as easy as we would like. The main issue is that BCryptPrimitives.dll doesn't have a corresponding library (.lib file), so we can't just use link(name="bcryptprimitives"). We are left with 4 options:

Use #[link(kind = "raw-dylib")]
- Only stabilized in Rust 1.71
- Adds call to dlltool during build time (on *-windows-gnu targets)
- Not supported by cranelift
Use windows-sys crate
- Takes external dependency (but it's similar to libc so thats fine)
- MSRV is 1.60
Manually write the DLL loading code
- This is what Go does and BoringSSL does
- Requires calling LoadLibrary and GetProcAddress
- Synchronization needed (LazyUsize or Similar)
Build our own .lib file like the windows crate does
- Allows us to avoid calling LoadLibrary and similar functions.
- Requires maintaining libraries for the ~8 different windows targets
- Requires figuring out how to build .lib files.

The fourth option would be to include your own pre-built .lib file (possibly generated by using raw-dylib and storing the output or else copying the windows-rs lib generator code).

Adds call to dlltool during build time

Note that msvc targets don't require dlltool.

@ChrisDenton

The fourth option would be to include your own pre-built .lib file (possibly generated by using raw-dylib and storing the output or else copying the windows-rs lib generator code).

Adds call to dlltool during build time

Note that msvc targets don't require dlltool.

Thanks! Updated the list. I do think that maintaining a collection of .lib files for each windows target is a lot more effort than just writing the ~20 lines of DLL loading code.

@ChrisDenton I have a question. If getrandom lazily calls LoadLibraryA/GetProcAddress on first use, while libstd uses raw-dylib, will this cause any issues? When experimenting, it seems like LoadLibraryA/GetProcAddress always return the same handle/address when run in the same process, so I think there's some sort of caching?

@newpavlov

@newpavlov @ChrisDenton I thinking of the following approach which would combine approaches (1) and (3).

Add a build.rs script to check the compiler version:
- Output cargo::rustc-cfg=raw_dylib if version >= 1.65 (See Stabilize raw-dylib for non-x86 rust-lang/rust#99916)
- Output cargo::rustc-cfg=raw_dylib_x86 if version >= 1.71 (See Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool rust-lang/rust#109677)
In Windows.rs we have a cfg_if block like:

cfg_if! {
    if #[cfg(all(raw_dylib, not(target_arch = "x86")))] {
        #[link(name = "bcryptprimitives", kind = "raw-dylib")]
        extern "system" { fn ProcessPrng(pbdata: *mut u8, cbdata: usize) -> BOOL; }
    } else if #[cfg(all(raw_dylib_x86, target_arch = "x86"))] {
        #[link(name = "bcryptprimitives", kind = "raw-dylib", import_name_type = "undecorated")]
        extern "system" { fn ProcessPrng(pbdata: *mut u8, cbdata: usize) -> BOOL; }
    } else {
        unsafe fn ProcessPrng(pbdata: *mut u8, cbdata: usize) -> BOOL {
            // Do the lazy DLL loading
        }
    }
}

@ChrisDenton

@ChrisDenton I have a question. If getrandom lazily calls LoadLibraryA/GetProcAddress on first use, while libstd uses raw-dylib, will this cause any issues? When experimenting, it seems like LoadLibraryA/GetProcAddress always return the same handle/address when run in the same process, so I think there's some sort of caching?

There's no problem with that. A DLL is only loaded once. Loading it more times indeed just returns the same module handle and increases the reference count.

@josephlr

@josephlr

combine approaches (1) and (3)

It would introduce a build script which will do nothing on all other targets.

We may consider delaying migration to ProcessPrng until getrandom v0.3, which can have a sufficiently high MSRV. Considering that rand is in the process of preparing new v0.9 release, it may be a good time to cut v0.3 for us as well.

@newpavlov

@newpavlov good idea. Looks like Rand 0.9's MSRV is 1.61, do you know if 1.71 would be too high?

I think regardless, we should move to ProcessPrng, and just always do the DLL loading (I can put up a draft PR to see how it looks).

I should add that the standard library is in general moving away from dynamic dll loading because it causes issues when used in some scenarios. E.g. attempting to load a library while the loader lock is already held can cause a deadlock or crash. This is especially important for std because HashMaps are used everywhere and, by default, use RandomState.

@ChrisDenton

Thanks for the heads up @ChrisDenton getrandom is also a transitive dependency of most crates in practice, so we also want to be sensitive to these concerns.

I should add that the standard library is in general moving away from dynamic dll loading because it causes issues when used in some scenarios. E.g. attempting to load a library while the loader lock is already held can cause a deadlock or crash. This is especially important for std because HashMaps are used everywhere and, by default, use RandomState.

Do you know if there is any way to call ProcessPrng on versions of Rust before 1.71 without loading a DLL? Or if there's a way to check if a DLL is already loaded (which it almost always will be) without calling LoadLibraryA?

In general, I wouldn't think that the approach taken by Go and BoringSSL would break too many things in practice, but it might just be that Google doesn't run into those specific problems. It might be the case that moving to ProcessPRNG with DLL loading isn't prefect, but fixes more problems than it creates.

Alternatively, we could just bump the MSRV to 1.71 on Windows, and have a lower MSRV on all other platforms.

Do you know if there is any way to call ProcessPrng on versions of Rust before 1.71 without loading a DLL?

The only way would be to use a pre-created lib file in one of the ways listed above (i.e. using the windows-targets crate or distributing a lib file with this crate).

Or if there's a way to check if a DLL is already loaded (which it almost always will be) without calling LoadLibraryA?

GetModuleHandleExA can be used to get a module that's already loaded.

In general, I wouldn't think that the approach taken by Go and BoringSSL would break too many things in practice, but it might just be that Google doesn't run into those specific problems. It might be the case that moving to ProcessPRNG with DLL loading isn't prefect, but fixes more problems than it creates.

For sure. It's fine in the vast majority of cases. It's only really TLS constructors/destructors or pre/post C main that may be an issue and only then if the DLL is not already loaded. Rust itself does not have any built-in support for TLS constructors (that's done lazily) or for running code pre/post main. However, people do use crates that implement such things. One example was someone setting up a logger when a DLL is loaded where the logger used a HashMap.

I imagine many sandboxes would block LoadLibrary too. Also, some projects do try to avoid dynamic loading in general. It would be good to have a way to opt out of using LoadLibrary, e.g. by having a msrv-1_71 feature.

@josephlr

@josephlr wrote:

@newpavlov good idea. Looks like Rand 0.9's MSRV is 1.61, do you know if 1.71 would be too high?

Many projects are sticking to MSRV <= 1.63 for the purpose of supporting Debian 8 which is out of mainstream support but which has some kind of extended (paid) support. Even some people who are infamous for their historical MSRV policies are at least trying to do so, so I imagine it is likely going to be difficult to move the MSRV beyond 1.63. I do support moving the MSRV to 1.63 though.

If we want to support Rust 1.63, then using windows-sys probably will be the best option. I would prefer to not introduce a Windows-specific feature.

Would code based on windows-sys have the same difficulties with sandboxes?

If we want to support Rust 1.63, then using windows-sys probably will be the best option. I would prefer to not introduce a Windows-specific feature.

I am using windows-sys in ring, but I am considering removing the dependency in favor of my own extern "C" bindings, because when MS updates windows-sys to a new version that isn't SemVer-compatible with the previous, then people feel the need to have ring updated with a new Cargo.toml that references the latest version. Otherwise, they end up having two versions of windows-sys in their dependency tree. This would be especially problematic if the newest version of windows-sys were to ever have an MSRV that is higher than ring's. It's probably less work for everybody to just avoid windows-sys for the small amounts of functionality that ring needs.

I expect that if getrandom were to use windows-sys, we'd end up with the same situation as ring.

Build our own .lib file like the windows crate does

Allows us to avoid calling LoadLibrary and similar functions.

Requires maintaining libraries for the ~8 different windows targets

Requires figuring out how to build .lib files.

It looks like windows-rs checks the .lib files into version control. I think we could have a script that uses libtool to extract the minimal info from the windows-rs *.lib files into a corresponding tiny set of *.lib files that we can then embed in getrandom. We could then just check in the minimal lib files into Git like windows-rs does. Because they will be tiny, it would be fine to have them all in one crate.

In the interim, why not add a "msrv_1_71" feature to getrandom. When built with msrv_1_71, we use #[link(kind = "raw-dylib") to get ProcessPRNG. By default, without the feature flag set, keep using RtlGenRandom. When we have a better solution, msrv_1_71 can become a no-op.

Not quite. We reserve the right to bump platform requirements, if the latest stable version of Rust did it. For example, we have dropped Windows XP support, even though Rust 1.36 theoretically supports it.

One advantage of getting 1.0 released, is that we can simplify our MSRV policy to be "we can drop platforms or increase the MSRV in a minor version release, but won't do so in a patch release"

I would prefer to not release 1.0 at the very least until the MSRV-aware resolver lands on stable and we are ready to bump crate's MSRV to encompass it. We also have some unresolved API questions like #365 and #281.

Bump the MSRV to 1.65 (noting that the MSRV is 1.71 if targeting x86 Windows)

This is still higher than 1.63 mentioned above and having a higher MSRV for a Tier 1 target is really undesirable.

@ChrisDenton

Release the next breaking change of getrandom (either v0.3 or v1.0)

I think we should avoid a SemVer-incompatible version upgrade if we can.

In that version, unconditionally use ProcessPRNG on normal -windows- targets with "raw-dylib"

Just to clarify my above comments, I don't object to bumping the Windows minimum supported version to Windows 10. Just, we should clarify the communication as the initial comment seemed to imply that doing so follows from what Rust 1.78 does, rather than us thinking it's a good idea independently even for older versions of Rust.

Bump the MSRV to 1.65

What is the benefit of a MSRV 1.65 vs 1.63?

I don't see any reason to have an MSRV less than #426. I don't think there's much reason right now to have an MSRV greater than 1.57 except for this raw-dylib issue.

(noting that the MSRV is 1.71 if targeting x86 Windows)

I think we should explore @ChrisDenton's ideas of creating a MSRV-1.63-compatible way to statically link bcryptprimitives.dll/ProcessPRNG instead.

I don't know if it would actually be problematic to have an MSRV higher than 1.63 for non-Linux targets, but having different MSRVs for different targets seems likely to cause pain.

@newpavlov

I think we should avoid a SemVer-incompatible version upgrade if we can.

I think we've considering bumping the MSRV at all in the past a breaking change (@newpavlov can confirm). If we stuck to that, any increase from the current MSRV 1.36 would be breaking. I'd be fine increasing the MSRV in a patch though. If you are using a rust version that old, you're almost certainly vendoring your deps as well.

Just to clarify my above comments, I don't object to bumping the Windows minimum supported version to Windows 10. Just, we should clarify the communication as the initial comment seemed to imply that doing so follows from what Rust 1.78 does, rather than us thinking it's a good idea independently even for older versions of Rust.

Makes sense. We should drop platforms because it makes sense. Windows 10 is ~9 years old, Windows 8.1 is EOL, and we would still have a way to target Windows 7 and 8, you would just need to specify a different target.

What is the benefit of a MSRV 1.65 vs 1.63?

I don't see any reason to have an MSRV less than #426. I don't think there's much reason right now to have an MSRV greater than 1.57 except for this raw-dylib issue.

I don't know if it would actually be problematic to have an MSRV higher than 1.63 for non-Linux targets, but having different MSRVs for different targets seems likely to cause pain.

1.65 stabilized raw-dylib for non-x86 targets, but I think you are right here. If we want to implement this in terms of raw-dylib, we should just increase the MSRV to 1.71.

I think we should explore @ChrisDenton's ideas of creating a MSRV-1.63-compatible way to statically link bcryptprimitives.dll/ProcessPRNG instead.

I think if we just depend on windows-targets we could have an MSRV as low as 1.56.

OK I updated #415 to use windows-targets and it seems to give a very clean and simple implementation. It would raise the MSRV to 1.56 though. windows-targets uses edition=2021. Looking at the implementation of that crate, it would still compile (with very minor changes) on edition=2018 all the way back to 1.31.

So we can either try to get windows-targets to use edition=2018, or we can bump our MSRV.

At this point I think doing LoadLibrary shenanigans doesn't make a lot of sense. This leaves the options of:

Use windows-targets crate (MSRV 1.56)
Use "raw-dylib" (MSRV 1.71, and other downsides noted in Windows: Switch to using ProcessPRNG by default. #414 (comment))

Personally I would prefer the first option (use windows-targets), but I'd be interested in what y'all think.

@newpavlov

I think we should avoid a SemVer-incompatible version upgrade if we can.

I think we've considering bumping the MSRV at all in the past a breaking change (@newpavlov can confirm). If we stuck to that, any increase from the current MSRV 1.36 would be breaking. I'd be fine increasing the MSRV in a patch though. If you are using a rust version that old, you're almost certainly vendoring your deps as well.

Treating conservative increases in MSRV as SemVer-breaking changes creates more problems than it solves. Realistically speaking, many, many projects I'm part of or am closely observing have a MSRV of 1.63. Including in particular cc-rs. So I think increasing the MSRV to 1.57 without bumping the SemVer minor version is the right thing to do. Note that Rust 1.57 was released Dec 2, 2021, about 2.5 years ago.

1.65 stabilized raw-dylib for non-x86 targets, but I think you are right here. If we want to implement this in terms of raw-dylib, we should just increase the MSRV to 1.71.

Using windows-sys/targets is much better than bumping MSRV to 1.64/1.71, regardless of how the version number of getrandom is updated.

I think we should explore @ChrisDenton's ideas of creating a MSRV-1.63-compatible way to statically link bcryptprimitives.dll/ProcessPRNG instead.

I think if we just depend on windows-targets we could have an MSRV as low as 1.56.

I think that is fine. As I mentioned in the PR, there's no reason AFAICT to prefer windows-targets over windows-sys since they seem to do the version bumping in sync with each other. Using windows-targets directly seems worse.

So we can either try to get windows-targets to use edition=2018, or we can bump our MSRV.

I don't think we should be bothering anybody with this. I feel like any effort towards that is would be wasting people's effort for no real benefit.

Treating conservative increases in MSRV as SemVer-breaking changes creates more problems than it solves. Realistically speaking, many, many projects I'm part of or am closely observing have a MSRV of 1.63. Including in particular cc-rs. So I think increasing the MSRV to 1.57 without bumping the SemVer minor version is the right thing to do. Note that Rust 1.57 was released Dec 2, 2021, about 2.5 years ago.

Using windows-sys/targets is much better than bumping MSRV to 1.64/1.71, regardless of how the version number of getrandom is updated.

I don't think we should be bothering anybody with this. I feel like any effort towards that is would be wasting people's effort for no real benefit.

Agreed on all these.

I think that is fine. As I mentioned in the PR, there's no reason AFAICT to prefer windows-targets over windows-sys since they seem to do the version bumping in sync with each other. Using windows-targets directly seems worse.

I agree that they seem to have similar versions/MSRVs, and I initially looked at using windows-sys. But it seems like:

Using windows-targets allows us to have significantly lower compile times.
- Even just selecting the Win32_Foundation (needed for ProcessPrng) causes a huge number of items to be generated.
Directly using windows-targets seems to be done by "foundational" "low level" crates which need 1-2 simple Windows bindings.

Note that in those cases it's typical to use windows-bindgen to generate the bindings. This can be done in a separate crate rather than a build script and the result committed to the repository.

Your comments regarding windows-targets vs windows-sys make sense to me. No objections.

Note that in those cases it's typical to use windows-bindgen to generate the bindings. This can be done in a separate crate rather than a build script and the result committed to the repository.

It's notably more work compared to just writing (or copy-pasting) the declarations. Hopefully there is not much that can go wrong in this case.

Fixed by #415

fix(windows): use RtlGenRandom to avoid bcryptprimitives.dll

Navigation Menu

Repository navigation

Windows: Switch to using ProcessPRNG by default. #414

3 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Windows: Switch to using ProcessPRNG by default. #414

Description

Activity

josephlr commented on Apr 30, 2024

ChrisDenton commented on May 1, 2024

josephlr commented on May 1, 2024

josephlr commented on May 1, 2024

ChrisDenton commented on May 1, 2024

newpavlov commented on May 1, 2024

josephlr commented on May 1, 2024

ChrisDenton commented on May 9, 2024

josephlr commented on May 9, 2024

ChrisDenton commented on May 9, 2024

briansmith commented on May 17, 2024

briansmith commented on May 20, 2024

newpavlov commented on May 20, 2024

briansmith commented on May 20, 2024

briansmith commented on May 21, 2024

3 remaining items

josephlr commented on May 21, 2024

newpavlov commented on May 21, 2024

newpavlov commented on May 21, 2024

briansmith commented on May 21, 2024

josephlr commented on May 21, 2024

josephlr commented on May 21, 2024

briansmith commented on May 21, 2024

briansmith commented on May 21, 2024

josephlr commented on May 22, 2024

ChrisDenton commented on May 22, 2024

briansmith commented on May 22, 2024

briansmith commented on May 22, 2024

josephlr commented on May 29, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions