zstd crashes on decoding invalid archives #11

Closed
magv opened this Issue Jan 30, 2015 · 10 comments

Comments

Projects
None yet
3 participants
@magv

magv commented Jan 30, 2015

Hi. It seems that zstd will read illegal pointers and crash when presented with mangled archives. Here's one such example file (GitHub doesn't allow binary attachments, so I'm providing a hex dump):

0000000    fd  2f  b5  1c  00  00  1c  40  00  12  31  32  31  31  31  31
0000020    31  31  31  31  32  32  32  32  32  32  32  0a  10  98  00  ff
0000040    7f  00  84  c0  00  00

Here's what gdb has to say about this problem:

(gdb) run -d <example.zst >example
Starting program: zstd -d <example.zst >example

Program received signal SIGSEGV, Segmentation fault.
0x0000000000410965 in ZSTD_decompressBlock (srcSize=28, src=0x801011000, maxDstSize=524288, dst=0x801032000, ctx=0x801006000) at lib/zstd.c:1533
(gdb) bt
#0  0x0000000000410965 in ZSTD_decompressBlock (srcSize=28, src=0x801011000, maxDstSize=524288, dst=0x801032000, ctx=0x801006000) at lib/zstd.c:1533
#1  ZSTD_decompressContinue (dctx=0x801006000, dst=0x801032000, maxDstSize=524288, src=0x801011000, srcSize=31) at lib/zstd.c:1680
#2  0x0000000000408681 in FIO_decompressFilename (output_filename=0x410f65 "-",input_filename=0x410f65 "-") at programs/fileio.c:363
#3  0x0000000000401a4d in main (argc=2, argv=0x7fffffffd9d0) at programs/zstdcli.c:314

This is with zstd as of commit 00f9507; the crash is located over here. The problem is that ZSTD_decompressBlock does not validate how big matchLength can get; in this case it is equal to 8650883, while the maxDstSize is only 524288 bytes, which results in an attempt to copy past the end of the output buffer.

@Cyan4973

This comment has been minimized.

Show comment
Hide comment
@Cyan4973

Cyan4973 Jan 30, 2015

Contributor

Correct. (and excellent debugging by the way)
Zstd is not yet protected against invalid input.
This is an objective, obviously.

Part of the work is done, since FSE seems to properly handle invalid input.
Now, Zstd proper must receive the same attention.

Contributor

Cyan4973 commented Jan 30, 2015

Correct. (and excellent debugging by the way)
Zstd is not yet protected against invalid input.
This is an objective, obviously.

Part of the work is done, since FSE seems to properly handle invalid input.
Now, Zstd proper must receive the same attention.

@geeknik

This comment has been minimized.

Show comment
Hide comment
@geeknik

geeknik Jan 31, 2015

I found a variation of this crash that is most likely exploitable by people with malicious intent. Let me know where I can send the details, or I can publish the details here.

geeknik commented Jan 31, 2015

I found a variation of this crash that is most likely exploitable by people with malicious intent. Let me know where I can send the details, or I can publish the details here.

@Cyan4973

This comment has been minimized.

Show comment
Hide comment
@Cyan4973

Cyan4973 Jan 31, 2015

Contributor

Hi Brian
It's just a bit too soon.
Zstd is not yet protected against such scenario, it's that simple.
Investigating specific situations will be useful later on, when protection code will be settled.
I'll keep this issue opened to track progresses.

Contributor

Cyan4973 commented Jan 31, 2015

Hi Brian
It's just a bit too soon.
Zstd is not yet protected against such scenario, it's that simple.
Investigating specific situations will be useful later on, when protection code will be settled.
I'll keep this issue opened to track progresses.

@Cyan4973 Cyan4973 referenced this issue Jul 7, 2015

Closed

Buffer bounds #22

@Cyan4973

This comment has been minimized.

Show comment
Hide comment
@Cyan4973

Cyan4973 Jul 8, 2015

Contributor

#22 contains some interesting testing code initiated by @luben

Contributor

Cyan4973 commented Jul 8, 2015

#22 contains some interesting testing code initiated by @luben

@Cyan4973

This comment has been minimized.

Show comment
Hide comment
@Cyan4973

Cyan4973 Aug 23, 2015

Contributor

Latest version of Zstd is now supposed to be safe against invalid input.

Contributor

Cyan4973 commented Aug 23, 2015

Latest version of Zstd is now supposed to be safe against invalid input.

@Cyan4973 Cyan4973 closed this Aug 23, 2015

@magv

This comment has been minimized.

Show comment
Hide comment
@magv

magv Aug 24, 2015

Not completely though. Here's another example file:

00000000  fd 2f b5 1e 00 00 31 40  00 20 31 32 32 32 32 32
00000010  32 31 33 31 32 33 31 31  31 32 32 32 32 32 33 31
00000020  32 32 31 31 32 32 32 32  32 0a 03 00 20 00 00 00
00000030  00 a1 00 10 00 08 42 10  c0 00 00

If you'll run zstd -d on it (as of commit 79d3b1e), it will segfault with this backtrace:

