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

Jump to bottom

encoding/asn1: document that Unmarshal drops trailing elements #38545

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

encoding/asn1: document that Unmarshal drops trailing elements #38545

AbstractSyntaxNotator opened this issue Apr 20, 2020 · 8 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@AbstractSyntaxNotator
Copy link

AbstractSyntaxNotator commented Apr 20, 2020 •
edited

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

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

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Apr 20, 2020
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Apr 20, 2020
@AbstractSyntaxNotator
Copy link
Author

AbstractSyntaxNotator commented Apr 20, 2020 •
edited

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

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

@AbstractSyntaxNotator
Copy link
Author

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 FiloSottile added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Apr 21, 2020
@FiloSottile
Copy link
Contributor

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

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

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

@katiehockman
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@FiloSottile @abemotion @ianlancetaylor @gopherbot @katiehockman @AbstractSyntaxNotator