Repository navigation
Conversation
6a19201 to
3d02300
Compare
|
@dgarske when you get back do you want to try and wrap this one up? |
3d02300 to
d4b6e8f
Compare
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: review
Overall recommendation: REQUEST_CHANGES
Findings: 5 total — 5 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
1 finding(s) not tied to a diff line (full detail below)
Posted findings
- [High] Report owner-auth restoration failures even after an earlier error —
examples/nvram/nvauth.c:341-346 - [Medium] Reject mismatched PCR pointer and length pairs —
src/tpm2_wrap.c:7903-7907,8051-8055 - [Medium] Use guaranteed zeroization for copied hierarchy credentials —
examples/nvram/nvauth.c:83,168,273-275,353 - [Medium] Cover successful no-PCR policy reads —
tests/unit_tests.c:2946-2959
Findings not tied to a diff line
Document hierarchy authorization for NV extend
File: wolftpm/tpm2_wrap.h:3332-3334
Function: wolfTPM2_NVExtend
The shared implementation now permits owner/platform authorization for wolfTPM2_NVExtend, but its public documentation omits the handle setup note added to the adjacent read and write wrappers. Callers cannot discover that they must retain the NV Name while replacing nv->handle.hndl and nv->handle.auth with the hierarchy credentials.
Recommendation: Add the same hierarchy-authorization usage note provided for the other affected NV wrappers.
Referenced code: wolftpm/tpm2_wrap.h:3332-3333 (2 lines)
Review generated by Skoll
d4b6e8f to
ea90d20
Compare
ea90d20 to
17097f4
Compare
Description
Adds owner/platform hierarchy authorization to the existing NV wrappers, plus an example for an NV index that is readable without a password while write and delete require the owner. No new API. ZD 20801.
Wrapper
wolfTPM2_NVWriteAuth,NVWriteAuthPolicy,NVExtend,NVReadAuthandNVReadAuthPolicyacceptnv->handle.hndl=TPM_RH_OWNERorTPM_RH_PLATFORM(withnv->handle.auth= hierarchy auth) as the authHandle for OWNERWRITE/PPWRITE/OWNERREAD/PPREAD indexes. Previously the name refresh went throughwolfTPM2_NVOpen(), which reset the authHandle to the NV index, so these indexes failed withTPM_RC_NV_AUTHORIZATION/TPM_RC_AUTH_UNAVAILABLE. The NV index Name is still refreshed for the cpHash whenTPMA_NV_WRITTENchanges.NVWriteAuthPolicy/NVReadAuthPolicyaccept no PCRs (pcrArrayNULL,pcrArraySz0) for a policy session the caller has already satisfied (PolicySecret, PolicyCommandCode, ...). Such a policy covers one chunk, so larger data returnsBUFFER_E.wolfTPM2_NVOpenshares a static Name helper with the above.Example
examples/nvram/nvauthTPMA_NV_AUTHREADwith an empty index auth for read,TPMA_NV_OWNERWRITEfor write, owner auth for delete.-policy:TPMA_NV_POLICYWRITEwith PolicyCommandCode(NV_Write) + PolicySecret(owner).-rotate=<pw>: changes the owner auth withTPM2_HierarchyChangeAuth(current owner auth only), shows the index still writes, then restores it.run_examples.shandexamples/README.md.ZD 20801
Testing
New unit test for an OWNERREAD/OWNERWRITE index with multi-chunk writes under an AES-CFB parameter encryption session, the index-auth write rejection, and the no-PCR
BUFFER_Epath.unit.testandrun_examples.shpass on the fwTPM and ibmswtpm2. Not tested on hardware.wolfTPM2_NVIncrementandwolfTPM2_NVWriteLocktake no separate index argument and stay index-authorized.TPMA_NV_POLICY_DELETEneeds platform auth (NV_UndefineSpaceSpecial) and is not covered.