cap varint length in DecodeVarint to avoid undefined shift#2106
cap varint length in DecodeVarint to avoid undefined shift#2106nabhan06 wants to merge 2 commits into
Conversation
|
Looks reasonable, can you add a test case that would have caught this too? |
|
Added proto_test.cc with a DecodeVarint case: a varint field whose bytes all set the continuation bit. Without the cap that drives the shift to |
| // All of the data that passes through this code is trusted because it flows | ||
| // through a closed loop within the absl::LogMessage object. It is not a robust | ||
| // protocol buffer encoder or decoder. |
There was a problem hiding this comment.
Sorry just noticed this comment when I was wondering where this was even called from. What issue did you hit with this? This is definitely not a hardened protobuf parser
There was a problem hiding this comment.
When I was asking for a test I was more wondering if this was reachable from public APIs. This unit-test illustrated the theoretical problem, but it uses internal APIs that I suspect aren't actually reachable
There was a problem hiding this comment.
No production crash behind it. I hit the << 70 while running the log internal code under UBSan and feeding DecodeFrom a deliberately malformed span (a varint field followed by 10 bytes that all set the continuation bit). Fair point that this isn't a hardened parser, but the loop already runs up to buf->size() specifically to tolerate junk input, and Decode32Bit/Decode64Bit right below already cap their loops to avoid the same over-wide shift. This just makes DecodeVarint consistent with them. If you'd rather not touch this path at all I'm fine closing it.
3 participants
DecodeVarint in absl/log/internal/proto.cc accumulates a base-128 varint with
value |= (byte & 0x7f) << 7 * sbut never caps the byte index, unlike the sibling Decode32Bit and Decode64Bit helpers that stop at sizeof(value). A malformed or over-long buffer whose first 10 bytes all set the continuation bit drives the shift to<< 70on a 64-bit value, which UBSan flags as an undefined shift exponent. This caps the loop at the 10-byte maximum of a 64-bit varint, matching the sibling decoders, with no change for well-formed input.