Soundness: Construction of invalid enum discriminant in cmp::Ordering::conditional_selec

[Manishearth]

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 subtle v2, Choice::from(u8) is a safe public constructor. The crate documentation explicitly states that internal debug_assert! invariant checks are stripped in release profile to avoid introducing data-dependent branches. Consequently, in release builds (or when debug assertions are disabled), safe downstream code can construct a Choice holding an arbitrary u8 value such as 2u8, violating the internal invariant self.0 \in {0, 1}.

When such an invalid Choice is passed into ConditionallySelectable::conditional_select for core::cmp::Ordering:

subtle/src/lib.rs

Lines 556 to 566 in 5457b54

	impl ConditionallySelectable for cmp::Ordering {
	fn conditional_select(a: &Self, b: &Self, choice: Choice) -> Self {
	let a = *a as i8;
	let b = *b as i8;
	let ret = i8::conditional_select(&a, &b, choice);
	// SAFETY: `Ordering` is `#[repr(i8)]` and `ret` has been assigned to
	// a value which was originally a valid `Ordering` then cast to `i8`
	unsafe { *((&ret as *const _) as *const cmp::Ordering) }
	}
	}

Integer bitwise selection arithmetic computes an out-of-bounds i8 value (ret = -2i8). Dereferencing a raw pointer to -2 as *const cmp::Ordering constructs an invalid enum discriminant, triggering immediate Undefined Behavior under the Rust Reference.

Minimal Reproduction (Miri)

use std::cmp::Ordering;
use subtle::{Choice, ConditionallySelectable};

fn main() {
    let invalid_choice = Choice::from(2u8);
    let _ub = Ordering::conditional_select(&Ordering::Equal, &Ordering::Less, invalid_choice);
}

error: Undefined Behavior: constructing invalid value of type std::cmp::Ordering: at .<enum-tag>, encountered 0xfe, but expected a valid enum tag
   --> /google/src/cloud/manishearth/verify-unsafe-rust-bugs/google3/third_party/rust/subtle/v2/src/lib.rs:564:18
    |
