Skip to content

[test] add new fuzz targets for tbor KDF/HMAC/RSA crypto commands - #766

Open
David Zimmermann (zimmy87) wants to merge 16 commits into
mainfrom
user/v-davidz/add_tbor_kdf_hmac_rsa
Open

David Zimmermann (zimmy87) wants to merge 16 commits into
mainfrom
user/v-davidz/add_tbor_kdf_hmac_rsa

Conversation

@zimmy87

@zimmy87 David Zimmermann (zimmy87) commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Adds four new fuzz targets as well as a fix for a test issue found in fuzz_tbor_attest_key

Copilot AI balanced review requested due to automatic review settings October 5, 2026 20:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

RSA unwrap currently aborts during setup, and several valid RSA/HMAC paths are misclassified or unreachable.

Review effort: Balanced
Findings: 4 Medium severity

Open (4)
What changed in this PR

Adds TBOR fuzz coverage for KDF, HMAC, RSA unwrap, and RSA modular exponentiation commands, plus broadens an attestation rejection check.

Changes:

  • Adds five crypto fuzz targets and Cargo registrations.
  • Updates oversized attestation-key error handling.
File Description
fuzz/​Cargo.toml Registers the new fuzz binaries.
fuzz_tbor_attest_key.rs Accepts device-side oversized-key rejection.
fuzz_tbor_hkdf.rs Adds HKDF command fuzzing.
fuzz_tbor_kbkdf.rs Adds KBKDF-equivalent fuzzing through HKDF.
fuzz_tbor_hmac.rs Adds HMAC fuzzing.
fuzz_tbor_rsa_mod_exp.rs Adds RSA modular-exponentiation fuzzing.
fuzz_tbor_rsa_unwrap.rs Adds wrapped-key import fuzzing.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_rsa_mod_exp.rs Outdated
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_rsa_mod_exp.rs Outdated
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_rsa_unwrap.rs Outdated
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_rsa_unwrap.rs
Co-authored-by: zimmy87 <5205889+zimmy87@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 5, 2026 22:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Valid RSA/ECC unwrap permissions are misclassified, and the KBKDF target only duplicates HKDF coverage.

Review effort: Balanced
Findings: None

Resolved since last review (4)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Target fuzzes HKDF instead of the advertised KBKDF path

fuzz/​fuzz_targets/​ddi/​tbor/​fuzz_tbor_kbkdf.rs:116

This target does not exercise KBKDF: it sends the same HkdfDerive request as fuzz_tbor_hkdf, whose handler always runs HKDF extract/expand. SP800-108 counter-HMAC KBKDF uses the distinct sp800_108_kdf path (fw/core/lib/src/ddi/mbor/kbkdf_derive.rs:73-80), so mapping label/context to salt/info only duplicates HKDF coverage and leaves the advertised KBKDF behavior unfuzzed. Add a real TBOR KBKDF command/selector, or remove/rename this target so it does not claim that coverage.

Medium severity Success predicate rejects valid UnwrapKey permission sets

fuzz/​fuzz_targets/​ddi/​tbor/​fuzz_tbor_rsa_unwrap.rs:340

This success predicate rejects two valid permission sets: UnwrapKey also accepts encrypt+decrypt for RSA/RSA-CRT and derive for ECC (fw/core/lib/src/ddi/tbor/unwrap_key.rs:118-125). If Fuzzed produces either set, the firmware correctly succeeds but this target reports a crash. Classify every accepted usage group rather than only the canonical generated one.

Copilot AI balanced review requested due to automatic review settings October 6, 2026 00:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The implementation adds four fuzz targets despite the PR description promising five.

Review effort: Balanced
Findings: 1 Low severity

Open (1)

Comment thread fuzz/Cargo.toml

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The RSA unwrap target’s usage validation conflicts with current firmware behavior and can report a predictable failure.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_rsa_unwrap.rs
Copilot AI balanced review requested due to automatic review settings October 6, 2026 04:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The targets align with firmware contracts and the previously identified issues are resolved.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_hmac.rs Outdated
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_hkdf.rs Outdated
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_hkdf.rs Outdated
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_rsa_mod_exp.rs Outdated
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_rsa_mod_exp.rs Outdated
Copilot AI balanced review requested due to automatic review settings October 7, 2026 02:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The fuzz targets align with the exercised command contracts, and no unresolved correctness issues were identified.

Review effort: Balanced
Findings: None

Copilot AI balanced review requested due to automatic review settings October 7, 2026 05:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

RSA modular-exponentiation fuzzing misclassifies valid SecurityDomain imports and consequently reports false crashes.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_rsa_mod_exp.rs
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 7, 2026 05:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

RSA mod-exp setup aborts before exercising several generated-key scope paths.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Provision unwrapping keys for all non-SecurityDomain scopes

fuzz/​fuzz_targets/​ddi/​tbor/​fuzz_tbor_rsa_mod_exp.rs:244

import_rsa_key always calls GetUnwrappingKey, but this setup provisions that key only for Ephemeral, Local, and SecurityDomain. Session, Unspecified, and Internal therefore hit the helper's expect with PendingKeyGeneration before UnwrapKey or RsaModExp is fuzzed. Finalize the partition for every non-SecurityDomain case so both valid Session requests and invalid-scope rejection paths reach the commands under test.

Copilot AI balanced review requested due to automatic review settings October 7, 2026 05:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The new targets align with firmware contracts and existing fuzz-harness conventions, with no unresolved correctness issues found.

Review effort: Balanced
Findings: None

Comment thread fuzz/fuzz_targets/common.rs
Comment thread fuzz/fuzz_targets/common.rs
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_hkdf.rs
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_hmac.rs Outdated
Comment thread fuzz/fuzz_targets/ddi/tbor/fuzz_tbor_rsa_unwrap.rs
Copilot AI balanced review requested due to automatic review settings October 8, 2026 18:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The extensive backend-dependent cryptographic fuzz oracles and security-domain setup warrant final human validation.

0 open findings

🧠 Review effort: Balanced

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.

4 participants