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

Add in a RtlGenRandom fallback for non-UWP Windows #337

Merged
merged 3 commits into from
Feb 22, 2023

Conversation

chutten
Copy link
Contributor

@chutten chutten commented Feb 16, 2023

In some instances BCryptRandom will fail when RtlGenRandom will work. On UWP, we might be unable to actually use RtlGenRandom.

Thread the needle and use RtlGenRandom when we have to, when we're able.

See also rust-lang/rust#108060

Fixes #314

@chutten
Copy link
Contributor Author

chutten commented Feb 16, 2023

Builds and tests on my Windows machine, fwiw. Gonna try patching Firefox and see where that leaves us.

@chutten chutten force-pushed the RtlGenRandom branch 2 times, most recently from b3cea33 to ab8c2d5 Compare February 16, 2023 21:38
@chutten
Copy link
Contributor Author

chutten commented Feb 16, 2023

(Whoops. Built and tested it on my Windows machine within WSL which isn't at all Windows. Lemme just tweak and rebuild that real quick.)

Copy link

@yjugl yjugl left a comment

Choose a reason for hiding this comment

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

Thank you @chutten for working on this! Here are my remarks on the Windows aspects of this change.

src/windows.rs Outdated
// system does not have a way to express this yet.
let code = unsafe { NonZeroU32::new_unchecked(code) };
return Err(Error::from(code));
if ret > 0 {
Copy link

Choose a reason for hiding this comment

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

if ret > 0 is not properly checking for failure of BCryptGenRandom here. NTSTATUS is a signed 32-bit value and the two most significant bits encode whether it is a success, information, warning, or error. Checking for success is typically "success or information" (which translates to if ret >= 0) while failure is typically "warning or error" (which translates to if ret < 0).

Be careful that the code before this patch has defined the return value from BCryptGenRandom as u32 instead of i32, which I would say is incorrect (wrt C definitions) or at best confusing (but this is what allowed ret >> 30 to compare with 0b11 instead of -1, I guess). So unless you also change that u32 to i32 (which I would recommend), my if ret < 0 suggestion would actually not work written like that here.

For more details check https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/using-ntstatus-values and/or https://searchfox.org/mozilla-central/source/__GENERATED__/__RUST_STDLIB__/std/src/sys/windows/c.rs#292-296 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, and here I thought I was clever by adapting rust-lang's nt_success... but without thinking about it first. Lemme give this another go, paying more close attention to the types and values (that's what I get for rushing it at the end of my day yesterday)

src/windows.rs Outdated

#[cfg(target_vendor = "uwp")]
#[inline(never)]
fn fallback_rng(_chunk: &mut [MaybeUninit<u8>]) -> u8 {
Copy link

@yjugl yjugl Feb 17, 2023

Choose a reason for hiding this comment

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

The return type does not seem to match the return value here? RtlGenRandom return no more information than TRUE or FALSE (respectively a non-zero or a zero u8 value), so I think fallback_rng could return bool or Maybe<bool> depending on whether or not we want to be able to distinguish "RtlGenRandom is unavailable" from "RtlGenRandom failed". Given that at the moment we do not need to be able to distinguish since the error we propagate will be the one from BCryptGenRandom, maybe bool is enough here.

Note: TRUE in RtlGenRandom means success so its return value is currently incorrectly checked on line 46.

src/windows.rs Outdated
// We zeroize the highest bit, so the error code will reside
// inside the range designated for OS codes.
let code = ret ^ (1 << 31);
// SAFETY: the second highest bit is always equal to one,
Copy link

@yjugl yjugl Feb 17, 2023

Choose a reason for hiding this comment

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

The code before this patch considered that warning values for NTSTATUS are not a failure (that's why it was checking for the two most significant bits of the return value from BCryptGenRandom). This is what this remark explains (although I think it's a mistake to consider that warning values are not a failure).

If in this patch we consider that warning values for NTSTATUS are a failure (and I think we should), then this remark does not hold anymore and should be updated. But I think we can still use NonZeroU32::new_unchecked, because NTSTATUS warning values start with 0x80000001 STATUS_GUARD_PAGE_VIOLATION, so there should be no 0x80000000. Or, for extra safety guarantees, we could map a theoretical 0x80000000 to 0xC0000001 STATUS_UNSUCCESSFUL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a fan of patches doing one thing at a time, so I'd prefer to restrain ourselves to not having opinions about the return values (like I accidentally did). If we wish to propose a change so warnings are failures as well (now that we have a fallback to attempt), I feel like that ought to be its own Issue and PR.

Copy link

Choose a reason for hiding this comment

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

OK, good for me then.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that our handling of error values here is not ideal. We should mimic what NT_SUCCESS(Status) does and only treat values 0 − 0x7FFFFFFF as "succeeding". However, this is better addressed in a followup PR.

src/windows.rs Outdated
if ret > 0 {
// Failed. Try RtlGenRandom as a fallback.
let fallback_ret = fallback_rng(chunk);
// NTSTATUS codes use the two highest bits for severity status.
Copy link

Choose a reason for hiding this comment

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

This comment is outdated / does not match with what the code does.

@chutten
Copy link
Contributor Author

chutten commented Feb 17, 2023

And to complete the see also links, see also: https://bugzilla.mozilla.org/show_bug.cgi?id=1816953

src/windows.rs Outdated
@@ -21,6 +21,12 @@ extern "system" {
) -> u32;
}

extern "system" {
// Forbidden when targetting UWP
Copy link
Member

@josephlr josephlr Feb 18, 2023

Choose a reason for hiding this comment

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

Instead of a comment, we should have a #[cfg(not(target_vendor = "uwp"))] attribute. Something like:

#[link(name = "bcrypt")]
extern "system" {
    fn BCryptGenRandom(
        hAlgorithm: *mut c_void,
        pBuffer: *mut u8,
        cbBuffer: u32,
        dwFlags: u32,
    ) -> i32;
}

#[cfg(not(target_vendor = "uwp"))]
#[link(name = "advapi32")]
extern "system" {
    #[link_name = "SystemFunction036"]
    pub fn RtlGenRandom(RandomBuffer: *mut c_void, RandomBufferLength: u32) -> u8;
}

Note that the buffer is of type PVOID and we need to make sure we link to Advapi32

src/windows.rs Outdated
Comment on lines 44 to 45
// Failed. Try RtlGenRandom as a fallback.
let success = fallback_rng(chunk);
Copy link
Member

Choose a reason for hiding this comment

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

This introduces a larger diff and more indentation than needed. Could we just do:

// NTSTATUS codes use the two highest bits for severity status.
if ret >> 30 == 0b11 {
    // Failed. Try RtlGenRandom as a fallback.
    #[cfg(not(target_vendor = "uwp"))]
    if unsafe { RtlGenRandom(chunk.as_mut_ptr() as *mut c_void, chunk.len() as u32) } != 0 {
        continue;
    }

    // We zeroize the highest bit, so the error code will reside
    // inside the range designated for OS codes.
    let code = ret ^ (1 << 31);
    ...

@chutten chutten requested review from josephlr and removed request for yjugl February 21, 2023 19:30
@chutten
Copy link
Contributor Author

chutten commented Feb 21, 2023

Pushed to run a few tests on moz's tryservers here: https://treeherder.mozilla.org/jobs?repo=try&revision=81ff7509fa334c3bf3d2fe0fcce8ca9f2376a6a7

(I expect some oranges here and there from flaky tests, but for it to overall work out).

Attn @yjugl for if you want to doublecheck that this more-final version fixes your particular testcase.

@newpavlov
Copy link
Member

@chutten
You need to wrap the if in a block to pass our MSRV tests.

@chutten
Copy link
Contributor Author

chutten commented Feb 22, 2023

@chutten You need to wrap the if in a block to pass our MSRV tests.

Oh, because of the cfg? Will do.

In some instances BCryptRandom will fail when RtlGenRandom will work.
On UWP, we might be unable to actually use RtlGenRandom.

Thread the needle and use RtlGenRandom when we have to, when we're able.

See also rust-lang/rust#108060

Fixes rust-random#314
@chutten
Copy link
Contributor Author

chutten commented Feb 22, 2023

There, that ought to do it. I pulled down 1.36 and it compiles with the extra block. (For anyone reading, I did it via rustup install 1.36.0 then rm Cargo.lock (because 1.36 can't parse the modern lockfile) then cargo +1.36.0 build)

src/windows.rs Outdated Show resolved Hide resolved
Co-authored-by: Artyom Pavlov <newpavlov@gmail.com>
src/windows.rs Outdated Show resolved Hide resolved
@yjugl
Copy link

yjugl commented Feb 23, 2023

Attn @yjugl for if you want to doublecheck that this more-final version fixes your particular testcase.

I was able to confirm that on a Windows 7 machine where bcryptprimitives.dll is unavailable, Uuid::new_v4() no longer panics and successfully generates a random uuid. Thank you!

@chutten
Copy link
Contributor Author

chutten commented Feb 23, 2023

@newpavlov @josephlr sooooo... when would it be polite to ask about your plans vis a vis cutting a v0.2.9...? : D

@notgull
Copy link

notgull commented Feb 25, 2023

Bit late to the party, but you probably shouldn't be matching on target_vendor. See rust-lang/rust#100343

@chutten
Copy link
Contributor Author

chutten commented Feb 27, 2023

Bit late to the party, but you probably shouldn't be matching on target_vendor. See rust-lang/rust#100343

TIL. The specific proposed guidance for UWP will be to replace target_vendor = "uwp" with target_abi = "uwp". Plenty of time for that yet, by the looks of things. e.g. the code we're adapting still uses target_vendor for now, at least.

@newpavlov newpavlov mentioned this pull request Apr 2, 2023
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.

Windows: should we fall back to RtlGenRandom if BCryptGenRandom fails
5 participants