Possible security issue in LZ4

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Dolske, Assigned: till)

Tracking

({sec-moderate})

unspecified
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox30+ wontfix, firefox31+ fixed, firefox32+ fixed, firefox33+ fixed, firefox-esr24 unaffected, b2g-v1.3 wontfix, b2g-v1.3T wontfix, b2g-v1.4 wontfix, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [adv-main31-][qa-] sec-moderate because doom if API misused, but we think we're not.)

Attachments

(2 attachments)

I happened to come across this post today:

http://fastcompression.blogspot.com/2014/06/debunking-lz4-20-years-old-bug-myth.html

Apparently there's a difficult-in-many-cases-to-exploit bug in LZ4. We've got LZ4 in the tree (/mfbt/lz4.c). The header / license author indicates this is the same code/author as the post. Out lz4.c hasn't been touched since it landed in October 2013, so presumably this issue exists.

No idea if it's exploitable as used in Firefox. The blog post implies someone's trying to make a big deal of the bug, if true we should probably fix it promptly just to avoid a flurry of inquiries about it.
(FTR, this landed in bug 888658)
Based on the fastcompression blog post, our use of lz4 is completely fine. If we were using decompress_fast somewhere, then we might have an issue. Since that's not the case, everything's good.

That said, we should look into updating lz4 because the implementation has generally become better.
calling this sec-low because the dangerous calls are present in our tree and /might/ be used by someone in the future; we should fix it.
Keywords: sec-low
Unhiding the bug, this is potentially a good mentored or first bug
Group: core-security
Posted patch Update LZ4Splinter Review
This updates LZ4 to the current version, which includes fixes for the reported issues. It's almost a straight import from https://github.com/Cyan4973/lz4. The only exception is that I removed the "obsolete functions" parts from both header and source file so we don't accidentally use them.

As far as I can tell, it's fully backwards-compatible and a browser builds and runs just fine for me. Not sure how best to test this otherwise. Given that it's all public code, I guess a try run isn't much of a problem?

