search

FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
310 stars 133 forks source link

Failed to handle case of alleged String with length of `Integer.MAX_VALUE` #259

Closed cowtowncoder closed 3 years ago

cowtowncoder commented 3 years ago

(from https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=32173)

Looks like CBORParser does not handle case of alleged String value with length of about 2 billion (Integer.MAX_VALUE). It should fail, but gracefully; right now handling produces negative length for checks and thinks it has all the content, tries to allocate all memory. Instead, it should recognize there isn't enough content and attempt chunked read which will then proceed to find that there isn't enough actual content to read.

cowtowncoder commented 3 years ago

I think I managed to deoptimize handling here. Should try not moving too fast. :-)

fmeum commented 3 years ago

Math.max(len + 3, len) in e35d4a04682ee779050ae9656d8d4ae2738efec3 looks off, but maybe I'm misunderstanding that part of the code.

cowtowncoder commented 3 years ago

@fmeum There's a comment next to it. It's not super clean, but what it achieves is that "needed" is never negative (previously there was int overflow to get it to negative, leading to parser think it had all contents). Input buffer should never be big enough to be close to Integer.MAX_VALUE so we should get on quick path.

Although now that I say that, I guess if caller passes input buffer explicitly they could theoretically pass that big a buffer... but I don't think it is possible, even then, to make it fail. One byte is needed for "String value" marker, and for length indicator we'll need 4 more bytes. As such, smallest offset for payload would be 5; and maximum available then Integer.MAX_VALUE - 5. So I think it is a safe check. Not very readable, granted.

(if someone can point a cleaner local patch would be happy to add a more readable variant -- above is just explaining why I think it does work even if is un-aesthetic :) )