fix oss-fuzz case 55714 #3476

Cyan4973 · 2023-02-06T00:11:55Z

This issue, discovered by oss-fuzz, can only happen with the following conditions :

Support for legacy format v0.3 enabled (note: support for format 0.3 is disabled by default)
compiled in 32-bit mode (not reproduced in 64-bit mode)

that is to say, this scenario is very unlikely to be reproducible in the wild.
Fixing it nonetheless for completeness.

The fix is trivial, though it's likely going to cost some performance. But at this stage, it's fair to say we don't care about the performance of the deprecated legacy decoder v0.3 anymore. There shouldn't be any data left using this old short lived legacy format.

terrelln · 2023-02-06T20:28:00Z

lib/legacy/zstd_v03.c

@@ -2711,7 +2711,7 @@ static size_t ZSTD_execSequence(BYTE* op,
    if (litEnd > litLimit) return ERROR(corruption_detected);   /* overRead beyond lit buffer */

    /* copy Literals */
-    ZSTD_wildcopy(op, *litPtr, sequence.litLength);   /* note : oLitEnd <= oend-8 : no risk of overwrite beyond oend */
+    ZSTD_memmove(op, *litPtr, sequence.litLength);   /* note : used to be wildCopy, changed to fix a bug in 32-bit mode (oss-fuzz case 55714) */


I don't think this fix is sufficient. We're supposed to have 8 extra bytes in the litBuffer, so this should be valid as long as litEnd <= litLimit.

I think the problem we're running into is that the limit checks should be re-written to avoid overflow. We don't care about speed, so we should do the obviously correct thing.

The offset check also looks janky, so we should clean that up as well.

E.g.

size_t const seqLength = sequence.litLength + sequence.matchLength; if (seqLength > (size_t)(oend - op)) return ERROR(dstSize_tooSmall); if (sequence.litLength > (size_t)(litLimit -*litPtr)) return ERROR(corruption_detected); /* Now we know we don't have any overflow in literal / match lengths, can use the pointer check for oend_8*/ if (oLitEnd > oend_8) return ERROR(dstSize_tooSmall); if (sequence.offset > (U32)(oLitEnd - base)) return ERROR(corruption_detected);

This bug is present in every version of the legacy decoder, so we'll have to fix every one.

terrelln

Just a few small changes left. And I think we also need to add the same checks to zstd_v07.c

terrelln · 2023-02-07T18:04:36Z

lib/legacy/zstd_v01.c

    if (endMatch > oend) return ERROR(dstSize_tooSmall);   /* overwrite beyond dst buffer */
-    if (litEnd > litLimit) return ERROR(corruption_detected);
-    if (sequence.matchLength > (size_t)(*litPtr-op))  return ERROR(dstSize_tooSmall);    /* overwrite literal segment */


I think that we need to keep this check, because the literals are put in the output buffer, but oend isn't reduced.

terrelln · 2023-02-07T18:08:46Z

lib/legacy/zstd_v04.c

+    if (sequence.litLength > (size_t)(litLimit - *litPtr)) return ERROR(corruption_detected);
+    /* Now we know there are no overflow in literal nor match lengths, can use pointer checks */
+    if (oLitEnd > oend_8) return ERROR(dstSize_tooSmall);
+    if (sequence.offset > (U32)(oLitEnd - base)) return ERROR(corruption_detected);


Looks like we have extDict starting in v0.4. But the offset check below looks correct, so we can just delete this starting in this version.

Suggested change

if (sequence.offset > (U32)(oLitEnd - base)) return ERROR(corruption_detected);

terrelln · 2023-02-07T18:10:23Z

lib/legacy/zstd_v05.c

+    if (sequence.litLength > (size_t)(litLimit - *litPtr)) return ERROR(corruption_detected);
+    /* Now we know there are no overflow in literal nor match lengths, can use pointer checks */
+    if (oLitEnd > oend_8) return ERROR(dstSize_tooSmall);
+    if (sequence.offset > (U32)(oLitEnd - base)) return ERROR(corruption_detected);


Suggested change

if (sequence.offset > (U32)(oLitEnd - base)) return ERROR(corruption_detected);

terrelln · 2023-02-07T18:10:51Z

lib/legacy/zstd_v06.c

+    if (sequence.litLength > (size_t)(litLimit - *litPtr)) return ERROR(corruption_detected);
+    /* Now we know there are no overflow in literal nor match lengths, can use pointer checks */
+    if (oLitEnd > oend_8) return ERROR(dstSize_tooSmall);
+    if (sequence.offset > (U32)(oLitEnd - base)) return ERROR(corruption_detected);


Suggested change

if (sequence.offset > (U32)(oLitEnd - base)) return ERROR(corruption_detected);

terrelln · 2023-02-07T20:52:39Z

@Cyan4973 the version compatibility test did catch the bug in v0.5 in the dictionary decompression tests. We don't test v0.4 and earlier in those tests though.

impacts legacy decoder v0.3 in 32-bit mode

in case it would be applicable here too.

slightly different constraints on end of buffer conditions

in case it would be applicable for this legacy version too.

in case it would be applicable for this version too

in case it would applicable for this version

Cyan4973 · 2023-02-07T22:44:22Z

v0.4 legacy decoder and above can be easily tested, using zstd -t.
The update (published above) is now able to decode to decode valid frames.

Below v0.3, this is more complex though, due to the lack of streaming capability.
For these cases, I modified the benchmark module, to trigger the one-pass decompression function and replace the size-query functions.

With this modified benchmark module (not published here), I could verify that the fixes for legacy decoders v0.1, v0.2 and v0.3 are still able to decode valid frames (generated with the corresponding zstd versions).

which uses a different technique to store literals, and therefore must check for potential overwrites.

terrelln · 2023-02-07T22:51:28Z

@Cyan4973 do we also need to fix v0.7?

terrelln · 2023-02-07T22:56:32Z

Otherwise the PR looks good!

Cyan4973 · 2023-02-07T23:04:59Z

@Cyan4973 do we also need to fix v0.7?

Well, it's not clear if that would be useful.
Previous versions v0.4 v0.5 v0.6 have been pre-emptively fixed on the ground of similarities with v0.3, but we are not even sure if there was really a problem.
On the other hand, execSequence in v0.7 looks significantly different.
And it also has been subject to much more intensive fuzzing (than v0.3).

terrelln · 2023-02-08T00:36:28Z

@Cyan4973 I see the same potential 32-bit pointer overflows in v0.7:

zstd/lib/legacy/zstd_v07.c

Lines 3555 to 3556 in df21ace

    
           if ((oLitEnd>oend_w) | (oMatchEnd>oend)) return ERROR(dstSize_tooSmall); /* last match must start at a minimum distance of WILDCOPY_OVERLENGTH from oend */ 
        
           if (iLitEnd > litLimit) return ERROR(corruption_detected);   /* over-read beyond lit buffer */

v0.7 has been subject to much more intensive fuzzing up to now.

We've built all of our OSS-Fuzz fuzzers with all legacy versions since 2019. But we haven't run any 32-bit fuzzers, where this issue could occur, until just now.

terrelln · 2023-02-08T00:53:45Z

we are not even sure if there was really a problem

There is definitely a pointer overflow bug in 32-bit mode. We have a similar fix in dev, but I guess we didn't apply it to all of our legacy decoders:

zstd/lib/decompress/zstd_decompress_block.c

Line 991 in df21ace

(MEM_32bits() && (size_t)(oend - op) < sequenceLength + WILDCOPY_OVERLENGTH)))

