5 ms·
The issue: "Certain invalid inputs to ReadUvarint or ReadVarint could cause those functions to read an unlimited number of bytes from the ByteReader argument b
by jerf 6y ago
The issue:
"Certain invalid inputs to ReadUvarint or ReadVarint could cause those functions to read an unlimited number of bytes from the ByteReader argument before returning an error. This could lead to processing more input than expected when the caller is reading directly from a network and depends on ReadUvarint and ReadVarint only consuming a small, bounded number of bytes, even from invalid inputs.
"With the update, ReadUvarint and ReadVarint now always return after consuming a bounded number of bytes (specifically, MaxVarintLen64, which is 10). The result being returned has not changed; the functions merely detect and return some errors without reading as much input."
What's a varint? This: https://developers.google.com/protocol-buffers/docs/encoding https://developers.google.com/protocol-buffers/docs/encoding
You have to use varints to be affected, but of course you could be using them in a library without knowing it.
- baby 6y agoSo is it Golang’s varint or protobuf/grpc varint that had the bug? It’s not clear to me. Edit: https://go-review.googlesource.com/c/go/+/247120/ https://go-review.googlesource.com/c/go/+/247120/
- jeffbee 6y agoProto has a different structure with a fully unrolled loop. It could never read more than 10 bytes. encoding/binary just looped as long as the more bit was set, which is a hazard of more-bit encodings (which suck; unary-length-prefixed codes like UTF-8 are strictly better in every way).
- baby 6y agoWoops. Sounds like an big mistake. Fuzzing or even protest would have caught that.
- zeeboo 6y agoI think it's unlikely this would have been caught by fuzzing. I think you'd have to write assertions that it only read as many bytes as necessary, which is basically saying just don't write the bug in the first place.
- baby 6y agoNot sure but couldn't a fuzzer have created an input that would have timed out the fuzzer? I guess it also depends on the timeout you set when running it.
- morelisp 6y agoYou would also have needed to have a slow unboundedly large data stream on the other end. I fuzzed a bunch of varint-using code last month and never found this, because I was always working with in-memory buffers. The loop is simple enough that anything reasonably sized (e.g. under a GB) would probably not hit any reasonable timeout.
- jerf 6y agoHypothetically yes, but I'm not sure any common fuzzing strategy would catch that. Pure random mutations would never get this far, and code-coverage based mutation won't either. How to create a strategy that would catch this but not be rife with false positives and wasted fuzzing time is not immediately obvious.
- dmurray 6y agoGo doesn't have assertions. Of course it is not hard to mimic them, but it's an intentional choice to increase the friction of doing that and make it feel unidiomatic.
- MaxBarraclough 6y agoDoes Go really aim to discourage the programmer from adding runtime checks? What's the reasoning there?
- saurik 6y agoFWIW, more bit encodings allow you to quickly "resynchronize" a stream if you are dropped in the middle... UTF-8 is actually a hybrid and uses both strategies.