Conversation
Signed-off-by: Effi-S <effi.szt@gmail.com>
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 #2189
Summary
typed.go's ASN.1 decoders discardasn1.Unmarshal'srestreturn value, so any number of trailing bytes appended after a validly-encoded token or metadata payload are silently accepted rather than rejected — the classic ASN.1 malleability pattern: two distinct byte strings decode to the same value. Neither decoder has aFuzzXxxtarget, which AGENTS.md mandates for any exported function that parses untrusted/attacker-controlled bytes; every sibling ASN.1 decoder in the repo (token/core/common/encoding/asn1,token/core/fabtoken/...,token/core/zkatdlog/...) already has one.Where
token/services/tokens/typed.go—UnmarshalTypedToken(~line 31) andUnmarshalTypedMetadata(~line 65) each callasn1.Unmarshal(raw, &typed)and check only the returned error, ignoring therest []bytereturn that reports unconsumed trailing bytes.Impact
A byte string
XandX + arbitrary trailerare treated as identical tokens/metadata by every caller of these two functions. Depending on where the raw bytes originate (any wire format that carries a typed token/metadata payload), this is a substrate for hash/signature-mismatch or replay-style confusion between two representations that "look the same" to this decoder but differ byte-for-byte.Reproduction
Both tests pass — i.e., the trailing-byte acceptance is confirmed present on current
main. (AFuzzUnmarshalTypedTokenNoPanic/FuzzUnmarshalTypedMetadataNoPanicpair, seeded with valid/empty/truncated/valid+trailer inputs, also ran clean for 20s with no panics — the decoders don't crash on malformed input, they just accept more than they should.)Severity
Medium — a real ASN.1 malleability bug with no panic/crash component, but no fuzz coverage either, on functions that parse externally-sourced bytes.