BUG/MINOR: peers: reject over-long varints in intdecode()

intdecode() shifts continuation bytes by 7 more bits for as long as they
have their stop bit set, without any upper bound on the shift count. A
64-bit value never needs more than 10 bytes, i.e. a shift of 60, but a
malformed message can carry a varint whose 10th continuation byte would
be shifted by 67. Shifting a 64-bit value by 64 or more is undefined
behavior, reported by UBSan, and yields a garbage value.

Check the shift count along with the end of buffer before consuming the
byte, so that such an encoding is rejected. Values up to 2^64-1, which is
encoded on exactly 10 bytes, are still accepted.

Only a buggy or malicious peer sending a malformed varint can reach this:
a compliant peer never emits more than 10 bytes, and peer sessions are
expected to run on trusted networks. The decoded value is later
range-checked against the message bounds, so no memory safety issue is
known; this is UB hardening.

This is long-standing and can be backported to all stable versions.

diff --git a/src/peers.c b/src/peers.c
index 7db63eeba64e5a52de52dabee1df6a85cc1d28e2..8da94578c804497837263306170a302a9e64c5f7 100644 (file)
--- a/src/peers.c
+++ b/src/peers.c
@@ -601,7 +601,8 @@ uint64_t intdecode(char **str, char *end)
        if (i >= PEER_ENC_2BYTES_MIN) {
                shift = PEER_ENC_2BYTES_MIN_BITS;
                do {
-                       if (msg >= (unsigned char *)end)
+                       /* more than 64 bits would be an invalid encoding */
+                       if (msg >= (unsigned char *)end || shift >= 64)
                                goto fail;
                        i += (uint64_t)*msg << shift;
                        shift += PEER_ENC_STOP_BIT;