Skip to content

Rewrite zeroization tests so they don't read freed memory - #1141

Open
benr-ml wants to merge 1 commit into
mainfrom
fix/zeroization-tests-ub
Open

benr-ml wants to merge 1 commit into
mainfrom
fix/zeroization-tests-ub

Conversation

@benr-ml

@benr-ml benr-ml commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Problem

The test_sk_zeroization_on_drop tests (and test_zeroization_on_drop in tbls) take raw pointers into a key inside a block, let the key drop at the end of the block, and then dereference the pointers. Reading through a pointer after its target goes out of scope is undefined behaviour. The tests pass only because the stack slot happens not to be reused, and Miri or AddressSanitizer (use-after-scope) would flag them. This blocks adding those tools to CI.

Fix

  • New test helpers in fastcrypto/src/tests/test_helpers.rs:
    • bytes_after_drop(value, ranges) moves the value into a ManuallyDrop, runs its destructor in place with ManuallyDrop::drop, and returns the requested (offset, len) byte ranges. The storage is never freed, so the reads are well defined. It also asserts that every range lies inside the value.
    • field_offset(base, field) computes those offsets while the value is still alive.
  • Rewrote the eight affected tests to use them, keeping the same assertions:
    • ed25519
    • secp256k1 and secp256k1 recoverable
    • secp256r1 and secp256r1 recoverable
    • bls12381
    • aes
    • tbls ecies_v1, which inlines the same ManuallyDrop pattern because it is a separate crate

Testing

  • cargo test -p fastcrypto -p fastcrypto-tbls zeroization passes, with and without --features copy_key.
  • cargo xclippy -D warnings is clean.
  • Not yet run under Miri; most of these keys wrap C libraries, which Miri cannot execute.

🤖 Generated with Claude Code

The zeroization-on-drop tests kept raw pointers into a key and
dereferenced them after the key went out of scope. That is undefined
behaviour (use after scope end), so the tests only passed by accident and
would be flagged by Miri or AddressSanitizer.

Add a test helper, `bytes_after_drop`, that moves the value into a
`ManuallyDrop`, runs its destructor in place and reads back the requested
byte ranges. The storage stays allocated, so the reads are well defined.
`field_offset` computes the ranges while the value is still alive.

The eight affected tests (ed25519, secp256k1, secp256k1 recoverable,
secp256r1, secp256r1 recoverable, bls12381, aes, tbls ecies_v1) keep the
same assertions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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.

1 participant