1031414 - Possible security issue in LZ4

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Justin Dolske [:Dolske]]

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.

Comment 1

[Justin Dolske [:Dolske]]

(FTR, this landed in bug 888658)

Comment 2

[Till Schneidereit [:till]]

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.

Comment 3

[Daniel Veditz [:dveditz]]

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.

Comment 4

[Daniel Veditz [:dveditz]]

Unhiding the bug, this is potentially a good mentored or first bug

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

It is exploitable, apparently.

http://blog.securitymouse.com/2014/07/so-i-guess-this-happened.html

Comment 7

[Till Schneidereit [:till]]

Attachment: Update LZ4

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.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(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?

Comment 10

[Till Schneidereit [:till]]

(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 11

[Till Schneidereit [:till]]

Comment on attachment 8453740 [details] [diff] [review]
Update LZ4

Tagging Ms2ger for review, too, to speed things up.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Can you get me a patch that doesn't remove carriage returns from every line?

Comment 13

[Daniel Veditz [:dveditz]]

(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 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

[Till Schneidereit [:till]]

Attachment: Update LZ4 to r119, diff -w patch

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.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8454396 [details] [diff] [review]
Update LZ4 to r119, diff -w patch

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

rs=me

Comment 17

[Till Schneidereit [:till]]

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

Comment 18

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f98f0f5ba507
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c9c94dc9d75
https://hg.mozilla.org/releases/mozilla-beta/rev/1cac0c43fa67

Going out on a limb that we don't want to bother taking this on b2g28 (v1.3) at this point, but feel free to un-wontfix if that's not the case. Nominating for v1.4 blocking status anyway.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f98f0f5ba507

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] for verification purposes, based on comments and lack of concrete exploit to test against.

Comment 22

[Preeti Raghunath(:Preeti)]

Paul

Not sure if FxOS is impacted here. Do you recommend taking it in 1.4?

Comment 23

[Daniel Veditz [:dveditz]]

I don't believe we need this on 1.4/b2g30.

Comment 24

[Preeti Raghunath(:Preeti)]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

It already landed on Aurora in comment 19.