Created attachment 15763 [details] valgrind output 0 The lzxpress code has a few problems. It is hard to say how many, exactly, as there are duplicate reports. Bugziilla bugs: Bug #14190 is open, unembargoed, and somewhat vague. Bug #14236 relates to this oss-fuzz report: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083&q=samba&can=2 OSS-Fuzz: This looks like the same problem as 20083 above (lzxpress.c, line 257): https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=19382&q=samba&can=2 Private honggfuzz fuzzing (Douglas): I have 8 results, which are all asan issues rather than outright crashes. Valgrind output attached. These may be effective duplicates of each other, or the ones above (though I don't see line 257 mentioned).
Created attachment 15764 [details] valgrind output 1
Created attachment 15765 [details] valgrind output 2
Created attachment 15766 [details] valgrind output 3
Created attachment 15767 [details] valgrind 4
Created attachment 15768 [details] valgrind 5
Created attachment 15769 [details] valgrind 6
Created attachment 15770 [details] valgrind 7
Created attachment 15771 [details] valgrind 8
Created attachment 15772 [details] results and strings tarball (In reply to Douglas Bagnall from comment #0) > I have 8 results I mean 9.
The main problem in #14236 is not related to lzxpress, so I am removing that dependency.
Do you found my wip hardening patches? I think I posted them on the team-list while we discussed some of this.
Created attachment 15774 [details] TODO lzxpress: add bounce checking to lzxpress_decompress()
(In reply to Stefan Metzmacher from comment #12) thanks. I will fuzz with that patch and see what remains.
(In reply to Douglas Bagnall from comment #13) > I will fuzz with that patch and see what remains. Nothing. 15 billion iterations without a crash. I won't look at reviewing it because "TODO", and because I know Andrew has some qualms about it that he needs to resolve. I am now fuzzing the compress function, with 22 crashes in 10 minutes.
(In reply to Douglas Bagnall from comment #14) > I am now fuzzing the compress function, with 22 crashes in 10 minutes. Actually those were all timeouts -- showing it can take > 10 seconds to compress a 12k payload, due it seems to all the nested loops searching for common substrings. Reducing the maximum size to 1000 bytes, I see no problems with compress in 5 million iterations.
(In reply to Douglas Bagnall from comment #14) I would prefer a patch that uses the NDR pull and push functions so we have a common basis for bounds checking, but if a non-WIP patch is proposed that handles the issues I won't object. There is much to do here, and I won't let perfect be the enemy of the good.
This will be made public by oss-fuzz in a few days. metze's TODO patch addresses the detected issues. what is still TODO here?
(In reply to Douglas Bagnall from comment #17) I guess a better commit message and a bug reference? And maybe specific tests? I'm not sure a rewrite to ndr functions is needed.
Created attachment 16164 [details] Metze's patch with BUG:, s-o-b, RB+, sans TODO. How about this. https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=22667 will be revealed on 2020-08-26.
Created attachment 16166 [details] patch that seems to work at -O3 The patch failed the -O3 CI builds: UNEXPECTED(failure): samba4.local.compression.lzxpress(none) REASON: Exception: Exception: ../../lib/compression/testsuite.c:61: c_size was -1 (0xFFFFFFFF), expected 38 (0x26): fixed lzxpress_decompress size This version works for me locally. +#define __CHECK_BYTES(__size, __index, __needed) do { \ + uint32_t __req_size = __index + __needed; \ + if (unlikely(__req_size < __index || \ + __req_size > __size)) { \ + return -1; \ + } \ +} while(0)
There was nothing special with -O3 if (unlikely(__needed < __avail)) { was just wrong and should be if (unlikely(__needed > __avail)) { I'll push the fixed version to the merge request
I've removed the embargo, the issues found were fixed publicly without the need for a security release.
fixed in some of these commits: $ git lol aab5034a9ddc57ee9ce14ce584e53bd9b96b7a58^..637e7cbdbab6a5229b51954f506e51c677739ce8 -- lib/compression * 637e7cbdbab lzxpress: compress shortcut if we've reached maximum length * 04309bc6824 lzxpress/test: time performance of long boring sequences * 505d2879fa8 compression:tests: align test names with functions * 05c760165bf compression: add a few comments, including MS-XCA pointers. * 383a7cfed98 compression: remove always false constant comparison * e36cb10b162 compression: lzxpress decompress empty string as empty string * 1ca44492941 compression: fix lzxpress decompress with trailing flags * d8a90d2a8fc compression:tests: test lzxpress in some edge cases * 075df819cce compression: Move maximum length calculation out of inner loop * 877f007f32d compression: Use correct values for max len and offset * fe5fa7e1974 compression: Replace divisions with shifts * 131eb752699 compression: Remove unneeded loop variable * 5b1f8ea8d3e compression: Reduce scope of variables * 1a964210d24 compression: Use PUSH_LE_U32 for first output buffer write * 41b88d35ce6 compression: Add bounds check for first output buffer write * 0c813ee5637 compression: Remove helper variables str1 and str2 * 430bcd7a083 compression: Fix writing output flags * bb9115e023b compression: Remove byte_left variable * 417e0c914fd compression: Remove redundant bounds check * 6f3f1ba5b4d compression: Add range check for indic_pos * b62fbc4a535 compression: Remove redundant nibble_index check * 52982c01a59 compression: Make use of PUSH_LE_Uxx macros * f2ea8d4c056 compression: Simplify code by making indic_pos an index * b1534457982 compression: Make use of CHECK_{IN,OUT}PUT_BYTES macros * ea42717ccae compression: Simplify code by removing metadata_size variable * 69244b52ed4 compression: Use correct value for indic_pos * 7fab9f90e8a compression: Use correct value for nibble_index * f8feac11cbb compression: Simplify redundant branches * d368fa61cfc compression: Consistently use PUSH_LE_Uxx macros * 9516b268458 compression: Use explicit data sizes * eb7f139dec0 compression tests: Add additional compression tests * 3c2f1f03c19 compression: fix lzxpress-compress * 8f7fbc5c8fd compression: lzxpress_compress: fix no-op shift of 0 * a8fb45247ba compression: fix lzxpress_decompress * f67ff611e96 compression tests: add test for legacy compressed data * 4bcdc3bf30a compression tests: add LZXpress tests based on [MS-XCA] * 0c461f3bd58 lzxpress: avoid technically undefined shift * a97c78fb221 lzxpress: add bounds checking to lzxpress_decompress() * aab5034a9dd lib:compression: Fix undefined behavior in lzxpress