The bug in v0.3 is occurring because of a pointer overflow. It happens that v0.3 allows very large literal lengths (in the fuzzed example it is 1965917), which makes it easier to happen. But AFAIK there is no reason that the output or literals pointer cannot be within 128 KB of the end of the address space.

I'm also wondering if we need to guard against the end of the literals buffer being within 128KB of the end of the address space in dev. We can consider that in a separate PR though.

terrelln · 2023-02-08T01:01:42Z

Versions v0.4 and v0.5 definitely have the same bug, because they both allow literal/match lengths up to ~2^24.

v0.6 onwards do cap lengths at ~2^17, so aren't as susceptible, but we should definitely still fix it.

…verflow

Cyan4973 · 2023-02-08T01:12:18Z

OK, checks of v0.7 have been modified in a way which should be independent of address space overflow.

facebook-github-bot added the CLA Signed label Feb 6, 2023

Cyan4973 self-assigned this Feb 6, 2023

terrelln reviewed Feb 6, 2023

View reviewed changes

Cyan4973 force-pushed the fix55714 branch from f1de371 to 4ac760e Compare February 7, 2023 04:21

Cyan4973 mentioned this pull request Feb 7, 2023

return error code when benchmark fails #3480

Merged

terrelln requested changes Feb 7, 2023