Also, should we uplift this to everywhere, or try to extract spot-fixes for the identified security issues and uplift those. It seems to me like that'd almost be less safe, as we might introduce issues during that process and, for all I know, Yann might've silently fixed other security issues, too.
Attachment #8453740 - Flags: review?(jwalden+bmo)
Attachment #8453740 - Flags: feedback?(dveditz)
Assignee: nobody → till
Status: NEW → ASSIGNED
Blocks: 1033388
FYI, 31 beta 9 go to build is today. We can always take it in the RC build but, obviously, I would prefer it beta 9.
(In reply to Till Schneidereit [:till] from comment #7)
> As far as I can tell, it's fully backwards-compatible and a browser builds
> and runs just fine for me. Not sure how best to test this otherwise. Given
> that it's all public code, I guess a try run isn't much of a problem?

We have in-tree LZ4 tests, so I would think a green Try run of those would be good. Also, this vulnerability's already pretty widely-known publicly, isn't it?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #9)
> We have in-tree LZ4 tests, so I would think a green Try run of those would
> be good. Also, this vulnerability's already pretty widely-known publicly,
> isn't it?

It is, yes. I just didn't want to jump the gun here, but it seems like a try run should be fine.

I did however run the lz4 tests locally and they pass, so the question is how much that try run would tell us. Then again, there could theoretically be ARM-only issues.
Comment on attachment 8453740 [details] [diff] [review]
Update LZ4

Tagging Ms2ger for review, too, to speed things up.
Attachment #8453740 - Flags: review?(Ms2ger)
Can you get me a patch that doesn't remove carriage returns from every line?
(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> It is exploitable, apparently.
> http://blog.securitymouse.com/2014/07/so-i-guess-this-happened.html

That is a similar (but worse) issue with LZO, and the use of Firefox there is pretty incidental. Any browser that passed the content to MPlayer2 would get the same results.

Good object lesson in not minimizing a researcher's claims, but not proof one way or another that Firefox's use of the library is exploitable. We should just fix it and sleep better at night.
Comment on attachment 8453740 [details] [diff] [review]
Update LZ4

Review of attachment 8453740 [details] [diff] [review]:
-----------------------------------------------------------------

It's hard to see the changes in this diff, is a diff -w possible? That said it's still going to be a fair amount of changes since we're going from upstream r97 (according to bug 888658; have there been updates since?) to r119.

This is not a review, but I'm happy to jump to the bugfixed latest upstream rather than try to cherry-pick the fixes.
Attachment #8453740 - Flags: feedback?(dveditz) → feedback+
Whiteboard: sec-moderate because doom if API misused, but we think we're not.
There are still quite a few irrelevant changes as the commenting style has changed, but it's far more readable.

I don't think it makes too much sense to really do an in-depth review of the changes here: I confirmed that our tests succeed and that the format is backwards-compatible, and the rest seems like it's the same as with every external library we use.
Attachment #8454396 - Flags: review?(Ms2ger)
Comment on attachment 8454396 [details] [diff] [review]
Update LZ4 to r119, diff -w patch

Review of attachment 8454396 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me
Attachment #8454396 - Flags: review?(Ms2ger) → review+
Comment on attachment 8453740 [details] [diff] [review]
Update LZ4

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Unknown. Update of an external library that fixes security holes in that library. Our usage doesn't seem affected by those holes, however, so this is mostly a precautionary measure.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes, as far as there is one.

Which older supported branches are affected by this flaw?

27+

If not all supported branches, which bug introduced the flaw? bug 888658

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The patch should apply cleanly on all branches.

How likely is this patch to cause regressions; how much testing does it need?

Very unlikely, library comes with its own set of tests for backwards compatibility and our usages are very straight-forward.


(Requesting uplift in advance, can obviously be ignored if sec-approval is denied.)

Approval Request Comment
[Feature/regressing bug #]: bug 888658
[User impact if declined]: potential for exploitable security holes 
[Describe test coverage new/current, TBPL]: local testing of the relevant tests
[Risks and why]: low, update to a newer version of an external library that has its own tests for backwards compatibility
[String/UUID change made/needed]: none
Attachment #8453740 - Flags: sec-approval?
Attachment #8453740 - Flags: review?(jwalden+bmo)
Attachment #8453740 - Flags: review?(Ms2ger)
Attachment #8453740 - Flags: review+
Attachment #8453740 - Flags: approval-mozilla-beta?
Attachment #8453740 - Flags: approval-mozilla-aurora?
Comment on attachment 8453740 [details] [diff] [review]
Update LZ4

As a moderate, this doesn't need sec-approval+. Just check it into trunk. 

I think we should take this on Beta and Aurora though so I'm approving. Risk seems low.
Attachment #8453740 - Flags: sec-approval?
Attachment #8453740 - Flags: approval-mozilla-beta?
Attachment #8453740 - Flags: approval-mozilla-beta+
Attachment #8453740 - Flags: approval-mozilla-aurora?
Attachment #8453740 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/f98f0f5ba507
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Marking [qa-] for verification purposes, based on comments and lack of concrete exploit to test against.
Whiteboard: sec-moderate because doom if API misused, but we think we're not. → [qa-] sec-moderate because doom if API misused, but we think we're not.
Whiteboard: [qa-] sec-moderate because doom if API misused, but we think we're not. → [adv-main31-][qa-] sec-moderate because doom if API misused, but we think we're not.
Paul

Not sure if FxOS is impacted here. Do you recommend taking it in 1.4?
Flags: needinfo?(ptheriault)
I don't believe we need this on 1.4/b2g30.
Per Dan's email:
I don't think this is important to backport to b2g30/1.4, We ought to
make sure it's fixed on b2g32/2.0 though.

Hence nom'ing for 2.0
blocking-b2g: 1.4? → 2.0?
It already landed on Aurora in comment 19.
blocking-b2g: 2.0? → ---
Flags: needinfo?(ptheriault)
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.