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

Use ProcessPrng on Windows #415

Merged
merged 6 commits into from
May 29, 2024
Merged

Use ProcessPrng on Windows #415

merged 6 commits into from
May 29, 2024

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Apr 30, 2024

Use ProcessPrng on Windows 10 and up, and use RtlGenRandom on older legacy Windows versions. Don't use BCryptGenRandom due to stability issues.

@josephlr josephlr force-pushed the windows branch 2 times, most recently from 5495418 to 5223c3e Compare April 30, 2024 11:05
@josephlr josephlr force-pushed the windows branch 7 times, most recently from 1d62b41 to 03abb05 Compare May 1, 2024 03:57
Copy link
Contributor

@briansmith briansmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the lines I commented on, I didn't review the rest.

src/windows.rs Outdated Show resolved Hide resolved
src/windows.rs Outdated Show resolved Hide resolved
src/windows.rs Outdated Show resolved Hide resolved
@josephlr
Copy link
Member Author

josephlr commented May 21, 2024

@briansmith the above are really good point w.r.t. sandboxing. I think that it would be good to have general documentation along the lines of "before starting a sandbox, you should first successfully call getrandom() on a non-empty buffer". That should be good platform-agnostic advice, and will handle things like LoadLibrary and libc::dlsym.

More generally, this won't work inside many sandboxes, including Chromium's.

I think that this won't cause issues in some sandboxes provided that ProcessPRNG is already loaded. IIRC the sandbox only complains on loading a new dll, not upon looking up a symbol from an already loaded DLL. Regardless, having specific documentation will be good here.

@josephlr josephlr force-pushed the windows branch 7 times, most recently from 6825ae5 to 4bfd348 Compare May 21, 2024 10:47
.github/workflows/tests.yml Show resolved Hide resolved
src/windows.rs Outdated Show resolved Hide resolved
src/windows.rs Outdated Show resolved Hide resolved
src/windows.rs Show resolved Hide resolved
src/windows7.rs Outdated Show resolved Hide resolved
@josephlr
Copy link
Member Author

@newpavlov and @briansmith this is now ready for review!

@josephlr josephlr marked this pull request as ready for review May 26, 2024 04:44
@josephlr josephlr changed the title WIP: Use ProcessPrng on Windows Use ProcessPrng on Windows May 26, 2024
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the two minor nits, it looks good to me. But as I noted in the issue, I would prefer if we released this change as v0.3.

src/windows7.rs Outdated Show resolved Hide resolved
src/windows.rs Show resolved Hide resolved
src/windows.rs Show resolved Hide resolved
src/windows.rs Outdated Show resolved Hide resolved
src/windows.rs Outdated
#[repr(transparent)]
#[derive(PartialEq, Eq)]
pub struct BOOL(pub i32);
pub const TRUE: BOOL = BOOL(1i32);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate to have this ugliness without the program that generated it to confirm that it actually created this ugliness. However, I also think it's fine because of how repr(transparent) works so it isn't the end of the world.

Copy link
Member Author

@josephlr josephlr May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good point. For both APIs I didn't directly use the output of windows-bindgen, but instead took that output as a starting point, then deleted lines until I got code that looked decent.

I think that referencing windows-bindgen in the comments at all was misleading, so I changed the comments to just reference the API metadata names directly. Given that, I also changed the BOOL/BOOLEAN types to just be typedefs (which is more honest IMO).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, here's the full output for running windows-bindgen with the following arguments:

// Bindings generated by `windows-bindgen` 0.56.0

#![allow(
    non_snake_case,
    non_upper_case_globals,
    non_camel_case_types,
    dead_code,
    clippy::all
)]
#[inline]
pub unsafe fn RtlGenRandom(
    randombuffer: *mut core::ffi::c_void,
    randombufferlength: u32,
) -> BOOLEAN {
    windows_targets::link!("advapi32.dll" "system" "SystemFunction036" fn RtlGenRandom(randombuffer : *mut core::ffi::c_void, randombufferlength : u32) -> BOOLEAN);
    RtlGenRandom(randombuffer, randombufferlength)
}
#[inline]
pub unsafe fn ProcessPrng(pbdata: &mut [u8]) -> BOOL {
    windows_targets::link!("bcryptprimitives.dll" "system" fn ProcessPrng(pbdata : *mut u8, cbdata : usize) -> BOOL);
    ProcessPrng(
        core::mem::transmute(pbdata.as_ptr()),
        pbdata.len().try_into().unwrap(),
    )
}
#[repr(transparent)]
#[derive(PartialEq, Eq)]
pub struct BOOL(pub i32);
impl Default for BOOL {
    fn default() -> Self {
        unsafe { core::mem::zeroed() }
    }
}
impl Clone for BOOL {
    fn clone(&self) -> Self {
        *self
    }
}
impl Copy for BOOL {}
impl core::fmt::Debug for BOOL {
    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
        f.debug_tuple("BOOL").field(&self.0).finish()
    }
}
impl windows_core::TypeKind for BOOL {
    type TypeKind = windows_core::CopyType;
}
#[repr(transparent)]
#[derive(PartialEq, Eq)]
pub struct BOOLEAN(pub u8);
impl Default for BOOLEAN {
    fn default() -> Self {
        unsafe { core::mem::zeroed() }
    }
}
impl Clone for BOOLEAN {
    fn clone(&self) -> Self {
        *self
    }
}
impl Copy for BOOLEAN {}
impl core::fmt::Debug for BOOLEAN {
    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
        f.debug_tuple("BOOLEAN").field(&self.0).finish()
    }
}
impl windows_core::TypeKind for BOOLEAN {
    type TypeKind = windows_core::CopyType;
}
pub const TRUE: BOOL = BOOL(1i32);

src/windows7.rs Outdated Show resolved Hide resolved
src/windows7.rs Outdated Show resolved Hide resolved
src/windows7.rs Outdated Show resolved Hide resolved
@josephlr josephlr merged commit 875081b into rust-random:master May 29, 2024
52 checks passed
@josephlr josephlr deleted the windows branch May 29, 2024 04:19
@newpavlov newpavlov mentioned this pull request Oct 11, 2024
newpavlov added a commit that referenced this pull request Oct 11, 2024
Tweak the breaking changes section and add entries for #415, #440, #442,
#448, #504, and #512.
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

Successfully merging this pull request may close these issues.

4 participants