fix(protocol): tpm exponent incorrectly derived#630
Open
james-d-elliott wants to merge 1 commit intomasterfrom
Open
fix(protocol): tpm exponent incorrectly derived#630james-d-elliott wants to merge 1 commit intomasterfrom
james-d-elliott wants to merge 1 commit intomasterfrom
Conversation
WalkthroughRewrote RSA exponent reconstruction in TPM attestation validation to accumulate bytes from the entire Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #630 +/- ##
==========================================
+ Coverage 71.68% 71.87% +0.19%
==========================================
Files 45 45
Lines 3125 3125
==========================================
+ Hits 2240 2246 +6
+ Misses 651 646 -5
+ Partials 234 233 -1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
protocol/attestation_tpm.go (1)
120-122: Guard exponent reconstruction againstuint32truncation.At Line 120-122, rebuilding
expintouint32without bounds checks can silently wrap for exponents longer than 4 bytes, which may let a mismatched exponent compare equal after truncation. Consider rejecting overflow (or length > 4) before comparison.Proposed patch
for _, b := range k.Exponent { + if exp > (^uint32(0) >> 8) { + return "", nil, ErrAttestationFormat.WithDetails("Unsupported RSA exponent size in credentialPublicKey") + } + exp = (exp << 8) | uint32(b) } + + if exp == 0 { + return "", nil, ErrAttestationFormat.WithDetails("Invalid RSA exponent in credentialPublicKey") + }Run this to verify whether upstream validation already guarantees a 32-bit-safe exponent (which would make this defensive check redundant but still clearer at this call site):
#!/bin/bash set -euo pipefail # Locate RSA exponent parsing and validation paths rg -n -C4 --type=go 'func parseRSAPublicKeyDataExponent|parseRSAPublicKeyDataExponent\(|func validateRSAPublicKey|invalid exponent length|ErrUnsupportedKey' # Inspect TPM attestation exponent comparison site rg -n -C4 --type=go 'case webauthncose\.RSAPublicKeyData|exp = \(exp << 8\) \| uint32\(b\)|tpm2Exponent\(params\)'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/attestation_tpm.go` around lines 120 - 122, The exponent reconstruction loop that builds exp from k.Exponent can overflow a uint32 when the exponent is longer than 4 bytes; update the logic in protocol/attestation_tpm.go (the loop that iterates k.Exponent and the site that uses the resulting exp, e.g., tpm2Exponent/any exponent comparison) to explicitly guard against overflow by rejecting exponents longer than 4 bytes (len(k.Exponent) > 4) or by checking before each shift that exp will not exceed uint32 bounds and returning an error if it would; ensure the reject path returns a clear error indicating unsupported exponent length so comparisons never silently truncate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@protocol/attestation_tpm.go`:
- Around line 120-122: The exponent reconstruction loop that builds exp from
k.Exponent can overflow a uint32 when the exponent is longer than 4 bytes;
update the logic in protocol/attestation_tpm.go (the loop that iterates
k.Exponent and the site that uses the resulting exp, e.g., tpm2Exponent/any
exponent comparison) to explicitly guard against overflow by rejecting exponents
longer than 4 bytes (len(k.Exponent) > 4) or by checking before each shift that
exp will not exceed uint32 bounds and returning an error if it would; ensure the
reject path returns a clear error indicating unsupported exponent length so
comparisons never silently truncate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e0e0845c-0cdb-46f9-8932-91687d6abb77
📒 Files selected for processing (1)
protocol/attestation_tpm.go
james-d-elliott
force-pushed
the
fix-tpm-exp-derive
branch
from
7af63dc to
045b3ac
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
protocol/attestation_tpm_test.go (2)
463-521: Test validates the algorithm but doesn't exercise the actual code path.This test duplicates the big-endian parsing logic inline rather than calling or exercising the actual code in
attestation_tpm.go. If the production code diverges or contains a bug, this test won't catch it.Consider either:
- Extracting the parsing logic into a shared helper function that both the test and production code use, or
- Constructing test attestation objects that exercise the full
attestationFormatValidationHandlerTPMcode path with various exponent values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/attestation_tpm_test.go` around lines 463 - 521, The test TestCOSEExponentBigEndianParsing currently implements its own big‑endian parsing loop instead of exercising the production code in attestation_tpm.go; refactor by extracting the exponent parsing into a single helper (e.g., parseCOSEExponent or COSEKeyExponentToUint32) and update attestationFormatValidationHandlerTPM to use that helper, then change this test to call the helper (or better, build minimal TPM attestation objects and call attestationFormatValidationHandlerTPM) for each exponent case so the actual production parsing path is exercised.
1196-1203:uint32ToByteshelper is incorrect for non-palindromic exponents.This function uses little-endian encoding and truncates to 3 bytes, which only produces correct big-endian output for exponents like 65537 (0x010001) that happen to be palindromic in their byte representation.
For example, exponent
3produces[0x03, 0x00, 0x00]which parses as 196608 in big-endian, not 3.This doesn't affect current tests since Go's RSA key generation uses 65537, but it's a latent bug if tests are extended to use other exponents.
🔧 Proposed fix using big-endian encoding
func uint32ToBytes(i uint32) []byte { - t, o := make([]byte, 4), make([]byte, 3) - - binary.LittleEndian.PutUint32(t, i) - - copy(o, t) - - return o + buf := make([]byte, 4) + binary.BigEndian.PutUint32(buf, i) + // Trim leading zeros but keep at least one byte + for len(buf) > 1 && buf[0] == 0 { + buf = buf[1:] + } + return buf }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/attestation_tpm_test.go` around lines 1196 - 1203, The uint32ToBytes helper currently writes the uint32 in little-endian and truncates to 3 bytes, producing incorrect big-endian exponents for non-palindromic values; change it to encode the value in big-endian (use binary.BigEndian.PutUint32) and return the least-significant 3 bytes in big-endian order (i.e., take the last 3 bytes of the 4-byte big-endian buffer) so small exponents like 3 become [0x00,0x00,0x03] as expected; update the function uint32ToBytes accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@protocol/attestation_tpm_test.go`:
- Around line 463-521: The test TestCOSEExponentBigEndianParsing currently
implements its own big‑endian parsing loop instead of exercising the production
code in attestation_tpm.go; refactor by extracting the exponent parsing into a
single helper (e.g., parseCOSEExponent or COSEKeyExponentToUint32) and update
attestationFormatValidationHandlerTPM to use that helper, then change this test
to call the helper (or better, build minimal TPM attestation objects and call
attestationFormatValidationHandlerTPM) for each exponent case so the actual
production parsing path is exercised.
- Around line 1196-1203: The uint32ToBytes helper currently writes the uint32 in
little-endian and truncates to 3 bytes, producing incorrect big-endian exponents
for non-palindromic values; change it to encode the value in big-endian (use
binary.BigEndian.PutUint32) and return the least-significant 3 bytes in
big-endian order (i.e., take the last 3 bytes of the 4-byte big-endian buffer)
so small exponents like 3 become [0x00,0x00,0x03] as expected; update the
function uint32ToBytes accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a3cc5ab5-0ee1-47d6-a3a6-02f5e0752dac
📒 Files selected for processing (2)
protocol/attestation_tpm.goprotocol/attestation_tpm_test.go
This fixes an issue where the exponent bytes are read to an integer in little-endian rather than big-endian.
james-d-elliott
force-pushed
the
fix-tpm-exp-derive
branch
from
045b3ac to
13599ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
protocol/attestation_spec_test.go (1)
191-193: Assert the credential COSE alg here if this is meant to be the RS256 fixture.Lines 191-193 describe this as the RS256 case, but Line 217 is only checking
attStmt.alg, i.e. the AIK signature algorithm. An explicit assertion on the credential public key's COSE alg would make it clear that this regression test is exercising the RSA/RS256 credential path.♻️ Suggested assertion
+ var pk webauthncose.PublicKeyData + require.NoError(t, webauthncbor.Unmarshal(att.AuthData.AttData.CredentialPublicKey, &pk)) + assert.Equal(t, int64(webauthncose.AlgRS256), pk.Algorithm) + alg, ok := att.AttStatement[stmtAlgorithm].(int64) require.True(t, ok) assert.Equal(t, int64(webauthncose.AlgRS1), alg)Also applies to: 215-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/attestation_spec_test.go` around lines 191 - 193, Test description claims RS256 but only asserts attStmt.alg (the AIK signature); add an explicit assertion that the credential public key's COSE alg equals the RS256 COSE value (numeric -257) to ensure the credential is RSA/RS256. Locate the credential public key in the parsed authData (e.g. the AttestedCredentialData -> CredentialPublicKey or similar structure used in the test) and, alongside the existing attStmt.alg assertion, add an assertion that that COSE alg field equals -257. Apply the same extra assertion in both places mentioned around the attStmt.alg checks (the blocks covering lines ~191-193 and ~215-217).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protocol/attestation_tpm.go`:
- Line 105: The exponent accumulator variable exp (declared as uint32) can
overflow when reconstructing from CBOR bytes in the loop that does exp = (exp <<
8) | uint32(b) and then compares to the expected exponent; change the
reconstruction to guard against overflow by either (a) validating the CBOR
exponent length before shifting and returning an error if it exceeds 4 bytes, or
(b) parse into a larger type (e.g., uint64 or math/big.Int) and explicitly check
that the decoded value fits in uint32 before assigning/ comparing; update the
code paths around the exp variable, the CBOR reconstruction loop, and the
subsequent comparison to return a clear error on overflow instead of silently
truncating.
---
Nitpick comments:
In `@protocol/attestation_spec_test.go`:
- Around line 191-193: Test description claims RS256 but only asserts
attStmt.alg (the AIK signature); add an explicit assertion that the credential
public key's COSE alg equals the RS256 COSE value (numeric -257) to ensure the
credential is RSA/RS256. Locate the credential public key in the parsed authData
(e.g. the AttestedCredentialData -> CredentialPublicKey or similar structure
used in the test) and, alongside the existing attStmt.alg assertion, add an
assertion that that COSE alg field equals -257. Apply the same extra assertion
in both places mentioned around the attStmt.alg checks (the blocks covering
lines ~191-193 and ~215-217).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 998ca039-7a28-4de0-ab45-afce2a42a900
📒 Files selected for processing (3)
protocol/attestation_spec_test.goprotocol/attestation_tpm.goprotocol/attestation_tpm_test.go
✅ Files skipped from review due to trivial changes (1)
- protocol/attestation_tpm_test.go
| var ( | ||
| params *tpm2.TPMSRSAParms | ||
| modulus *tpm2.TPM2BPublicKeyRSA | ||
| exp uint32 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the generic RSA exponent parser used elsewhere in the repo.
sed -n '567,585p' protocol/webauthncose/webauthncose.go
# Demonstrate why the uint32 accumulator here can wrap.
python - <<'PY'
cose_exponent = [0x01, 0x00, 0x00, 0x00, 0x01]
wrapped = 0
for b in cose_exponent:
wrapped = ((wrapped << 8) | b) & 0xffffffff
full_width = 0
for b in cose_exponent:
full_width = (full_width << 8) | b
print("uint32 accumulator:", wrapped) # expected: 1
print("full-width integer:", full_width) # expected: 4294967297
PYRepository: go-webauthn/webauthn
Length of output: 510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the code in attestation_tpm.go around the flagged lines
sed -n '95,130p' protocol/attestation_tpm.goRepository: go-webauthn/webauthn
Length of output: 1359
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the tpm2Exponent function to see what it returns
rg -A 5 "func tpm2Exponent" protocol/attestation_tpm.go
# Find the RSAPublicKeyData struct definition
rg -A 10 "type RSAPublicKeyData struct" protocol/webauthncose/webauthncose.goRepository: go-webauthn/webauthn
Length of output: 509
Guard the uint32 exponent accumulator against overflow.
Line 105 declares the exponent accumulator as uint32. The reconstruction loop at Lines 120-122 processes raw CBOR bytes via exp = (exp << 8) | uint32(b) without bounds checking. Any exponent wider than 4 bytes will silently truncate, allowing a mismatched exponent to falsely compare equal to the expected value at Line 124. For example, the 5-byte exponent [0x01, 0x00, 0x00, 0x00, 0x01] (value 4294967297) wraps to 0x00000001 under uint32, matching an expected exponent of 1.
⚠️ Suggested guard
for _, b := range k.Exponent {
+ if exp > (1<<24)-1 {
+ return "", nil, ErrUnsupportedKey
+ }
exp = (exp << 8) | uint32(b)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@protocol/attestation_tpm.go` at line 105, The exponent accumulator variable
exp (declared as uint32) can overflow when reconstructing from CBOR bytes in the
loop that does exp = (exp << 8) | uint32(b) and then compares to the expected
exponent; change the reconstruction to guard against overflow by either (a)
validating the CBOR exponent length before shifting and returning an error if it
exceeds 4 bytes, or (b) parse into a larger type (e.g., uint64 or math/big.Int)
and explicitly check that the decoded value fits in uint32 before assigning/
comparing; update the code paths around the exp variable, the CBOR
reconstruction loop, and the subsequent comparison to return a clear error on
overflow instead of silently truncating.
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
1 participant
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.
This fixes an issue where the exponent bytes are read to an integer in little-endian rather than big-endian.