encoding/asn1: extra elements at the end of a sequence are completely ignored #35680
Comments
katiehockman
added
the
NeedsFix
|
/cc @FiloSottile @agl @rsc |
|
ASN.1 structures are extended by appending elements, therefore extra elements at the end of a sequence are ignored. Since the length prefix of the sequence was updated to include the extra zero bytes, and the extra bytes are valid ASN.1, I believe this is intended behaviour. If you keep the second byte as 0x2d, then the extra bytes are returned in This does, indeed, allow signature malleability: it's possible to produce non-identical valid signatures from a valid signature. By standard cryptographic properties, that's fine, although some cryptocurrencies make additional assumptions and would need to re-encode to confirm that a signature is canonical. (ECDSA signatures are also malleable in that s can be negated in the scalar field without making the signature invalid.) |
FiloSottile
changed the title
|
I wasn't actually aware that appending elements was the intended extension mechanism for ASN.1 structures. In that case, it sounds like we should just document that in the asn1.Unmarshal docs. It does feel a little contradictory to have a DER parser that will always allow extra unknown elements, but I suppose we flushed all cases where malleability matters thanks to the fact that most of the ecosystem never actually implemented DER right? For ECDSA, we can just make a cryptobyte VerifyFromASN1 API per #20544. Most users don't want to deal with encoding/asn1 and math/big anyway. |
katiehockman
added
NeedsInvestigation
|
Change https://golang.org/cl/233537 mentions this issue: |
Does this issue reproduce with the latest release?
Yes
What did you do?
Appended garbage data (such as zeros, null, etc) to a hex encoded asn1 SEQUENCE type.
For example, rather than:
302d021500aa6a258fbf7d90e15614676d377df8b10e38db4a0214496d5220b5f67d3532d1f991203bc3523b964c3bI provided:
302f021500aa6a258fbf7d90e15614676d377df8b10e38db4a0214496d5220b5f67d3532d1f991203bc3523b964c3b0000which has a slightly larger length, and some trailing zeros.
https://play.golang.org/p/TrwfRC31i1m
What did you expect to see?
The dropped/ignored data should be provided in
rest, or documented otherwise, so users know that some of the data provided toasn1.Unmarshalwas not included in the resulting struct.Since this is often used alongside
ecdsa.Verify, the best long term solution would be implement an API similar to the one described in #20544 that can except an asn1 encoded string and verify it directly, returning 'false' if there is either a parsing issue (ie. some trailing garbage) or if the verification failed.What did you see instead?
The garbage at the end is simply dropped off and ignored, but not provided in
restwhen returned fromasn1.Unmarshal. Both of the encoded strings above produce the same struct value without an error or anything inrest.This is surprising, as I would expect DER encoding to have a 1:1 relationship between input and output, but also that data is dropped off without being included in rest (as many users likely expect)
The text was updated successfully, but these errors were encountered: