From b53177a36853e265943fb01159da0fa99ebd430d Mon Sep 17 00:00:00 2001 From: oyvindln Date: Sun, 28 Jan 2018 15:46:25 +0100 Subject: [PATCH] Add fuzz target for decompression and fix invalid code len bug Fuzz test is only for non-wrapping buffers for now Some invalid data resulted in 0-length codes being decoded, which resulted in an infinite loop causing OOM. --- Cargo_fuzz.toml | 26 ----- fuzz/Cargo.toml | 6 ++ fuzz/fuzz_targets/fuzz_high.rs | 0 fuzz/fuzz_targets/inflate_nonwrapping.rs | 8 ++ miniz_oxide/src/inflate/core.rs | 94 +++++++++++------- miniz_oxide/tests/test.rs | 6 ++ .../tests/test_data/invalid_code_len_oom | Bin 0 -> 32941 bytes 7 files changed, 80 insertions(+), 60 deletions(-) delete mode 100644 Cargo_fuzz.toml mode change 100644 => 100755 fuzz/fuzz_targets/fuzz_high.rs create mode 100755 fuzz/fuzz_targets/inflate_nonwrapping.rs create mode 100644 miniz_oxide/tests/test_data/invalid_code_len_oom diff --git a/Cargo_fuzz.toml b/Cargo_fuzz.toml deleted file mode 100644 index a9af3827..00000000 --- a/Cargo_fuzz.toml +++ /dev/null @@ -1,26 +0,0 @@ -[package] -name = "miniz_oxide" -version = "0.1.0" -build = "src/build.rs" -links = "miniz" -license = "MIT" -authors = ["Frommi "] - -[workspace] - -[lib] -name = "miniz_oxide" -crate-type = ['rlib'] - -[dependencies] -libc="0.2.22" - -[build-dependencies] -gcc = "0.3" - -[features] -default = [] -fuzzing = [] - -[profile.dev] -panic = "abort" diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index f1c51c06..39667d57 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -13,6 +13,8 @@ libc="0.2.22" [dependencies.miniz_oxide_c_api] path = ".." +[dependencies.miniz_oxide] +path = "../miniz_oxide" [dependencies.libfuzzer-sys] git = "https://github.com/rust-fuzz/libfuzzer-sys.git" @@ -28,3 +30,7 @@ path = "fuzz_targets/fuzz_high.rs" default = ["fuzzing"] fuzzing = ["miniz_oxide_c_api/fuzzing"] + +[[bin]] +name = "inflate_nonwrapping" +path = "fuzz_targets/inflate_nonwrapping.rs" diff --git a/fuzz/fuzz_targets/fuzz_high.rs b/fuzz/fuzz_targets/fuzz_high.rs old mode 100644 new mode 100755 diff --git a/fuzz/fuzz_targets/inflate_nonwrapping.rs b/fuzz/fuzz_targets/inflate_nonwrapping.rs new file mode 100755 index 00000000..38f9fd59 --- /dev/null +++ b/fuzz/fuzz_targets/inflate_nonwrapping.rs @@ -0,0 +1,8 @@ +#![no_main] +#[macro_use] extern crate libfuzzer_sys; +extern crate miniz_oxide; + +fuzz_target!(|data: &[u8]| { + // fuzzed code goes here + let _ = miniz_oxide::inflate::decompress_to_vec(data); +}); diff --git a/miniz_oxide/src/inflate/core.rs b/miniz_oxide/src/inflate/core.rs index 6978c300..b78b5313 100644 --- a/miniz_oxide/src/inflate/core.rs +++ b/miniz_oxide/src/inflate/core.rs @@ -63,13 +63,24 @@ impl HuffmanTable { #[inline] /// Look up a symbol and code length from the bits in the provided bit buffer. - fn lookup(&self, bit_buf: BitBuffer) -> (i32, u32) { + /// + /// Returns Some(symbol, length) on success, + /// None if the length is 0. + /// + /// It's possible we could avoid checking for 0 if we can guarantee a sane table. + /// TODO: Check if a smaller type for code_len helps performance. + fn lookup(&self, bit_buf: BitBuffer) -> Option<(i32, u32)> { let symbol = self.fast_lookup(bit_buf).into(); if symbol >= 0 { - (symbol, (symbol >> 9) as u32) + if (symbol >> 9) as u32 != 0 { + Some((symbol, (symbol >> 9) as u32)) + } else { + // Zero-length code. + None + } } else { // We didn't get a symbol from the fast lookup table, so check the tree instead. - self.tree_lookup(symbol.into(), bit_buf, FAST_LOOKUP_BITS.into()) + Some(self.tree_lookup(symbol.into(), bit_buf, FAST_LOOKUP_BITS.into())) } } } @@ -253,6 +264,7 @@ enum State { BadCodeSizeDistPrevLookup, InvalidLitlen, InvalidDist, + InvalidCodeLen, } impl State { @@ -779,7 +791,7 @@ fn decompress_fast( fill_bit_buffer(&mut l, &mut in_iter); - let (symbol, code_len) = r.tables[LITLEN_TABLE].lookup(l.bit_buf); + if let Some((symbol, code_len)) = r.tables[LITLEN_TABLE].lookup(l.bit_buf) { l.counter = symbol as u32; l.bit_buf >>= code_len; @@ -795,22 +807,29 @@ fn decompress_fast( fill_bit_buffer(&mut l, &mut in_iter); } - let (symbol, code_len) = r.tables[LITLEN_TABLE].lookup(l.bit_buf); - - l.bit_buf >>= code_len; - l.num_bits -= code_len; - // The previous symbol was a literal, so write it directly and check - // the next one. - out_buf.write_byte(l.counter as u8); - if (symbol & 256) != 0 { - l.counter = symbol as u32; - // The symbol is a length value. - break; + if let Some((symbol, code_len)) = r.tables[LITLEN_TABLE].lookup(l.bit_buf) { + l.bit_buf >>= code_len; + l.num_bits -= code_len; + // The previous symbol was a literal, so write it directly and check + // the next one. + out_buf.write_byte(l.counter as u8); + if (symbol & 256) != 0 { + l.counter = symbol as u32; + // The symbol is a length value. + break; + } else { + // The symbol is a literal, so write it directly and continue. + out_buf.write_byte(symbol as u8); + } } else { - // The symbol is a literal, so write it directly and continue. - out_buf.write_byte(symbol as u8); + state.begin(InvalidCodeLen); + break 'o TINFLStatus::Failed; } } + } else { + state.begin(InvalidCodeLen); + break 'o TINFLStatus::Failed; + } } state.begin(HuffDecodeOuterLoop1); @@ -1372,7 +1391,7 @@ fn decompress_inner( } else { fill_bit_buffer(&mut l, &mut in_iter); - let (symbol, code_len) = r.tables[LITLEN_TABLE].lookup(l.bit_buf); + if let Some((symbol, code_len)) = r.tables[LITLEN_TABLE].lookup(l.bit_buf) { l.counter = symbol as u32; l.bit_buf >>= code_len; @@ -1388,23 +1407,29 @@ fn decompress_inner( fill_bit_buffer(&mut l, &mut in_iter); } - let (symbol, code_len) = r.tables[LITLEN_TABLE].lookup(l.bit_buf); - - l.bit_buf >>= code_len; - l.num_bits -= code_len; - // The previous symbol was a literal, so write it directly and check - // the next one. - out_buf.write_byte(l.counter as u8); - if (symbol & 256) != 0 { - l.counter = symbol as u32; - // The symbol is a length value. - Action::Jump(HuffDecodeOuterLoop1) + if let Some((symbol, code_len)) = r.tables[LITLEN_TABLE].lookup(l.bit_buf) { + + l.bit_buf >>= code_len; + l.num_bits -= code_len; + // The previous symbol was a literal, so write it directly and check + // the next one. + out_buf.write_byte(l.counter as u8); + if (symbol & 256) != 0 { + l.counter = symbol as u32; + // The symbol is a length value. + Action::Jump(HuffDecodeOuterLoop1) + } else { + // The symbol is a literal, so write it directly and continue. + out_buf.write_byte(symbol as u8); + Action::None + } } else { - // The symbol is a literal, so write it directly and continue. - out_buf.write_byte(symbol as u8); - Action::None + Action::Jump(InvalidCodeLen) } } + } else { + Action::Jump(InvalidCodeLen) + } } }) } @@ -1651,7 +1676,8 @@ fn decompress_inner( // Anything else indicates failure. // BadZlibHeader | BadRawLength | BlockTypeUnexpected | DistanceOutOfBounds | - // BadTotalSymbols | BadCodeSizeDistPrevLookup | BadCodeSizeSum + // BadTotalSymbols | BadCodeSizeDistPrevLookup | BadCodeSizeSum | InvalidLitlen | + // InvalidDist | InvalidCodeLen _ => break TINFLStatus::Failed, }; }; @@ -1774,7 +1800,7 @@ mod test { } fn masked_lookup(table: &HuffmanTable, bit_buf: BitBuffer) -> (i32, u32) { - let ret = table.lookup(bit_buf); + let ret = table.lookup(bit_buf).unwrap(); (ret.0 & 511, ret.1) } diff --git a/miniz_oxide/tests/test.rs b/miniz_oxide/tests/test.rs index 5549c85d..5cd5796e 100644 --- a/miniz_oxide/tests/test.rs +++ b/miniz_oxide/tests/test.rs @@ -31,6 +31,12 @@ fn inf_issue_19() { let _ = decompress_to_vec(data.as_slice()); } +#[test] +fn decompress_oom() { + let data = get_test_file_data("tests/test_data/invalid_code_len_oom"); + let _ = decompress_to_vec(data.as_slice()); +} + fn get_test_data() -> Vec { use std::env; let path = env::var("TEST_FILE").unwrap_or_else(|_| "../miniz/miniz.c".to_string()); diff --git a/miniz_oxide/tests/test_data/invalid_code_len_oom b/miniz_oxide/tests/test_data/invalid_code_len_oom new file mode 100644 index 0000000000000000000000000000000000000000..3e030474c34ec19c023e5caf0b806f8f1bbd2186 GIT binary patch literal 32941 zcmeI5Wl&s=ddG3uFgQVjJG*Fb*##DN3Bf{eCqU2y5ALoZ1QH1Di+gYhOK^AC5FmI6 zgpl6bPCM&-t()cV?eC=j=Iu`8^kCe>tLt8i@KCc_&6txY@4~o=|_I z;R;)T`7!V4`H4cO$KVQFT@~C1-+5k-wxly5#@HqXx`pexD2E6)Nu49GTSr}=f39t! z!*wRRacGyiNN;>%<3=WFqZ3~LbNJ;bY$k_+NbMu5X`l8xf#Xl~r?8Q;V-$N_)v44@RRqWxf&-&vdRDR9Gur;=iCQV!;+hV;jQ_-SU!T+FDG* z#Pd;RJs50y8EW)XE1hlUf~(e{pa6ZWZ_hkPI64_@J?8MfF8><4#tXVPD^=o`#v7{T zb!kNuGtH6uY81_*Ym*nDOg7q`M72JyOJJ4ONx?a0lNhEB(YQl$wEGON1f(HAa9(>I zJlbIMP*cj#-IRcpMFkO&xkFy5N$>g{t!Xn?VSUF#kaO9*GgbBqhIs(`+>1Ooeo8Eb zT~Rk*{=o>3_eu#0zs=#zQ4*c>!to8V%oJ)J2XR3_sI~oIi_Y>kq~bVr-z_M3Giz#V z)$Y0C0JpVljo9aBqYxLtMyxZrEge7CT%Pb7><@~O&q)&BW@MKKQ)567QS^Hr_d!21 za_c+GXzr?mRsH6m=tOAYteP3GLUP~^D`vf(Zd?Mifms7^up=U#hL5x>#=)*|&AIGu z44OJ9W6tdU`e<89XljJrho9u^g5WM}B!h9&k*K1; zJ9fi7muQ|IsIZ?g*zn zyj@4Vky|p(Tz8OC6p#5m6t+_2GWM?5%XXGDzdZSHmfY$w#)pNXZNNpr}G}078 zI=)U#b=*wrvf1IWKH>D{>b*3Q+=mVFQsv-JcI7ImB!42|-vaS-o zk?$_uMW~IF!n5-)C&FQF`kBZD7cfkLterL6I{nBggL0^NExG?|C|ki*0hs`M;fNbu ze%+XfIm$;R_Uy=QOXe9_-6J~Qj(mN>hGNKx*ZA926_ROD=~rV&kF zK{DfgB-83)OZ*OqniVbuB7;e518fpEAR7S;GIN4)hG`l#1J$ z^vJ&n(LdC{C@YD`y&eK_U?nqo!&X!Ss?x%C9e$K8#JMUf!06G~21hicu#j-c_z&+2 zBb_K0vse=!OE+QdDkgq3y&(rp=?J)bjWTH?6xF-Q3-FoD|s65Q%0EHDScFZ9aMNKrc%) zeltlexf1u_8!VjJ5$6|qGB7lD9kqyjys8EIfz5<`->llkfS;!vca>rA*Ga@y8Nv6A zrnabH$QN3Y7lDHpMtJz=mc$mV=wg>&uKkrh1T%If*&#=(IGPg|AcAkxdv;E13-i2; ztp~XMSs640?g#_1NpWK6v`U550)ZRIb5= zA_JsjTq>$-FSN-pU|+Ypn*|$YjaJ`BJwN_w^t6Tn2U;klBNl?6DFz{Q`S}{0D=yJr zw{AVt1iNG$F-l84>BGGs*E3tsZA5ak9Cd0#P#K~G5iyaTM^>KQPD=|G_mUFlXe$$u zcUmx_eE1?9BMEyEiV_4BLOh_-)SiWJ`!Ry&`Kl2(d%IVWV%mL~$CS?vAPuadq*8K~ zTJy#Nq8Wx|E_Xw61RItGi`jI*vStCPD9JMa-7~Zi1GCB%)+@ejiI%`~ zV`wAZ_R*J~CLhLO?=%GNSk64980^# z8c@pfO2;w?O}TnpI@+Vg1_P(p-XKgJx>I9GpQWo-2$+`yG%_?yXR1T4mq%0i9SR>1 zi&|wdZ@J>ey=%KjF-3dX^G;KWdi1$=&tlj~`;khY58O4YNsVrqb%UayYK&2c;LT5KxPYt(RdoIy$ruM9b1Jg{6tu_BaIiXpY+l zXLIM9M37sjI_sr{NHSQJvlQciS?r&joJDugH*2Bnu@W3Yv#pK!lD+P9%~P$)l8JlR zUv3ySai8Bn%Q0}Ygc7M9F9gigfxYfJt4#HZxN7%HYEi{?>p^W+H_anw<#o?5iKADv zEee86ES=m(F117AA!Xd0pgF698Q}?J{F{0o8CW8zzU~V$EviT6fdMzCb<*7HC)MJX z;RsX-q52epLZ!wqnL_@E=6QP|8)6#6v9Hp1fMVvhn1ptR8p7OEX3v^m$K&Ngl%1iz z{=hv@MFh)~cd3sfY^kt}sqg#wndr{x@sp~@qt3F3fVT zL{`Yjd)zofqpn;c+VvAcr$pdxQn;jTsLRa;QT@F}3Hi2UEB|aZ-A5=w3S|<1;;+3> zwPX)3?)@5gHr$WxiO7&%CX>Wz8FeV#3=wmloDFF3r14-di;!NcDo)M&u<$K>!3ff8 zG1vBvTU)&${lzsiKRhobh|BKK$#RW`F&q*e<*kT@->-57>n3CmB*V-pdb8w5KS%IlGonvB-T9n(|_?D{YXBLO^gLyR$H~QH^+YHB7C`B-0h*CTs zWqob`wVhRYcqI}xyPpIB&A4@4lJ!do|y7^skPIO;P-fjyOI?lA7w5&7z zvA0EveMv;k{PxZ$CxUgUt3zHjQh`Wyq)=_$=Lkz3O)8vTr4S;K5H$O?6DoLMVNEqT z@hkvTC65uw@rrcqgInRGZjb$o&v#!0b)%>B-B&@BjBxrx)6G{ovO(vsonAgYi+1ps zAYkuT*{${D)BJ2Hj6V<;t&?3TPixCd7lUuReonQ0CG8ew8w$nh8}CcL625ABg0P?l zVT{!q)Pc(J1?obkMB%!4QrUwx-@LV9T1{+;Z8TLF;)*WsP)8k}{RJ3Psl<$@suHbN z!?}Sr$NR$1tIJQogS^e&av028v` z5WssF&uZqruT8-EohR8Y41O53xk<);Et(N~RxkfatToMhBFB=P%y5XmIKibI{w7Yyh`VbE@*Tt*qggXya#qf7rqBaph2Lf9mOp(~bM4Qtrtq`cU+%=$#V_==XgV>_uA^g&u`p2CdD%7m>#iH6bMLPo*;EXzJ=v%v0Zi4v&w~RK^?@@kDfdW z&xuhV>K_m{HfGo(#?30U@>$W#gK4>Fz3(}>m@Z|m{O@!$>6-_KI3M0t@HlJoD2ZBw zRU(s}Taq1keRBRcOQd4;yoze&+U2@E{g_l3?ar(VO~+pRhxOU8cXTA zjX{X#BqJJ>MAlr9QfjT8YP~|>-XYO~Rh*Y4!U4ZsvwI}W7g1MPkr{ZhcdC8C&=9!w zUaQiATfbeLUNtC-7Wbgf8~KuyVm^faGMcM$RB$ruY4^=HPi3kS2@N~f9FN{)GP0LT z7CE69Yb=L`hLU~3ul5h#@PVTlo~HR)HN1CCR$XCP*s<#6&Ipn!89gQ}i@Lq4xnZMk z+NYV9wGt?ZQD6zKQHfw@-PiIZh*S=ys-$#>4`?RjVW1?#;E zPsn7@tb!V+dBF;qeArK4`!%? z-uSAsc0+P7hFL1H@oVbeuK(_=N zk8e}oviA(5>q~i*1{S_54>3)5YD`@2UnqKrZGzoCG@J#>4f(L8U+iALuYPngt&9WNHImsHq>w85jpCIc&u@VUA5Nwk}>2p!FblQTLM$@ccb zC(tK!rpu`vs_#E(Cw}3wP`z1kWYsnZuRfh(VKzxC!s#nE$%%5AZaJkR!;~b9l<(Y_C;KoL`K7KOVh2<>kJSdm+xK8DoIxEXRIu{orsFxBb z$RO8E|IM&p&&{}->c^Pj`)<`bQE{aga3#ZQ*a`oQLDZUX%vc{8A}C6RMrf#wiJh9?pv%F`NnCLhHN(HfI9?mhX9NS21JC}%4ZOdC41eVUHqf60u!FxWXc1^f0qrQD9rZhsOMi!U)ISqF{t8U|cT^7m z==!T6Z2v9=>tC9Nz$Y*PU;@AdfC&H-_=6|#Zxr8O4havusr)`~Dgeb7p!fn5Ux4BZ zP<#Q3FF^4HD82y27ohk8d%}L(p0MAT3