Absolucy / Absolucy/nanorand-rs

Incorrect return type in Windows RtlGenRandom binding

未关闭
#56 0 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看
主要语言
Rust
星标
251
派生
24
PR 合并指标
30 天内没有已合并 PR

描述

> [!NOTE]
> This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification.

## The Issue

In `src/entropy/windows.rs`, the Win32 API `RtlGenRandom` (exported as `SystemFunction036` from `advapi32.dll`) is declared returning `u32`:

https://github.com/Absolucy/nanorand-rs/blob/f7c67967cb78dde9629f78e4d8ece384089c2446/src/entropy/windows.rs#L1-L4

In the [official Windows SDK definitions (`ntsecapi.h`)](https://learn.microsoft.com/en-us/windows/win32/api/ntsecapi/nf-ntsecapi-rtlgenrandom), `RtlGenRandom` returns `BOOLEAN` (`unsigned char`, 1 byte).

Under the Microsoft x64 ABI calling convention, callee functions returning data types smaller than 64 bits only populate the required low bits of the return register (`AL` for `BOOLEAN`). The upper 24 bits of `EAX` (and upper 32 bits of `RAX`) contain uninitialized register garbage left over from callee execution.

Declaring the return type as `u32` in Rust forces the compiler to read the entire 32-bit `EAX` register at the foreign function boundary. Reading uninitialized register bits from an ABI boundary is UB in Rust. Furthermore, in `windows::entropy`, the return value is checked using `== 0`. In Windows conventions, `BOOLEAN` functions return `TRUE` (`1` / non-zero) on success and `FALSE` (`0`) on failure. By checking `== 0`, `windows::entropy` reports `false` when entropy generation succeeds and `true` when it fails.

Minimal Reproduction (Miri)

`cargo +nightly miri run --target x86_64-pc-windows-gnu`

```rust
use nanorand::WyRand;

fn main() {
// Calling `WyRand::new()` invokes `crate::entropy::system(&mut entropy)`,
// which on Windows targets calls WinAPI `RtlGenRandom(ptr, len) -> u32`.
// In authoritative Windows SDK headers (`ntsecapi.h`), `RtlGenRandom` returns `BOOLEAN` (`u8`).
// Under the Microsoft x64 calling convention, reading a 32-bit integer (`u32`) from
// a callee returning a 1-byte `BOOLEAN` reads uninitialized register garbage
// in the upper 24 bits of EAX, causing Undefined Behavior at the FFI boundary.
let _rng = WyRand::new();
}
```

```text
error: Undefined Behavior: calling a function with return type u8 passing return place of type u32
--> /usr/local/google/home/manishearth/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/nanorand-0.7.0/src/entropy/windows.rs:8:11
|
8 | unsafe { RtlGenRandom(out.as_mut_ptr(), out.len()) == 0 }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= help: this means these two types are not *guaranteed* to be ABI-compatible across all targets
= help: if you think this code should be accepted anyway, please report an issue with Miri
= note: stack backtrace:
0: nanorand::entropy::entropy
at /usr/local/google/home/manishearth/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/nanorand-0.7.0/src/entropy/windows.rs:8:11: 8:52
1: ::default
at /usr/local/google/home/manishearth/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/nanorand-0.7.0/src/rand/wyrand.rs:35:3: 35:39
2: nanorand::WyRand::new
at /usr/local/google/home/manishearth/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/nanorand-0.7.0/src/rand/wyrand.rs:21:3: 21:18
3: main
at src/bin/repro1.rs:10:16: 10:29

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error
```

Suggested Fix

Correct the FFI return type declaration in `src/entropy/windows.rs` to `u8` (or `std::ffi::c_uchar`) and adjust `cbBuffer` to `u32` (`ULONG`), updating the return check accordingly:

```diff
extern "system" {
#[link_name = "SystemFunction036"]
- fn RtlGenRandom(pBuffer: *mut u8, cbBuffer: usize) -> u32;
+ fn RtlGenRandom(pBuffer: *mut u8, cbBuffer: u32) -> u8;
}

/// Obtain a random 64-bit number using WinAPI's `RtlGenRandom` function.
pub fn entropy(out: &mut [u8]) -> bool {
- unsafe { RtlGenRandom(out.as_mut_ptr(), out.len()) == 0 }
+ unsafe { RtlGenRandom(out.as_mut_ptr(), out.len() as u32) != 0 }
}
```

--------------------------------------------------------------------------------

> [!NOTE]
> The full audit report below also contains additional minor findings (such as missing safety comments or undocumented FFI assumptions) that are probably worth fixing as well but not the primary goal of this issue. The audit report has not been human-reviewed, it may contain misleading claims.

Full Gemini Codebase Audit Report Appendix

# Unsafe Rust Review: `nanorand` (`v0_7`)

## Overall Safety Assessment
`nanorand` (`v0_7`) is a minimal, zero-dependency random number generation library providing pseudo-random number generators (`WyRand`, `Pcg64`, `ChaCha`) alongside platform-specific OS entropy sources.

The crate maintains a relatively low density of unsafe code. The core RNG implementations (`WyRand`, `Pcg64`, `ChaCha`) and buffering wrappers (`BufferedRng`) are written entirely in safe Rust. The `unsafe` footprint is strictly contained within system entropy gathering (`src/entropy.rs` and the `src/entropy/` platform submodules), comprising exactly five `unsafe` blocks/expressions across the codebase:

1. `src/entropy.rs`: Invoking x86/x86_64 hardware intrinsic `_rdseed64_step`.
2. `src/entropy/darwin.rs`: Invoking Apple Security FFI `SecRandomCopyBytes`.
3. `src/entropy/linux.rs`: Invoking libc FFI `getrandom`.
4. `src/entropy/windows.rs`: Invoking Win32 FFI `RtlGenRandom` (`SystemFunction036`).
5. `src/entropy/windows_uwp.rs`: Invoking Win32 FFI `BCryptGenRandom`.

While encapsulating `unsafe` code behind safe APIs is architecturally sound, the audit reveals an unsound ABI return type declaration in Windows FFI bindings (a Critical finding), several dubious FFI assumptions and logic bugs in safe code (Fishy findings), and a complete absence of safety comments across all five `unsafe` call sites.

## Critical Findings

### 1. FFI Return Type Unsoundness in Windows `RtlGenRandom` Binding 🔴 ⚠️

- **Priority**: 🔴 High
- **Threat Vector**: ⚠️ Accidental Misuse

- **Bug Type**: `Unsound ABI Return Type` In `src/entropy/windows.rs:1-4`, the Win32 API `RtlGenRandom` (exported as `SystemFunction036` from `advapi32.dll`) is declared with an incorrect return type:

```rust

extern "system" {
#[link_name = "SystemFunction036"]
fn RtlGenRandom(pBuffer: *mut u8, cbBuffer: usize) -> u32;
}

```

In the official Windows SDK definitions (`ntsecapi.h`), `RtlGenRandom` returns `BOOLEAN` (`unsigned char`, 1 byte). Under the Microsoft x64 ABI calling convention, callee functions returning data types smaller than 64 bits only populate the required low bits of the return register (`AL` for `BOOLEAN`). The upper 24 bits of `EAX` (and upper 32 bits of `RAX`) contain uninitialized register garbage left over from callee execution.

Declaring the return type as `u32` in Rust forces the compiler to read the entire 32-bit `EAX` register at the foreign function boundary. Reading uninitialized register bits from an ABI boundary violates Rust FFI safety contracts and constitutes Undefined Behavior.

## Fishy Findings

### 1. Inverted FFI Success Check and Ignored Return Values in Entropy Seeding 🟠 ⚠️

- **Priority**: 🟠 Medium
- **Threat Vector**: ⚠️ Accidental Misuse

- **Bug Type**: `Improper Error Handling` In `src/entropy/windows.rs:8`, the wrapper checks the return value of `RtlGenRandom` using `== 0`:

```rust

pub fn entropy(out: &mut [u8]) -> bool {
unsafe { RtlGenRandom(out.as_mut_ptr(), out.len()) == 0 }
}

```

In Windows conventions, `BOOLEAN` functions return `TRUE` (`1` / non-zero) on success and `FALSE` (`0`) on failure. By checking `== 0`, `windows::entropy` reports `false` when entropy generation succeeds and `true` when it fails.

Furthermore, `WyRand::default()` in `src/rand/wyrand.rs:35` calls `crate::entropy::system(&mut entropy)` and completely discards its boolean return value. When `RtlGenRandom` succeeds, `WyRand::default()` happens to function correctly because the buffer is mutated in place. However, if system entropy sourcing fails on any platform (e.g., `getrandom` or `RtlGenRandom` returning failure), the error is ignored, leaving `entropy` as `[0u8; 8]` and initializing the RNG with a static seed of `0`.

### 2. ABI Argument Mismatches (`usize` vs `ULONG` in Windows FFI) 🟡 🧪

- **Priority**: 🟡 Low
- **Threat Vector**: 🧪 Contrived Setup

- **Bug Type**: `ABI Argument Mismatch` In `src/entropy/windows.rs` and `src/entropy/windows_uwp.rs`, the buffer length parameter (`cbBuffer`) for `RtlGenRandom` and `BCryptGenRandom` is declared as `usize`. The Windows SDK specifies `ULONG` (32-bit unsigned integer `u32`). On 64-bit Windows targets, `usize` is 64 bits (`u64`). Passing a 64-bit integer to an ABI expecting a 32-bit integer violates x64 calling convention argument typing in `RDX`/`R8`, which risks truncation or misinterpretation if slice lengths exceed `u32::MAX`.

### 3. Deterministic Out-of-Bounds Panic in `rdseed` Entropy Generator 🟠 ⚠️

- **Priority**: 🟠 Medium
- **Threat Vector**: ⚠️ Accidental Misuse

- **Bug Type**: `Out-of-Bounds Panic` In `src/entropy.rs:83-94`, `rdseed(out: &mut [u8])` calculates `rdseed_amt` as `div_ceil(out.len(), 8)`. For each iteration, it generates an 8-byte `u64` seed and unconditionally writes all 8 bytes into the output slice:

```rust

x.iter()
.enumerate()
.for_each(|(i, val)| out[(core::mem::size_of::() * n) + i] = *val);

```

If `out.len()` is not an exact multiple of 8 (e.g., `out.len() == 5`), indexing `out[5]` deterministically triggers a runtime slice bounds check panic. While safe Rust bounds checking prevents memory corruption, this logic flaw causes unexpected crashes for non-8-aligned buffer lengths.

### 4. Broken Compilation on 32-bit x86 Targets with `rdseed` Feature 🟡 ⚠️

- **Priority**: 🟡 Low
- **Threat Vector**: ⚠️ Accidental Misuse

- **Bug Type**: `Broken Compilation` In `src/entropy.rs:57`, when `target_arch = "x86"` (32-bit x86) and feature `rdseed` is enabled, the crate imports `core::arch::x86::_rdseed64_step`. However, 64-bit intrinsic registers are restricted to `x86_64` architectures. Attempting to compile `nanorand` for `i686` with `rdseed` enabled results in a compiler resolution failure.

## Missing Safety Comments

### 1. `src/entropy.rs:63`: Missing `// SAFETY:` comment before calling target-feature CPU intrinsic `rdseed`. 🔴

Proposed proof comment:

```rust

// SAFETY:
// - CPU runtime feature support: `rdseed` requires the `rdseed` target feature. `stupid_rdseed_hack` is a private helper whose only reachable call path in this crate is `rdseed(out: &mut [u8])`, which explicitly verifies `std::is_x86_feature_detected!("rdseed")` prior to execution.
// - Pointer validity: `&mut x` is a valid, properly aligned pointer to a stack-allocated `u64` local variable valid for writes of `size_of::()` bytes.

```

### 2. `src/entropy/darwin.rs:10`: Missing `// SAFETY:` comment before calling FFI `SecRandomCopyBytes`. 🔴

Proposed proof comment:

```rust

// SAFETY:
// - `rnd`: Passing `core::ptr::null()` (`kSecRandomDefault`) is documented by Apple Security framework as the valid constant to use the system default random number generator.
// - `bytes` and `count`: `out` is a valid mutable slice `&mut [u8]` representing `out.len()` contiguous bytes. `out.as_mut_ptr()` is valid for writes of `out.len()` bytes. If `out.len() == 0`, `SecRandomCopyBytes` performs no memory accesses and returns `errSecSuccess` (0).

```

### 3. `src/entropy/linux.rs:7`: Missing `// SAFETY:` comment before calling libc FFI `getrandom`. 🔴

Proposed proof comment:

```rust

// SAFETY:
// - `buf` and `buflen`: `out` is a valid mutable slice `&mut [u8]`, providing a valid pointer `out.as_mut_ptr()` valid for writes of up to `out.len()` contiguous bytes.
// - `flags`: `0x0001` (`GRND_RANDOM`) is a valid bitmask flag defined by Linux `getrandom(2)`.
// - The kernel guarantees it will not write beyond `buflen` (`out.len()`) bytes into `buf`.

```

### 4. `src/entropy/windows.rs:8`: Missing `// SAFETY:` comment before calling Win32 FFI `RtlGenRandom`. 🔴

Proposed proof comment:

```rust

// SAFETY:
// - `pBuffer` and `cbBuffer`: `out` is a valid mutable slice `&mut [u8]`, providing `out.as_mut_ptr()` valid for writes of `out.len()` bytes.
// - Assuming the FFI declaration is corrected to return `BOOLEAN` (`u8`) and `cbBuffer` as `u32`, `out.as_mut_ptr()` is properly aligned for `u8` and the length passed does not exceed allocated slice bounds.

```

### 5. `src/entropy/windows_uwp.rs:18`: Missing `// SAFETY:` comment before calling Win32 FFI `BCryptGenRandom`. 🔴

Proposed proof comment:

```rust

// SAFETY:
// - `hAlgorithm`: Passing `ptr::null_mut()` is documented by Win32 CNG API as valid when `BCRYPT_USE_SYSTEM_PREFERRED_RNG` is specified in flags.
// - `pBuffer` and `cbBuffer`: `out` is a valid mutable slice `&mut [u8]`, providing `out.as_mut_ptr()` valid for writes of `out.len()` bytes.
// - `dwFlags`: `BCRYPT_USE_SYSTEM_PREFERRED_RNG` (`0x00000002`) is a valid flag constant.

```

贡献指南

这个仓库没有索引到贡献指南

评估

这个 Issue 还没有评估数据。

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。