io/varint.go: binary.Uvarint can return negative lenLen

[GoogleCodeExporter]

If the incoming data is corrupt or malicious, 
the call to http://golang.org/pkg/encoding/binary/#Uvarint
in http://godoc.org/code.google.com/p/gogoprotobuf/io#NewDelimitedReader
ReadMsg can cause a slice index out of bounds panic.

The code in
https://code.google.com/p/gogoprotobuf/source/browse/io/varint.go#98
should check for lenLen <= 0 and handle it somehow (0 is internal error, <0 is 
corrupt input). I don't think there's any graceful way to recover, so maybe the 
cleanest way is to have the error become sticky; all further ReadMsg calls 
should get the same message.

I can contribute code if that helps, just wanted to get buy-in on the idea 
first.

Original issue reported on code.google.com by tommi.vi...@gmail.com on 23 Jun 2014 at 5:59

[GoogleCodeExporter]

I agree that ReadMsg should return an error given lenLen <= 0, but my first 
instinct is that, that is all that needs to change, other than also adding a 
test.

Maybe I just need to see an example of such a sticky error in the go standard 
library?

Original comment by awalterschulze on 23 Jun 2014 at 6:10

• Changed state: Accepted

[GoogleCodeExporter]

For example http://golang.org/src/pkg/bufio/scan.go#L30

The point of the sticky error is to guard against a buggy caller, that might 
e.g. log the error, but continue the reading loop. For gogoprotobuf/io, that 
would mean a stream of gibberish messages, with errors interspersed every now 
and then.

You could say that the caller must stop reading after the first error, and just 
leave it at that. I'm ok with that. This bug is really about the fact that you 
can make varintReader.ReadMsg panic with bad input. 

Original comment by tommi.vi...@gmail.com on 23 Jun 2014 at 7:10

[GoogleCodeExporter]

I fixed the main issue for now
https://code.google.com/p/gogoprotobuf/source/detail?r=d228c1a206c3a756d7ec6cc35
79d92d00c35a161

We can revisit the sticky error when I come back from holiday

Original comment by awalterschulze on 24 Jun 2014 at 8:12

[GoogleCodeExporter]

I am happy with the non sticky error.
If you are unhappy please reopen the issue.

Thank you for reporting the issue.

Original comment by awalterschulze on 4 Aug 2014 at 3:02

[GoogleCodeExporter]

Original comment by awalterschulze on 4 Aug 2014 at 3:03

• Changed state: Fixed

[GoogleCodeExporter]

I'm fine with this.

Original comment by tommi.vi...@gmail.com on 4 Aug 2014 at 3:04