Skip to content

fix(error): return TLS 1.2 PRF setup errors - #14

Merged
thieman merged 1 commit into
mainfrom
thieman/audit-crypto-panics
Jun 24, 2026
Merged

fix(error): return TLS 1.2 PRF setup errors#14
thieman merged 1 commit into
mainfrom
thieman/audit-crypto-panics

Conversation

@thieman

@thieman thieman commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Reduce panic surface in the TLS 1.2 PRF path:

  • Make the cached CNG TLS 1.2 KDF provider load fallible instead of unwrapping during initialization.
  • Return rustls::Error from the fallible Prf::for_key_exchange path if CNG KDF setup or derivation fails.
  • Keep the rustls-mandated infallible Prf::for_secret method, but route it through the same fallible helper and use an explicit invariant expect message rather than opaque CNG unwraps.
  • Add a pure unit test for checked CNG buffer length conversion.

This PR intentionally avoids changing other infallible rustls trait implementations where the API has no Result return. Those sites need separate design/review if we want to preflight or restructure them.

Testing

  • cargo fmt -- --check
  • cargo check --target x86_64-pc-windows-msvc
  • cargo check --target x86_64-pc-windows-msvc --no-default-features
  • cargo check --features fips --target x86_64-pc-windows-msvc (passes with pre-existing kx.rs dead-code warnings)
  • cargo clippy --target x86_64-pc-windows-msvc -- -D warnings
  • git diff --check

Windows CI should run the unit tests. Local macOS cannot compile Windows-target test dependencies because aws-lc-sys needs Windows SDK headers.

@thieman
thieman marked this pull request as ready for review June 23, 2026 19:27
@thieman
thieman requested a review from a team as a code owner June 23, 2026 19:27
Comment thread src/prf.rs
output: &mut [u8; 48],
kx: Box<dyn ActiveKeyExchange>,
peer_pub_key: &[u8],
output: &mut [u8],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure I understand why we're switching to a variable-length slice here... 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[GPT 5.5] This is shared with for_secret, whose rustls trait signature is already &mut [u8]. The for_key_exchange method still receives &mut [u8; 48]; before this PR it immediately called for_secret, which coerced that fixed array to the same slice type. So this does not broaden the key-exchange caller contract, it just avoids duplicating the CNG KDF implementation. I checked the PR diff and did not find another introduced fixed-array-output helper pattern that needs the same treatment.

@thieman
thieman merged commit 766c4f6 into main Jun 24, 2026
10 checks passed
@thieman
thieman deleted the thieman/audit-crypto-panics branch June 24, 2026 19:51
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.

2 participants