Closed
Bug 1398021
Opened 7 years ago
Closed 7 years ago
Update lz4 to 1.8.0
Categories
(Core :: MFBT, task)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: tjr, Assigned: RyanVM)
References
Details
(Keywords: csectype-bounds, sec-high, Whiteboard: [third-party-lib-audit][adv-main57-][adv-esr52.7-][post-critsmash-triage])
Attachments
(1 file, 2 obsolete files)
120.33 KB,
patch
|
RyanVM
:
review+
RyanVM
:
approval-mozilla-esr52+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
The lx4 implementation we have in mfbt is version 1.2.0, from 2015. Since then there's been a lot of updates, with many due to fuzzing finding issues in that time period. The latest release is from 8/17/2017 and is 1.8.0 Glen reviewed the commits since our version and noted: In that time, there have been many hundreds of commits and a spot check revealed a number that have security implications. To note, many commits that may have security implications are not marked as such, so if an update or release is relevant to security isn’t easily determined. In this time frame many speed/performance improvements have been made as well. Here is a selection of commits that are likely to be relevant to security and reliability. Looking at: https://github.com/lz4/lz4/commits/dev March 8th 2017: https://github.com/lz4/lz4/commit/66b26a389f210548c49a69983ff4ffd2250bf0d0 looks like fuzzing found something that they fixed. No security note. https://github.com/lz4/lz4/commit/7eecd32c079cdf1afda8c00bba83dd3a1fee3f62 fixed a buffer check in 1.8.0 (not fixed in 1.7.5) Chromium dev submitted this: https://github.com/lz4/lz4/commit/dc868cd5b1bc2c68405732fe080b7b0e5dff0cca not marked as security, but probably is potentially, although maybe hard to exploit. Looked at all git logs for buffer, overflow, fuzz, followed commits after fuzzing. Not great commit messages, but these sort of fixes could have security ramifications and the library should be updated. Example: https://github.com/lz4/lz4/pull/295/commits
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ryanvm
Assignee | ||
Comment 1•7 years ago
|
||
I've got this mostly working (and Try agrees). The only pending issue at the moment is clang Wcomma bustage that I ended up filing upstream: https://github.com/lz4/lz4/issues/398. Frankly, I agree with clang on that one :) Other than that, it's building and passing tests on Try with gcc/msvc builds.
Assignee | ||
Comment 2•7 years ago
|
||
Import of lz4 1.8.0 plus the upstream fix for the Wcomma bustage.
Attachment #8906420 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•7 years ago
|
||
Fix for https://treeherder.mozilla.org/logviewer.html#?job_id=129917460&repo=try. I intend to fold this into the main patch before landing. Green on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a01a6c8053a18eff61d1be657f53a2dc96a0c92 I'm not sure about the relative severity of this bug. Can this update ride the 57 train or should we be considering it for backport too? I guess this would be sec-audit or sec-other as far as security ratings go?
Attachment #8906421 -
Flags: review?(nfroyd)
Comment 4•7 years ago
|
||
That's a pretty big change to digest in the week before we do RC builds. I'd be happy to bump it to Aurora if we still had that, but it's not going to get much beta testing before release if we land it in 56.
Group: core-security → core-security-release
Comment 5•7 years ago
|
||
Let's call this "sec-high" based on the various commits Tom found (e.g. 7eecd32c079cdf1afda8c00bba83dd3a1fee3f62). That means we'll want to land this on ESR52 after we ship 56.
status-firefox55:
--- → wontfix
status-firefox56:
--- → wontfix
status-firefox57:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox56:
--- → -
tracking-firefox57:
--- → +
tracking-firefox-esr52:
--- → 57+
Comment 6•7 years ago
|
||
Comment on attachment 8906421 [details] [diff] [review] Windows Werror fix Review of attachment 8906421 [details] [diff] [review]: ----------------------------------------------------------------- I think we want to consider this for backporting everywhere; the backports should not be difficult, given that we haven't been actively updating this code anyway, AFAIK. Ah, I see dveditz feels the same way, that's good.
Attachment #8906421 -
Flags: review?(nfroyd) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8906420 [details] [diff] [review] update lz4 to version 1.8.0 Review of attachment 8906420 [details] [diff] [review]: ----------------------------------------------------------------- rs=me
Attachment #8906420 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 8•7 years ago
|
||
> How easily could an exploit be constructed based on the patch? Not sure how web-exposed our lz4 code is. The issues are already in the open given the upstream commits, so presumably the damage is already done, though. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Commit message is a generic "Update to version 1.8.0" one. > Which older supported branches are affected by this flaw? All > If not all supported branches, which bug introduced the flaw? N/A > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Applies cleanly to ESR52. As mentioned by Dan, we'll want this to ride the 57/52.5 train, though. > How likely is this patch to cause regressions; how much testing does it need? It passes on Try across all platforms and I see that we have xpcshell tests for our lz4 code. I really don't know what testing this code needs beyond that. I know at least our Telemetry and Sync code uses lz4 compression, so some bake time on Trunk probably wouldn't hurt.
Attachment #8906420 -
Attachment is obsolete: true
Attachment #8906421 -
Attachment is obsolete: true
Attachment #8906631 -
Flags: sec-approval?
Attachment #8906631 -
Flags: review+
Comment 9•7 years ago
|
||
Comment on attachment 8906631 [details] [diff] [review] update lz4 to version 1.8.0 (folded-up) sec-approval for trunk only.
Attachment #8906631 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 10•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4077eda5d8
Comment 11•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9a4077eda5d8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Updated•7 years ago
|
Updated•7 years ago
|
Whiteboard: [third-party-lib-audit] → [third-party-lib-audit][adv-main57-][adv-esr52.5-]
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [third-party-lib-audit][adv-main57-][adv-esr52.5-] → [third-party-lib-audit][adv-main57-][adv-esr52.5-][post-critsmash-triage]
Assignee | ||
Comment 12•6 years ago
|
||
Looks like the esr52 status was accidentally set to fixed when this was merged to m-c.
Assignee | ||
Comment 13•6 years ago
|
||
Comment on attachment 8906631 [details] [diff] [review] update lz4 to version 1.8.0 (folded-up) I discussed this with Al on IRC. Taking it for ESR 52.7.0 since it was supposed to have shipped in 52.5.0 alongside Fx57 :(
Attachment #8906631 -
Flags: approval-mozilla-esr52+
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 14•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/28855df568d8 I folded the fix for bug 1399412 into the ESR52 uplift as well.
Updated•6 years ago
|
Whiteboard: [third-party-lib-audit][adv-main57-][adv-esr52.5-][post-critsmash-triage] → [third-party-lib-audit][adv-main57-][adv-esr52.7-][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
Updated•5 years ago
|
Type: defect → task
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•