feat!: default-on size limits on CAR decode and encode#186
Conversation
| } | ||
| length += (reader.pos - start) | ||
| const cid = await readCid(reader) | ||
| const cid = await readCid(reader, options) |
There was a problem hiding this comment.
we know length at this point, we should make sure that readCid can't read beyond it, short length, long cid = error; currently this allows a badly declared CID in here to blow past any limitation
There was a problem hiding this comment.
well, you have maxCidLength in there, but still if the section is declared to be 1Kb and the CID is longer than that it shoudl error or if there is no maxCidLength it still should be capped at length
There was a problem hiding this comment.
probably rework this whole function I reckon, so we pass in the unmodified section length to readCid something like:
const sectionLength = decodeVarint(await reader.upTo(8), reader)
const lengthPrefixLength = Number(reader.pos - start)
const { cid, cidLength } = await readCid(reader, options, sectionLength)
const blockLength = sectionLength - cidLength
if (blockLength < 0) {
throw new Error('Invalid CAR section')
}
return {
cid,
length: lengthPrefixLength + sectionLength,
blockLength
}|
As per slack, I really think we should just default-on limits and let the user override them; this is already done in Go and we should do it here. The fact that the go parser isn't going to let large values through already puts an effective protocol limit on this format so we're not really going backward here. Anyone streaming bytes is going to have the same problems with potentially malicious (or just erroneous) input so we should solve it properly. I think we should adopt the Go limits for defaults. But we also have a parity problem because here you're doing CID and block limits separately rather than section limits. It would be easier if we had a whole section limit and not a block limit, kept the CID separate. In Go we arrived at the following limits:
There is also
And note that that CID limit is bigger than the section limit anyway, so it's just a separate safety hatch. If we adopt a section limit and cap |
| * @returns {CarDecoder} | ||
| */ | ||
| export function createDecoder (reader) { | ||
| export function createDecoder (reader, options) { |
There was a problem hiding this comment.
it'd be good to do a cheap validation of options here to not let invalid and non-uint values get by
|
I'll rework to use go's section limits with same defaults like you suggested originally 👍 |
Aligns js-car's size limits with go-car: two caps that are on by default, bounding the largest single allocation a decoder makes and ensuring the library never emits a CAR it would refuse to read back. - maxAllowedHeaderSize (default 32 MiB) and maxAllowedSectionSize (default 8 MiB) on CarCodecOptions, using go-car's names and defaults; the section cap bounds the CID and block body combined. Both count the varint-declared length; a length strictly greater than a cap throws RangeError (decode from the length prefix before buffering, encode before writing). undefined uses the default, 0 rejects every section, Number.MAX_SAFE_INTEGER disables a cap, and any other non-negative-safe-integer value throws TypeError. - readCid is bounded by its declared section, so a CID cannot read past the section it lives in. - A hardcoded 32 MiB digest cap (go-cid's maxDigestAlloc) bounds the multihash digest independently, so raising maxAllowedSectionSize stays safe. - Enforced on every decode and encode entry point (streaming, in-memory, CarBufferReader, CarBufferWriter, CarIndexedReader.fromFile), so one CarCodecOptions profile holds on both read and write. updateRootsInBytes and updateRootsInFile keep their signatures under the default header cap. BREAKING CHANGE: decode and encode now enforce default-on size caps. With no options passed, decode throws for a section over 8 MiB or a header over 32 MiB on every entry point (including fromBytes and CarBufferReader), and encode throws for a section over 8 MiB. Opt out per cap with Number.MAX_SAFE_INTEGER. Closes ipld#185
62bfb0c to
7d5e667
Compare
|
reworked to the default-on go-parity design per your review. the pr description has been updated. |
…rim tests - CarBufferWriterOptions extends CarCodecOptions; CarLimits = Required<CarCodecOptions> - getMultihashLength returns a number and enforces the 32 MiB digest cap itself (MAX_DIGEST_ALLOC moved to decoder-common.js), dropping the duplicated check from both readCid functions - reword resolveLimits JSDoc - drop overlapping size-limit tests: fromBytes-threading for CarReader/CarIndexer, CarCIDIterator defaults-on (shares CarBlockIterator's path), resolveLimits both-caps override
Aligns js-car's size limits with go-car: two
caps that are on by default, bounding the largest single allocation a decoder
makes and ensuring the library never emits a CAR it would refuse to read back.
Changes
maxAllowedHeaderSize(default 32 MiB) andmaxAllowedSectionSize(default 8 MiB) on
CarCodecOptions, using go-car's names and defaults. Thesection cap bounds the CID and block body combined. Both count the
varint-declared length; a length strictly greater than a cap throws
RangeError(decode: from the length prefix before buffering; encode: beforewriting).
undefineduses the default,0rejects every section,Number.MAX_SAFE_INTEGERdisables a cap, and any other non-negative-safe-integervalue throws
TypeError.readCidis bounded by its declared section, so a CID cannot read past thesection it lives in.
maxDigestAlloc) bounds themultihash digest independently, so raising
maxAllowedSectionSizestays safe.CarBufferReader,CarBufferWriter,CarIndexedReader.fromFile), so oneCarCodecOptionsprofile holds on both read and write.updateRootsInBytes/updateRootsInFilekeep their signatures under thedefault header cap.
Breaking change
With no options passed, decode now throws for a section over 8 MiB or a header
over 32 MiB on every entry point (including
fromBytesandCarBufferReader),and encode throws for a section over 8 MiB. Opt out per-cap with
Number.MAX_SAFE_INTEGER.Closes #185