564 |         unsafe { *((&ret as *const _) as *const cmp::Ordering) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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
    = note: stack backtrace:
            0: <std::cmp::Ordering as subtle::ConditionallySelectable>::conditional_select
                at /google/src/cloud/manishearth/verify-unsafe-rust-bugs/google3/third_party/rust/subtle/v2/src/lib.rs:564:18: 564:63
            1: main
                at src/bin/repro1.rs:6:15: 6:94

Suggested Fix

Choice::from must structurally enforce its internal invariant (self.0 \in {0, 1}) across safe code boundaries without branching, for example by masking the input bitwise:

 impl From<u8> for Choice {
     #[inline]
     fn from(input: u8) -> Choice {
         debug_assert!((input == 0u8) | (input == 1u8));
-        Choice(input)
+        Choice(black_box(input & 1))
     }
 }

Alternatively, cmp::Ordering::conditional_select should avoid raw pointer casts and safely map the resulting integer back to Ordering.

---

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.

Full Gemini Unsafe Code Audit Report

Unsafe Rust Review: subtle (v2)

Overall Safety Assessment

subtle is a low-level cryptographic utility library providing pure-Rust traits and wrapper types (Choice, CtOption, ConstantTimeEq, ConditionallySelectable, etc.) designed to execute in constant time to prevent timing side-channel attacks.

The crate has a very low density of unsafe code. Across the core library (src/lib.rs), there are only two unsafe blocks:

1. src/lib.rs:225 (black_box): Uses core::ptr::read_volatile(&input) as
   an optimization barrier to prevent the compiler from recognizing bitwise masking patterns and optimizing them back into conditional branches.

2. src/lib.rs:564 (cmp::Ordering::conditional_select): Dereferences a raw
   pointer cast *const cmp::Ordering from an i8 stack variable ret.

While the architecture is minimalist and primarily relies on safe bitwise operations, there is a critical soundness vulnerability at the boundary between Choice::from(u8) and cmp::Ordering::conditional_select. The crate intentionally strips internal debug_assert! invariant checks in release profile to avoid introducing data-dependent branches. Consequently, Choice::from(u8) allows safe downstream code to construct a Choice holding an arbitrary u8 value (violating the internal invariant self.0 \in {0, 1}). When such an invalid Choice is passed into cmp::Ordering::conditional_select, integer arithmetic produces an out-of-bounds discriminant (e.g., -2i8). Dereferencing a raw pointer to this value in the unsafe block constructs an invalid enum discriminant, triggering immediate Undefined Behavior.

Critical Findings

Soundness Vulnerability: Invalid Enum Discriminant Construction in cmp::Ordering::conditional_select 🔴 🚨

• Severity: 🔴 High

• Threat Vector: 🚨 Untrusted Input

• Bug Type: Validity Invariant Violation, Invalid Enum Discriminant

• Location: src/lib.rs:556-566 (impl ConditionallySelectable for cmp::Ordering), specifically line 564.

• Description: The trait method
  ConditionallySelectable::conditional_select is a safe function. For cmp::Ordering, it converts references a and b to i8, performs i8::conditional_select(&a, &b, choice), and then transmutes the resulting i8 (ret) back to cmp::Ordering via pointer dereference:

  unsafe { *((&ret as *const _) as *const cmp::Ordering) }

The validity of dereferencing *const cmp::Ordering requires that ret holds a valid discriminant bit pattern for core::cmp::Ordering (-1, 0, or 1). However, Choice::from(input: u8) (src/lib.rs:236-245) is a safe public constructor. The crate documentation explicitly states that debug_assert! checks are stripped in release mode to avoid branching. In release mode, Choice::from(2u8) safely constructs a Choice with internal representation 2u8. When choice = Choice::from(2u8) is passed to Ordering::conditional_select(&Ordering::Equal, &Ordering::Less, choice):

-   Inside `i8::conditional_select`: `mask = -(2i8) = -2i8` (`0xFE`).
-   `a = 0` (`Equal`), `b = -1` (`Less`).
-   `ret = a ^ (mask & (a ^ b)) = 0 ^ (-2 & -1) = -2` (`0xFE`).
-   The `unsafe` block dereferences a raw pointer to `-2` as `*const

cmp::Ordering`. According to the Rust Reference, producing an integer discriminant that violates the valid range of discriminants for an enum type is immediate Undefined Behavior.

• Proof of Concept: In safe Rust (compiled in release profile or with
  debug assertions disabled):

  use std::cmp::Ordering;
  use subtle::{Choice, ConditionallySelectable};
  
  let invalid_choice = Choice::from(2u8);
  // Immediate Undefined Behavior (invalid enum discriminant -2 constructed)
  let _ub = Ordering::conditional_select(&Ordering::Equal, &Ordering::Less, invalid_choice);

• Remediation: Choice must structurally enforce its internal invariant
  (self.0 \in {0, 1}) across safe code boundaries without branching, e.g., by masking the input:

  impl From<u8> for Choice {
      #[inline]
      fn from(input: u8) -> Choice {
          debug_assert!((input == 0u8) | (input == 1u8));
          Choice(black_box(input & 1))
      }
  }

Alternatively, cmp::Ordering::conditional_select should avoid raw pointer casts and instead safely convert the resulting integer back to Ordering.

Fishy Findings

1. Invalid Claim in Safety Comment for cmp::Ordering::conditional_select 🟡 🤦

• Severity: 🟡 Low
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Incorrect Safety Documentation
• Location: src/lib.rs:562-564
• Description: The safety comment states: ret has been assigned to a value which was originally a valid Ordering then cast to i8. This claim is factually incorrect. ret is the computed output of i8::conditional_select(&a, &b, choice), which performs bitwise XOR, AND, and negation operations. While a and b were originally valid Ordering values cast to i8, ret itself is a newly computed bit pattern. The safety proof relies on unstated lemmas: (1) that choice.0 \in {0, 1}, and (2) that integer conditional selection over the domain {-1, 0, 1} is closed under valid Ordering discriminants. As demonstrated in the Critical Findings, lemma (1) does not hold for safe public callers in release profile.

2. Stability of read_volatile as an Optimization Barrier 🟡 🤸

• Severity: 🟡 Low
• Threat Vector: 🤸 Deliberate Contortion
• Bug Type: Fragile Optimization Barrier
• Location: src/lib.rs:222-234 (black_box)
• Description: The fallback implementation of black_box uses
  core::ptr::read_volatile(&input) on a local stack variable to prevent compiler optimizations. While this is memory safe and sound, the authors correctly note in comments that relying on volatile reads of non-escaping stack variables as a cryptographic optimization barrier is fragile against aggressive compiler memory models. (In modern Rust versions with the core_hint_black_box feature, core::hint::black_box is used instead).

Missing Safety Comments

The core crate (src/lib.rs) contains safety comments on all unsafe blocks. However, three unsafe blocks in the fuzzer test suite lack // SAFETY: comments:

1. fuzz/fuzzers/conditional_assign_i8.rs:8 🟡 🤦

• Severity: 🟡 Low

• Threat Vector: 🤦 Accidental Misuse

• Bug Type: Missing Safety Comment

• Proposed Proof Comment:

  // SAFETY: `transmute::<u8, i8>` transmutes between primitive integer types
  // of identical size (1 byte) and alignment (1 byte), where all bit patterns
  // are valid initialized values.

2. fuzz/fuzzers/conditional_assign_u16.rs:14 🟡 🤦

• Severity: 🟡 Low

• Threat Vector: 🤦 Accidental Misuse

• Bug Type: Missing Safety Comment

• Proposed Proof Comment:

  // SAFETY: `transmute::<[u8; 2], u16>` transmutes a 2-byte array into a `u16`.
  // Both types have identical size (2 bytes) and all bit patterns are valid for `u16`.
  // Alignment requirements are satisfied because `transmute` takes the array by value.

3. fuzz/fuzzers/conditional_assign_i128.rs:14 🟡 🤦

• Severity: 🟡 Low

• Threat Vector: 🤦 Accidental Misuse

• Bug Type: Missing Safety Comment

• Proposed Proof Comment:

  // SAFETY: `transmute::<[u8; 16], i128>` transmutes a 16-byte array into an `i128`.
  // Both types have identical size (16 bytes) and all bit patterns are valid for `i128`.
  // Alignment requirements are satisfied because `transmute` takes the array by value.

[tarcieri]

It seems like the core issue here isn't actually in the ConditionallySelectable impl for cmp::Ordering, but in i8::conditional_select selecting a completely different value from either of its inputs in the case the provided choice isn't in {0,1} (I say as the author of that SAFETY comment on the cmp::Ordering impl).

subtle's strategy is to deliberately avoid giving the compiler information that would let it infer that the inner u8 of a Choice always falls in {0,1} in release builds, which is why the check is only in debug builds. But if it does that, IMO all of its impls need to treat all non-zero inputs as the truthy value.

(FWIW, ctutils has a very similar impl on cmp::Ordering, but I hope a similar problem doesn't exist there specifically because its predication is built on CMOV-like instructions as wrapped in the cmov crate that differentiate between zero and non-zero cases exactly for this reason and should handle all of the non-zero cases identically)