proto: fix uint64->int overflow in table unmarshal #461
Conversation
proto/table_unmarshal.go
Outdated
| @@ -1849,7 +1849,7 @@ func findEndGroup(b []byte) (int, int) { | |||
| return -1, -1 | |||
| } | |||
| i += k | |||
| if i+int(m) > len(b) { | |||
| if uint64(len(b)) < uint64(i)+m { | |||
There was a problem hiding this comment.
Can't this still overflow with malicious values of m? E.g., if m was set to math.MaxUint64-i+1, then this check will never trigger.
Since, we know that i ≤ len(b), wouldn't a more safe check be: uint64(len(b)-i) < m since the LHS is guaranteed to be a positive value?
Similarly, we should also protect the WireFixed32 and WireFixed64 cases to be of this form, although it is far less likely those can be exploited.
There was a problem hiding this comment.
The i+m operation can wrap, although the harm is limited to misinterpreting the contents of the buffer--it can't panic, because we've ensured that the wrapped i+m is within the bounds of b.
Rewrote it anyway to be more correct. Fixed one other existing case where this could wrap at the same time.
No description provided.