Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The private response buffer retains plaintext secrets without scrubbing, and handle validation after narrowing can allow malformed input to flush a real TPM object.
2 open issues
What changed in this PR
This PR addresses six reported security and correctness issues in wolfTPM’s firmware TPM, command-line examples, and API documentation.
Changes:
- Restricts template-bound policy sessions to creation commands and isolates TIS response processing from shared memory.
- Validates existing secure-boot NV indices, corrects persistence authority selection, and improves flush argument handling.
- Adds regression coverage and clarifies that
wolfTPM2_Clearperforms a destructive TPM operation.
| File | Description |
|---|---|
wolftpm/tpm2_wrap.h |
Corrects wolfTPM2_Clear documentation and related links. |
tests/fwtpm_unit_tests.c |
Adds PolicyTemplate authorization regression tests. |
src/fwtpm/fwtpm_tis.c |
Builds responses in a private buffer before copying them out. |
src/fwtpm/fwtpm_command.c |
Enforces template-policy scope and bounds response parameter lengths. |
examples/run_examples.sh |
Adds endorsement-key persistence and flush argument checks. |
examples/management/flush.c |
Handles help explicitly and rejects malformed handles. |
examples/keygen/create_primary.c |
Selects owner or platform authority for key persistence. |
examples/boot/secure_rot.c |
Checks existing NV index properties before reuse. |
Review effort: Balanced
dgarske
force-pushed
the
fenrir_20261103
branch
from
November 4, 2026 00:38
3961f83 to
00d4a31
Compare
dgarske
force-pushed
the
fenrir_20261103
branch
from
November 4, 2026 21:52
00d4a31 to
099b14a
Compare
embhorn
reviewed
Nov 5, 2026
embhorn
left a comment
Member
There was a problem hiding this comment.
Skoll Code Review
Scan type: review
Overall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] Check strtoul overflow on 32 bit targets —
examples/management/flush.c:71-75 - [Medium] Qualify which NV indices Clear removes —
wolftpm/tpm2_wrap.h:3656-3660 - [Medium] Cover rejection of mismatched existing ROT indices —
examples/boot/secure_rot.c:235-244 - [Medium] Exercise the new TIS response staging path —
src/fwtpm/fwtpm_tis.c:163-193
Review generated by Skoll
dgarske
force-pushed
the
fenrir_20261103
branch
from
November 5, 2026 16:59
099b14a to
d0577e8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Fixes for six Fenrir reports, one commit each.
secure_rotreuses an existing NV index only when its attributes, size, name algorithm and policy match what it would create, and prints the hash it reads in every build.create_primary -eh -store=persists the key with owner authorization, since EvictControl takes the owner or platform hierarchy.flushprints usage for-h/--helpand rejects a non-numeric handle.wolfTPM2_Cleardocumentation now describes TPM2_Clear and points towolfTPM2_Cleanupfor deinitialization.Tests: a fwTPM unit test for PolicyTemplate scoping, HAL tests for the TIS GO success and error responses, and
run_examples.shcoverage forcreate_primary -eh -store, theflusharguments and rejection of a mismatchedsecure_rotindex.