Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

encoding/asn1: extra elements at the end of a sequence are completely ignored #35680

Open
katiehockman opened this issue Nov 18, 2019 · 3 comments
Assignees
Milestone

Comments

@katiehockman
Copy link
Member

@katiehockman katiehockman commented Nov 18, 2019

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:
302d021500aa6a258fbf7d90e15614676d377df8b10e38db4a0214496d5220b5f67d3532d1f991203bc3523b964c3b

I provided:

302f021500aa6a258fbf7d90e15614676d377df8b10e38db4a0214496d5220b5f67d3532d1f991203bc3523b964c3b0000

which 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 to asn1.Unmarshal was 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 rest when returned from asn1.Unmarshal. Both of the encoded strings above produce the same struct value without an error or anything in rest.

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)

@katiehockman katiehockman added this to the Go1.15 milestone Nov 18, 2019
@katiehockman katiehockman self-assigned this Nov 18, 2019
@katiehockman

This comment has been minimized.

Copy link
Member Author

@katiehockman katiehockman commented Nov 18, 2019

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Nov 19, 2019

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 rest, as expected.

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 FiloSottile changed the title encoding/asn1: garbage at the end of a sequence is completely ignored encoding/asn1: extra elements at the end of a sequence are completely ignored Nov 19, 2019
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 19, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.