#0  0x000000000040c9ac in FSE_read64 (memPtr=0x70101103b) at fse.c:202
#1  0x000000000040c8f5 in FSE_readLE64 (memPtr=0x70101103b) at fse.c:277
#2  0x000000000040474b in FSE_readLEST (memPtr=0x70101103b) at fse.c:311
#3  0x0000000000404a1e in FSE_reloadDStream (bitD=0x7fffffffe470) at fse.c:1594
#4  0x0000000000404a7c in FSE_initDState (DStatePtr=0x7fffffffe490, bitD=0x7fffffffe470, dt=0x801006000) at fse.c:1604
#5  0x000000000040a5d9 in ZSTD_decompressSequences (ctx=0x801006000, dst=0x801032000, maxDstSize=524288, seqStart=0x801011023, seqSize=14, litStart=0x801011003 "1222222131231112222231221122222\n\003", litSize=32) at ../lib/zstd.c:1543
#6  0x000000000040a387 in ZSTD_decompressBlock (ctx=0x801006000, dst=0x801032000, maxDstSize=524288, src=0x801011000, srcSize=14) at ../lib/zstd.c:1591
#7  0x000000000040a1df in ZSTD_decompressContinue (dctx=0x801006000, dst=0x801032000, maxDstSize=524288, src=0x801011000, srcSize=49) at ../lib/zstd.c:1734
#8  0x00000000004167d5 in FIO_decompressFilename (output_filename=0x4181d6 "-", input_filename=0x7fffffffec50 "--file-path-here--") at fileio.c:362
#9  0x00000000004174ee in main (argc=3, argv=0x7fffffffe998) at zstdcli.c:334

BTW, I'm using this fuzzer to find these crashes; I fully recommend you to use it too.

magv commented Aug 24, 2015

Not completely though. Here's another example file:

00000000  fd 2f b5 1e 00 00 31 40  00 20 31 32 32 32 32 32
00000010  32 31 33 31 32 33 31 31  31 32 32 32 32 32 33 31
00000020  32 32 31 31 32 32 32 32  32 0a 03 00 20 00 00 00
00000030  00 a1 00 10 00 08 42 10  c0 00 00

If you'll run zstd -d on it (as of commit 79d3b1e), it will segfault with this backtrace:

#0  0x000000000040c9ac in FSE_read64 (memPtr=0x70101103b) at fse.c:202
#1  0x000000000040c8f5 in FSE_readLE64 (memPtr=0x70101103b) at fse.c:277
#2  0x000000000040474b in FSE_readLEST (memPtr=0x70101103b) at fse.c:311
#3  0x0000000000404a1e in FSE_reloadDStream (bitD=0x7fffffffe470) at fse.c:1594
#4  0x0000000000404a7c in FSE_initDState (DStatePtr=0x7fffffffe490, bitD=0x7fffffffe470, dt=0x801006000) at fse.c:1604
#5  0x000000000040a5d9 in ZSTD_decompressSequences (ctx=0x801006000, dst=0x801032000, maxDstSize=524288, seqStart=0x801011023, seqSize=14, litStart=0x801011003 "1222222131231112222231221122222\n\003", litSize=32) at ../lib/zstd.c:1543
#6  0x000000000040a387 in ZSTD_decompressBlock (ctx=0x801006000, dst=0x801032000, maxDstSize=524288, src=0x801011000, srcSize=14) at ../lib/zstd.c:1591
#7  0x000000000040a1df in ZSTD_decompressContinue (dctx=0x801006000, dst=0x801032000, maxDstSize=524288, src=0x801011000, srcSize=49) at ../lib/zstd.c:1734
#8  0x00000000004167d5 in FIO_decompressFilename (output_filename=0x4181d6 "-", input_filename=0x7fffffffec50 "--file-path-here--") at fileio.c:362
#9  0x00000000004174ee in main (argc=3, argv=0x7fffffffe998) at zstdcli.c:334

BTW, I'm using this fuzzer to find these crashes; I fully recommend you to use it too.

@Cyan4973

This comment has been minimized.

Show comment
Hide comment
@Cyan4973

Cyan4973 Aug 24, 2015

Contributor

Thanks @magv, this is exactly the kind of feedback I'm looking for right now.

Thanks also for the link, I did not know this tool, and I'll certainly have a look at it !

Contributor

Cyan4973 commented Aug 24, 2015

Thanks @magv, this is exactly the kind of feedback I'm looking for right now.

Thanks also for the link, I did not know this tool, and I'll certainly have a look at it !

Cyan4973 added a commit that referenced this issue Aug 24, 2015

@Cyan4973

This comment has been minimized.

Show comment
Hide comment
@Cyan4973

Cyan4973 Aug 24, 2015

Contributor

This specific error fixed into fee8e24

Contributor

Cyan4973 commented Aug 24, 2015

This specific error fixed into fee8e24

@Cyan4973

This comment has been minimized.

Show comment
Hide comment
@Cyan4973

Cyan4973 Aug 24, 2015

Contributor

Thanks for the link. AFL is really an excellent fuzzer tool.
It managed to find 2 more bugs that internal fuzzer tools did not triggered yet.
What's more, it did it without knowing the internal data format, just "guessing" the edges, and progressing along promising test cases. This is truly impressive.

After a few hours, it did not found any new bug, so I guess we are starting to see a relatively robust release ...

Contributor

Cyan4973 commented Aug 24, 2015

Thanks for the link. AFL is really an excellent fuzzer tool.
It managed to find 2 more bugs that internal fuzzer tools did not triggered yet.
What's more, it did it without knowing the internal data format, just "guessing" the edges, and progressing along promising test cases. This is truly impressive.

After a few hours, it did not found any new bug, so I guess we are starting to see a relatively robust release ...

@magv

This comment has been minimized.

Show comment
Hide comment
@magv

magv Aug 25, 2015

Yeah, AFL is magical. High yield with low effort? Yes, please.

I also see no crashes in the most recent commit, which is great, but I'd like to leave the fuzzer running for a while longer (a few more days would be good, I think).

magv commented Aug 25, 2015

Yeah, AFL is magical. High yield with low effort? Yes, please.

I also see no crashes in the most recent commit, which is great, but I'd like to leave the fuzzer running for a while longer (a few more days would be good, I think).

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