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: document that Unmarshal drops trailing elements #38545

Closed
AbstractSyntaxNotator opened this issue Apr 20, 2020 · 8 comments
Closed

Comments

@AbstractSyntaxNotator
Copy link

@AbstractSyntaxNotator AbstractSyntaxNotator commented Apr 20, 2020

What version of Go are you using (go version)?

go1.14

Does this issue reproduce with the latest release?

Yes, on Go playground the current version is 1.14.2

What did you do?

As shown in the following Go playground snippet, I define 2 structs:

type AB struct {
	A, B int
}

type ABC struct {
	A, B, C int
}

and then I first feed the smaller struct (AB) instance into the larger one (ABC):

	abBytes, _ := asn1.Marshal(AB{A: 1, B: 2})

	abc := ABC{}
	asn1.Unmarshal(abBytes, &abc)

then I assign the third field of the larger instance (ABC) and marshal the larger instance (ABC) to bytes

	abc.C = 3
	abcBytes, _ := asn1.Marshal(abc)

Followed by pouring the bytes from the larger instance (ABC) into an empty smaller instance (AB):

	ab := AB{}
	rest, err := asn1.Unmarshal(abcBytes, &ab)
	fmt.Println(rest, err) // [] <nil>

	fmt.Println(ab) // {1 2}

	fmt.Println(hex.EncodeToString(abBytes))  // 3006020101020102
	fmt.Println(hex.EncodeToString(abcBytes)) // 3009020101020102020103

What did you expect to see?

I expected the rest not to be empty! I should get a hint that there were trailing bytes, should I not?

	rest, err := asn1.Unmarshal(abcBytes, &ab)
	fmt.Println(rest, err) // [] <nil>

What did you see instead?

The rest is empty and no error was returned.

Needless to say, some applications might be... sensitive to this kind of behavior.

@AbstractSyntaxNotator AbstractSyntaxNotator changed the title encoding/asn1 silently drops trailing undefined fields encoding/asn1 Unmarshal silently drops trailing undefined fields Apr 20, 2020
@ianlancetaylor ianlancetaylor changed the title encoding/asn1 Unmarshal silently drops trailing undefined fields encoding/asn1: Unmarshal silently drops trailing undefined fields Apr 20, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 20, 2020

For better or for worse, that is how the encoding/asn1 package works. There is a comment in the source code:

		// We allow extra bytes at the end of the SEQUENCE because
		// adding elements to the end has been used in X.509 as the
		// version numbers have increased.

I don't think we can change that now.

CC @agl @FiloSottile

@AbstractSyntaxNotator
Copy link
Author

@AbstractSyntaxNotator AbstractSyntaxNotator commented Apr 20, 2020

I don't think we can change that now.

That's fine, I guess people would have to live with that, however:

  • These comments you quoted are not from the go-doc but buried deep within the code.
  • The go-doc doesn't say anything about when rest is (not)? empty, can we change that?
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 20, 2020

Yes, we can certainly change the documentation. Want to send a patch?

@AbstractSyntaxNotator
Copy link
Author

@AbstractSyntaxNotator AbstractSyntaxNotator commented Apr 20, 2020

I think it's best if someone else does it.

I have no knowledge of ASN1 and I also opened this throwaway user just for reporting this.

@FiloSottile FiloSottile changed the title encoding/asn1: Unmarshal silently drops trailing undefined fields encoding/asn1: document that Unmarshal drops trailing elements Apr 21, 2020
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Apr 21, 2020

This is indeed intended behavior, although something that surprised me at some point, too. Adding elements to SEQUENCEs is how ASN.1 is extended. If you want to check there's no extra elements, you should use golang.org/x/crypto/cryptobyte instead (and we do here and there for things like signature parsing).

@FiloSottile FiloSottile modified the milestones: Unplanned, Backlog Apr 21, 2020
@abemotion
Copy link

@abemotion abemotion commented Apr 23, 2020

rest is needed? initOffset is always zero and json.Unmarshal returns only error.
https://golang.org/src/encoding/asn1/asn1.go?s=30565:30631#L1082

func Unmarshal(b []byte, val interface{}) (rest []byte, err error) {
	return UnmarshalWithParams(b, val, "")
}
func UnmarshalWithParams(b []byte, val interface{}, params string) (rest []byte, err error) {
	v := reflect.ValueOf(val).Elem()
	offset, err := parseField(v, b, 0, parseFieldParameters(params))
	if err != nil {
		return nil, err
	}
	return b[offset:], nil
}
func parseField(v reflect.Value, bytes []byte, initOffset int, params fieldParameters) (offset int, err error)
@gopherbot
Copy link

@gopherbot gopherbot commented May 12, 2020

Change https://golang.org/cl/233537 mentions this issue: encoding/asn1: document what Unmarshal returns in rest

@katiehockman
Copy link
Member

@katiehockman katiehockman commented May 12, 2020

Marking this as a duplicate of #35680, which is addressed in https://golang.org/cl/233537.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.