Go: encoding/binary: Uvarint function can potentially read entire buffer

[mdevan]

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]

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

[randall77]

@katiehockman

[FiloSottile]

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]

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]

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...