Independent Security Review: NSM RNG & Canonical CBOR Changes

Scope

NSM ioctl ABI, parsing, wiping; Linux 4.14.256 RNG reseed ordering; direct NSM entropy supply; fail-closed errors; CBOR deterministic encoding; authenticated record preservation.

Finding 1 (Medium): canonical_record claims canonical CBOR but uses serde_cbor without deterministic encoding guarantees

File: crates/enclave/src/v2_proxy.rs, function canonical_record Trigger: Every record sealing operation

fn canonical_record(record: &serde_json::Value) -> Result<Vec<u8>> {
    Ok(serde_cbor::to_vec(&serde_cbor::value::to_value(record)?)?)
}

Issue: The serde_cbor crate does not enforce RFC 8949 §4.2 deterministic encoding. Specifically, it does not guarantee:

• Shortest-form integer encoding (serde_cbor does this in practice for positive integers, but relies on serde's type hints; integers flowing through serde_json::Value → serde_cbor::Value conversion may not preserve the smallest-width hint).
• Definite-length encoding for all strings/arrays (the default serializer uses definite-length, but this is not contractually guaranteed by the crate).

The accompanying test record_encoding_uses_canonical_cbor_key_order only validates map key ordering:

assert_eq!(hex::encode(encoded), "a261620262616101");

It does not test that, for example, integer 1 is always 0x01 and never 0x1801 (17-bit form), or that small strings are never encoded in indefinite-length form. Since the CBOR plaintext is the AEAD-authenticated payload in encrypt_record, non-deterministic plaintext encoding means the same logical record encrypted twice could produce different authenticated plaintext bytes. While the AEAD tag authenticates whatever bytes were produced, offline tools that attempt to re-derive the canonical form (e.g., for transparency auditing) could compute a mismatched ciphertext.

Reproduction: Serialize the same serde_json::Value through canonical_record under different serde_cbor feature flags or crate versions; observe byte-level divergence beyond key ordering.

Recommendation: Either replace serde_cbor with a deterministic CBOR library (e.g., minicbor with explicit encode operations), or document that the "canonical" claim is aspirational and that authenticated records MUST be verified by AEAD decryption, never by re-encoding hash comparison.

Finding 2 (Low): encrypt_record bypasses attested NSM entropy architecture for per-record nonce

File: crates/timelock/src/lib.rs, function encrypt_record Trigger: Every proxied HTTP request producing a sealed record

pub fn encrypt_record(
    manifest: &SignedManifest,
    epoch_key: &[u8; 32],
    sequence: u64,
    plaintext: &[u8],
) -> Result<EncryptedRecord> {
    let mut nonce = [0; 24];
    OsRng.fill_bytes(&mut nonce);  // ← bypasses NSM entropy path
    ...
}

Issue: The entire enclave boot sequence (seed_os_rng → double NSM reseed → TLS provider → signing key from fill_random → epoch key from fill_random) meticulously routes all secret material through Attester::fill_random, which sources directly from the NSM in production with zero OS fallback (fail-closed per the audit requirement).

encrypt_record is called from the enclave hot path (v2_proxy::dispatch) but calls OsRng directly. The XChaCha20Poly1305 nonce buffer is a plain stack [u8; 24] (not Zeroizing). While nonces are not secret, this is an architectural gap:

1. The existing supplied_entropy_drives_native_generation_and_fails_closed test injects faults at all 16 entropy draws in generate_with_rng and confirms fail-closed behavior. encrypt_record has no corresponding test coverage for its entropy path.
2. A future regression that breaks the NSM→kernel seeding without detection would leave record nonces sourced silently from getrandom(2) rather than audited NSM bytes.
3. The nonce buffer lacks Zeroizing, inconsistent with the rest of the codebase's secret-handling discipline.

Reproduction: Call encrypt_record from the enclave path with a mocked/fault-injected OsRng; no error propagation or NSM entropy check gates the nonce generation.

Recommendation: Add an encrypt_record_with_rng API mirroring generate_with_rng, or at minimum document why OsRng after kernel seeding is sufficient and wrap the nonce buffer in Zeroizing.

Finding 3 (Low): nsm_random_chunk constructs ioctl command without validating direction field against _IOC_SIZESHIFT consistency

File: crates/enclave/src/attest.rs, function nsm_random_chunk

let operation = (3u64 << 30) | ((std::mem::size_of::<Message>() as u64) << 16) | (0x0a << 8);
let result = unsafe { libc::ioctl(fd, operation as libc::c_ulong, &mut message) };

Issue: The ioctl command is constructed via manual bit manipulation rather than the kernel's _IOWR macro. The direction bits 3 << 30 encode _IOC_READ | _IOC_WRITE, and the size field uses sizeof(Message) at bits 16-29. However:

• On 64-bit Linux, the kernel's _IOC_SIZEBITS is 14 bits (mask 0x3FFF). sizeof(Message) is 2 × sizeof(iovec) = 2 × 16 = 32 bytes on aarch64. This fits in 14 bits, so no overflow.
• The direction field at bits 30-31 uses value 3 for bidirectional. The kernel's _IOC_DIR mask expects _IOC_READ=2, _IOC_WRITE=4 (in the kernel's encoding, which differs from the userspace _IOC_READ=1, _IOC_WRITE=2). The 3 here refers to the glibc userspace encoding (_IOC_READ=2, _IOC_WRITE=1). The NSM driver expects the kernel encoding, but the AWS NSM driver's magic number 0x0A never collides with standard kernel ioctl ranges, and the driver's nsm_ioctl handler matches on the full command. This works by convention but not by verified ABI contract.

