Skip to content

fix(cbor): stop decoding at the end of the input and bound declared lengths - #663

Open
solidsnakedev wants to merge 3 commits into
mainfrom
fix/cbor-decode-length-bound
Open

solidsnakedev wants to merge 3 commits into
mainfrom
fix/cbor-decode-length-bound

Conversation

@solidsnakedev

Copy link
Copy Markdown
Collaborator

CBOR.fromCBORHex("9a7fffffff") aborted the Node process with an out-of-memory error, and Transaction.fromCBORHex did the same. The five bytes declare an array of 2^31 - 1 items. Past the end of the input, decodeItemAt read undefined as an integer and returned an item. So decodeArrayAt looped over the declared length and filled the array it had allocated for it, and the map form ba7fffffff ran for tens of seconds.

decodeItemAt now throws CBORError at the end of the input. decodeArrayAt and decodeMapAt reject a definite length larger than the bytes left before they loop or allocate. Every array item takes at least one byte and every map entry two. Well-formed CBOR never meets either check, and decoding an 859 KB value takes the same time as before.

Closes #661

…engths

decodeItemAt throws CBORError at an offset past the end of the input, and
decodeArrayAt and decodeMapAt reject a definite length larger than the
bytes left before they loop or allocate.
@Kshot3000

Copy link
Copy Markdown

One extra case from verifying this against the same inputs, in case it helps: the 4-byte length in decodeLengthAt is built with signed 32-bit shifts, so a declared length with the top bit set reads as a negative number and slips past both new checks. Checked against this branch's code:

  • CBOR.fromCBORHex("baffffffff") (a map declaring 0xffffffff pairs) returns an empty Map instead of throwing — the length reads as -1, so the bound check and the loop both pass it by.
  • CBOR.fromCBORHex("9affffffff") (the array form) throws a raw RangeError (invalid array length), not a CBORError.

Reading that length unsigned (>>> 0 on the assembled value) makes both throw CBORError through your new array/map bounds checks, since 4294967295 is larger than any remaining input. I had opened #664 covering #661 before I saw this PR — it predates mine, so I'm closing #664 and leaving the case here instead.


Building in the open as an independent Cardano developer — not affiliated with the Cardano Foundation, IOG or Intersect.

@Kshot9000 · https://x.com/kshot9000 · github.com/Kshot3000
ADA donations: addr1q8hnl6vl5a6k3rw3n5g3jtte696zcl76kfatzv7gpswa9r0dj7fma6klq55y4ffm7tf0em09udnyhuk4ah92pl5x9jpqjae44v

cc @cardano-foundation @IntersectMBO

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CBOR decoder reads past the end of its input and trusts declared lengths

2 participants