fix: reject non-minimally encoded varints#343
Conversation
|
Nice work, a couple of thoughts on this as is:
The spec we work adopt here is https://github.com/multiformats/unsigned-varint/blob/master/README.md, and you'll note that even with this we don't fully comply. We should be rejecting varints longer than 9 bytes, so a number like Also on the encode side we have some problems:
I'd be fine merging this with just a bit more test extension so we lock in the behaviour, you're welcome to go further as well in this PR or in a follow-up. As long as we're doing it properly, we could just consider moving varint.js out of vendor and building the validation into the decode process because it'll be more efficient that way. |
Per the unsigned-varint spec a varint MUST be minimally encoded, yet varint.decode accepted overlong encodings (e.g. [0x81,0x00] for 1). Because CID and multihash decoding read their prefixes through this function, distinct byte sequences decoded to equal CIDs, making content addresses malleable. Reject encodings whose final byte is a redundant zero continuation.
22326d1 to
9fade84
Compare
|
Added all three: the overlong I left the encode-side issues (signed / non-integer / the |
The unsigned-varint spec requires that varints be minimally encoded, but
varint.decodeaccepts overlong encodings such as[0x81, 0x00](a non-minimal encoding of1).CID.decode,CID.decodeFirst,CID.inspectBytesandDigest.decodeall read their varint prefixes (version, codec, multihash code, digest length) through this function. As a result, distinct byte sequences decode to.equals()-equal CIDs and re-encode to different bytes than were supplied — content addresses become malleable, which can defeat dedup/cache-key logic that assumes a canonical binary form.A varint is non-minimal exactly when its final (most-significant) byte is a redundant zero group, so the check is
length > 1 && lastByte === 0.Tests added for the varint primitive and for
CID.decoderejecting a non-minimal version prefix.