Reproduction: Compile against a future kernel header that reshuffles the NSM ioctl command layout; the manual bit construction would silently diverge from the authoritative _IOWR(NSM_MAGIC, ...) definition in the NSM driver headers. The code does not verify the constructed command against the driver's expected value.

Recommendation: Use the upstream aws_nitro_enclaves_nsm_api::driver::nsm_get_random helper or define the ioctl via inline nix::ioctl_readwrite! macro to eliminate manual encoding drift risk.

Finding 4 (Low): trusted_time_ms_uncached is called on every request, creating NSM serialization bottleneck with no backpressure

File: crates/enclave/src/v2_proxy.rs, function dispatch

let now = state.attester.trusted_time_ms_uncached()?;

Trigger: Every GET request to /f/https/...

Issue: trusted_time_ms_uncached performs a fresh NSM attestation ioctl (attest(&[], None)) on every proxied request:

pub fn trusted_time_ms_uncached(&self) -> Result<u64> {
    let signed = self.attest(&[], None)?
        .ok_or_else(|| anyhow::anyhow!("NSM returned no attestation document"))?;
    attestation_timestamp_ms(&signed)
}

This is in contrast to trusted_time_ms(), which caches the timestamp for 1 second. The /dev/nsm device serializes all ioctl calls within the enclave. An attacker flooding requests can:

1. Saturate the NSM with attestation generation requests.
2. Cause head-of-line blocking for the legitimate epoch activation path (v2_epoch::activate), which also calls trusted_time_ms_uncached().
3. While the public attestation endpoint has its own semaphore (capacity 4), the trusted_time_ms_uncached call in dispatch has no rate limiting before hitting the NSM.

The NSM is trusted per scope, so this does not cross a trust boundary, but it degrades availability of the enclave's epoch rotation and time-dependent admission checks.

Reproduction: Send 100 concurrent GET /f/https/example.com/ requests; observe trusted_time_ms_uncached serializing on the NSM while the epoch rotation task blocks on the same device.

Recommendation: Use the cached trusted_time_ms() in the request dispatch path (the 1-second staleness is already accepted elsewhere, and the epoch admission check tolerates minor staleness). Reserve trusted_time_ms_uncached for epoch activation only.

Finding 5 (Informational): Missing Zeroize on ed25519_dalek::SigningKey—signing key seed may not be wiped on enclave teardown

File: crates/enclave/src/v2_main.rs

let mut signing_seed = zeroize::Zeroizing::new([0u8; 32]);
attester.fill_random(signing_seed.as_mut())?;
let signer = relay_timelock::SigningKey::from_bytes(&signing_seed);
drop(signing_seed);  // zeroizes the temporary buffer
// signer lives in State for the enclave's entire lifetime

Issue: ed25519_dalek::SigningKey stores the 32-byte seed internally. The SigningKey struct implements Drop and Zeroize only if the zeroize crate feature is enabled in ed25519_dalek. The signer field is stored in Arc<State> and lives for the enclave's entire lifetime—it is never explicitly zeroized. If ed25519_dalek is compiled without zeroize support, the raw seed bytes persist in memory until enclave teardown.

The signing key is not the epoch key (which decrypts records), so exposure would allow forging puzzle manifests but not decrypting traffic. However, a forged manifest could be used in a social-engineering attack against clients that verify manifests against the attested public key without also verifying the attestation chain.

Reproduction: Check Cargo.lock for ed25519-dalek feature flags; if zeroize is absent, the signing key seed is never wiped.

Recommendation: Enable ed25519-dalek's zeroize feature, or wrap the SigningKey in Zeroizing at rest.

Summary

No critical or high-severity defects found. The core security primitives—NSM→kernel seeding order, double reseed with tick delay, direct NSM entropy for epoch/signing keys, fail-closed error propagation, AEAD context binding, sequence exhaustion prevention, and secret zeroization—are correctly implemented.