Bug 14190 - lzxpress compression library bounds checking
Summary: lzxpress compression library bounds checking
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.11.2
Hardware: All All
: P5 critical (vote)
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 14263
  Show dependency tree / graph
 
Reported: 2019-11-07 07:52 UTC by Andrew Bartlett
Modified: 2022-12-02 23:59 UTC (History)
3 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2019-11-07 07:52:41 UTC 
The lzxpress.c compression code in Samba does not implement strong bounds checking.

Found by Andrew Bartlett using the fuzz_lzxpress tool written by Michael Hanselmann against Google's oss-fuzz.

Examples of the missing checks can be seen here in another derivative of the same codebase:

https://github.com/google/rekall/blob/master/contrib/pyxpress/pyxpress.c#L110
Comment 1 Andrew Bartlett 2019-11-07 08:18:31 UTC 
To be clear, I'm not convinced that a client-side 11 (or so) byte over-read triggered by a server that we are calling GetNCChanges on (so will very likely accept passwords from) is worth a security release, but I want to file an embargoed bug for now and we can clear this later.
Comment 2 Andrew Bartlett 2019-11-08 03:16:08 UTC 
While this is an unfortunate flaw, the only code paths to this are those for DRS replication, where in all but quite fancifal use cases it can be expected that we are going to trust the DC for secrets, so a small memory over-read will be the least of our concerns.

Therefore this will not warrant a CVE.

The CVS score would look to me like:

CVSS:3.1/AV:N/AC:H/PR:H/UI:N/S:U/C:N/I:N/A:L (2.2)

I plan to take the patch metze proposed on the list and/or craft my own and fix this normally next week unless there are objections.
Comment 3 Andreas Schneider 2019-11-08 07:56:00 UTC 
Huzaifa,

do you think this is worth a CVE or should we just fix it? We could at leaste fix it with the next security release. There is always one around the corner ;-)
Comment 4 Andrew Bartlett 2019-11-08 08:39:47 UTC 
(In reply to Andreas Schneider from comment #3)
Thanks Andreas and Huzaifa.  I do really value a second opinion here!
Comment 5 Andrew Bartlett 2019-11-21 06:27:14 UTC 
(In reply to Andrew Bartlett from comment #4)
I still don't think this is a CVE, given this has to be a triggered by malicious Domain Controller accessed over authenticated and protected transport (ie, no MITM can do this).
Comment 6 Huzaifa Sidhpurwala 2019-11-21 06:53:04 UTC 
(In reply to Andrew Bartlett from comment #5)
Agree, and if you connect to a malicious domain controller, there is a lot more damage the domain controller can do, then just this bug for instance.
Comment 7 Andrew Bartlett 2019-11-25 20:53:15 UTC 
(In reply to Huzaifa Sidhpurwala from comment #6)
Thanks, removing embargo
Comment 8 Douglas Bagnall 2022-12-02 23:59:55 UTC 
This was fixed in a97c78fb221a2f1aaca2effdb44c51e4f78ddd93 or thereabouts (August 2020 in master).