View reviewed changes

Cyan4973 and others added 7 commits February 7, 2023 13:55

fix oss-fuzz case 55714

e04706c

impacts legacy decoder v0.3 in 32-bit mode

fix for v0.3 blindly ported to v0.2

cfec005

in case it would be applicable here too.

adapt v0.3 fix to v0.1

7eb4471

slightly different constraints on end of buffer conditions

copy fix for v0.3 to v0.4

b20e4e9

in case it would be applicable for this legacy version too.

port fix for v0.3 to v0.5

7a1a171

in case it would be applicable for this version too

port fix for v0.3 to v0.6

67d7a65

in case it would applicable for this version

fix legacy decoders v0.4, v0.5 and v0.6

9419747

Cyan4973 force-pushed the fix55714 branch from f3e4635 to 9419747 Compare February 7, 2023 22:02

add requested check for legacy decoder v0.1

c5bf6b8

which uses a different technique to store literals, and therefore must check for potential overwrites.

Cyan4973 requested a review from terrelln February 7, 2023 22:49

rewrite legacy v0.7 bound checks to be independent of address space o…

c689310

…verflow

terrelln approved these changes Feb 8, 2023

View reviewed changes

Cyan4973 merged commit 488f7c0 into dev Feb 8, 2023

Cyan4973 deleted the fix55714 branch February 10, 2023 00:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix oss-fuzz case 55714 #3476

fix oss-fuzz case 55714 #3476

Cyan4973 commented Feb 6, 2023 •

edited

Loading

terrelln Feb 6, 2023

terrelln left a comment

terrelln Feb 7, 2023

terrelln Feb 7, 2023

terrelln Feb 7, 2023

terrelln Feb 7, 2023

terrelln commented Feb 7, 2023

Cyan4973 commented Feb 7, 2023

terrelln commented Feb 7, 2023

terrelln commented Feb 7, 2023

Cyan4973 commented Feb 7, 2023 •

edited

Loading

terrelln commented Feb 8, 2023

terrelln commented Feb 8, 2023

terrelln commented Feb 8, 2023 •

edited

Loading

Cyan4973 commented Feb 8, 2023

fix oss-fuzz case 55714 #3476

fix oss-fuzz case 55714 #3476

Conversation

Cyan4973 commented Feb 6, 2023 • edited Loading

terrelln Feb 6, 2023

Choose a reason for hiding this comment

terrelln left a comment

Choose a reason for hiding this comment

terrelln Feb 7, 2023

Choose a reason for hiding this comment

terrelln Feb 7, 2023

Choose a reason for hiding this comment

terrelln Feb 7, 2023

Choose a reason for hiding this comment

terrelln Feb 7, 2023

Choose a reason for hiding this comment

terrelln commented Feb 7, 2023

Cyan4973 commented Feb 7, 2023

terrelln commented Feb 7, 2023

terrelln commented Feb 7, 2023

Cyan4973 commented Feb 7, 2023 • edited Loading

terrelln commented Feb 8, 2023

terrelln commented Feb 8, 2023

terrelln commented Feb 8, 2023 • edited Loading

Cyan4973 commented Feb 8, 2023

Cyan4973 commented Feb 6, 2023 •

edited

Loading

Cyan4973 commented Feb 7, 2023 •

edited

Loading

terrelln commented Feb 8, 2023 •

edited

Loading