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/binary: Uvarint function can potentially read entire buffer #41185

Open
mdevan opened this issue Sep 2, 2020 · 5 comments
Open

encoding/binary: Uvarint function can potentially read entire buffer #41185

mdevan opened this issue Sep 2, 2020 · 5 comments
Milestone

Comments

@mdevan
Copy link

@mdevan mdevan commented Sep 2, 2020

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

$ go version
go version go1.15 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

What did you do?

The function encoding/binary.Uvarint can potentially read the entire buffer passed to it, although it only needs to read a maximum of MaxVarintLen64 bytes.

https://play.golang.org/p/kx2TXUbPIyN

The function encoding/binary.ReadUvarint does not have this issue.

What did you expect to see?

0 -10

What did you see instead?

0 -1000
@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 2, 2020

See #40618 for when ReadUvarint was fixed.
We should fix Uvarint also.

@randall77 randall77 added this to the Go1.16 milestone Sep 2, 2020
@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 2, 2020

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Sep 8, 2020

We considered this and decided not to change the behaviour of Uvarint in the security release because it takes a buffer which is naturally limited by its length, so there is no risk of it consuming resources unexpectedly or unlimitedly.

If we want to change Uvarint too for consistency, we can, but there's no security reason, and it might be better not to change existing behaviour.

@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 9, 2020

I feel like we should fix this, if for no other reason than to make ReadUvarint and Uvarint consistent.

It is a good question, though, if anyone is depending on the current behavior. It seems unlikely to me, but it might be worth codesearching the corpus to see what people do with the second return value when it is <0.

@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 15, 2020

I searched through a fair amount of uses of the second return value of binary.Uvarint on github. I didn't see any code that ever distinguished different negative values. All the code did either:

  1. Ignore the result entirely.
  2. Compare it against 0 (< 0, <= 0, ...), usually to just report an error if bad.
  3. Use it to unconditionally slice something (which would panic if the return value is anything < 0)
  4. Add it unconditionally to an index into a byte slice (which will do bad things if the return value is anything < 0)

So I think we're safe redefining the behavior here.

After looking at all the uses, I have the sinking feeling that this API for reporting errors is not well understood by the general Go programmer. Oh well...

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
4 participants
You can’t perform that action at this time.