Bug 14263 - lzxpress triage meta-bug
Summary: lzxpress triage meta-bug
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on: 14190
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-04 21:17 UTC by Douglas Bagnall
Modified: 2020-11-11 08:47 UTC (History)
3 users (show)

See Also:


Attachments
valgrind output 0 (3.11 KB, text/plain)
2020-02-04 21:17 UTC, Douglas Bagnall
no flags Details
valgrind output 1 (2.51 KB, text/plain)
2020-02-04 21:17 UTC, Douglas Bagnall
no flags Details
valgrind output 2 (2.51 KB, text/plain)
2020-02-04 21:19 UTC, Douglas Bagnall
no flags Details
valgrind output 3 (2.51 KB, text/plain)
2020-02-04 21:19 UTC, Douglas Bagnall
no flags Details
valgrind 4 (2.55 KB, text/plain)
2020-02-04 21:20 UTC, Douglas Bagnall
no flags Details
valgrind 5 (989 bytes, text/plain)
2020-02-04 21:20 UTC, Douglas Bagnall
no flags Details
valgrind 6 (4.05 KB, text/plain)
2020-02-04 21:21 UTC, Douglas Bagnall
no flags Details
valgrind 7 (991 bytes, text/plain)
2020-02-04 21:21 UTC, Douglas Bagnall
no flags Details
valgrind 8 (2.52 KB, text/plain)
2020-02-04 21:22 UTC, Douglas Bagnall
no flags Details
results and strings tarball (74.06 KB, application/gzip)
2020-02-04 21:32 UTC, Douglas Bagnall
no flags Details
TODO lzxpress: add bounce checking to lzxpress_decompress() (2.79 KB, patch)
2020-02-05 12:36 UTC, Stefan Metzmacher
no flags Details
Metze's patch with BUG:, s-o-b, RB+, sans TODO. (3.31 KB, patch)
2020-08-06 22:19 UTC, Douglas Bagnall
metze: review+
Details
patch that seems to work at -O3 (3.29 KB, patch)
2020-08-08 00:21 UTC, Douglas Bagnall
dbagnall: review? (metze)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Douglas Bagnall 2020-02-04 21:17:00 UTC
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).
Comment 1 Douglas Bagnall 2020-02-04 21:17:45 UTC
Created attachment 15764 [details]
valgrind output 1
Comment 2 Douglas Bagnall 2020-02-04 21:19:29 UTC
Created attachment 15765 [details]
valgrind output 2
Comment 3 Douglas Bagnall 2020-02-04 21:19:54 UTC
Created attachment 15766 [details]
valgrind output 3
Comment 4 Douglas Bagnall 2020-02-04 21:20:25 UTC
Created attachment 15767 [details]
valgrind 4
Comment 5 Douglas Bagnall 2020-02-04 21:20:57 UTC
Created attachment 15768 [details]
valgrind 5
Comment 6 Douglas Bagnall 2020-02-04 21:21:27 UTC
Created attachment 15769 [details]
valgrind 6
Comment 7 Douglas Bagnall 2020-02-04 21:21:57 UTC
Created attachment 15770 [details]
valgrind 7
Comment 8 Douglas Bagnall 2020-02-04 21:22:16 UTC
Created attachment 15771 [details]
valgrind 8
Comment 9 Douglas Bagnall 2020-02-04 21:32:54 UTC
Created attachment 15772 [details]
results and strings tarball

(In reply to Douglas Bagnall from comment #0)
> I have 8 results

I mean 9.
Comment 10 Douglas Bagnall 2020-02-04 22:17:04 UTC
The main problem in #14236 is not related to lzxpress, so I am removing that dependency.
Comment 11 Stefan Metzmacher 2020-02-05 12:31:48 UTC
Do you found my wip hardening patches? I think I posted them on
the team-list while we discussed some of this.
Comment 12 Stefan Metzmacher 2020-02-05 12:36:04 UTC
Created attachment 15774 [details]
TODO lzxpress: add bounce checking to lzxpress_decompress()
Comment 13 Douglas Bagnall 2020-02-07 02:02:29 UTC
(In reply to Stefan Metzmacher from comment #12)
thanks. I will fuzz with that patch and see what remains.
Comment 14 Douglas Bagnall 2020-02-08 21:23:37 UTC
(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.
Comment 15 Douglas Bagnall 2020-02-09 21:33:00 UTC
(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.
Comment 16 Andrew Bartlett 2020-02-09 23:43:06 UTC
(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.
Comment 17 Douglas Bagnall 2020-03-06 02:42:45 UTC
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?
Comment 18 Stefan Metzmacher 2020-03-09 14:53:00 UTC
(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.
Comment 19 Douglas Bagnall 2020-08-06 22:19:43 UTC
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.
Comment 20 Douglas Bagnall 2020-08-08 00:21:01 UTC
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)
Comment 21 Stefan Metzmacher 2020-08-08 09:29:37 UTC
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
Comment 22 Andrew Bartlett 2020-11-11 08:47:28 UTC
I've removed the embargo, the issues found were fixed publicly without the need for a security release.