Closed Bug 1187420 Opened 4 years ago Closed 4 years ago

MSan use-of-uninitialized-value jdhuff.c:668 in decode_mcu_fast

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 + fixed
firefox-esr38 46+ fixed
firefox-esr45 46+ fixed

People

(Reporter: tsmith, Assigned: tnikkel)

References

Details

(Keywords: csectype-uninitialized, sec-high, testcase, Whiteboard: [adv-main46+][adv-esr38.8+][adv-esr45.1+])

Attachments

(5 files)

Attached file call_stack.txt
Found while using djpeg as a fuzzing target with afl.
Attached image test_case.jpg
(In reply to Tyson Smith [:tsmith] from comment #0)
> Created attachment 8638678 [details]
> call_stack.txt
> 
> Found while using djpeg as a fuzzing target with afl.

I am using version 1.4.0 of libjpeg-turbo.
Seth, could you figure out what to do with this bug? Thanks.
Flags: needinfo?(seth)
Group: core-security → gfx-core-security
Please check w/valgrind or equivalent in Firefox to see if this code is actually used.
Flags: needinfo?(twsmith)
Attached file valgrind_log.txt
I can reproduce this on m-c.
Flags: needinfo?(twsmith)
I'll just mark this incomplete given that Tyson can't reproduce the issue.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(seth)
Resolution: --- → INCOMPLETE
Tyson points out that I misread his last comment, so I'm reopening...
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Flags: needinfo?(seth)
Maybe you could look at this, Timothy? Thanks.
Flags: needinfo?(seth) → needinfo?(tnikkel)
Blocks: 963907
Flags: needinfo?(tnikkel)
Attached patch patchSplinter Review
Commit msg provides explanation.

One thing I don't understand is that if I force the slow huffman decoder in decode_mcu we don't get this problem. In fact is accesses different locations completely of the huffval array. I don't have the patience to pull apart two huffman decoders to understand why. It's possible the fast (or less likely, the slow) huffman decoder has a bug.

This is a bug in libjpeg-turbo, so we need to notify the maintainer, and whoever else uses it (Chrome?). I don't know how to go about that, anyone?

The commit msg obviously needs to be mostly stripped before landing.
Assignee: nobody → tnikkel
Flags: needinfo?(dveditz)
Attachment #8726005 - Flags: review?(jmuizelaar)
Attachment #8726005 - Flags: review?(dcommander)
I'm the maintainer of libjpeg-turbo, so consider me informed.  :)  This isn't reproducible with the latest release (1.5 beta1, which just landed this week), and apparently this fix for an unrelated security issue:

https://github.com/libjpeg-turbo/libjpeg-turbo/commit/2d56acb84083ba17cc26db2eb9c2616db0feb9bc
(that is a merge commit, so refer to https://github.com/libjpeg-turbo/libjpeg-turbo/commit/0463f7c9aad060fcd56e98d025ce16185279e2bc for description)

also fixed this issue.  But your patch seems innocuous, so I wouldn't be averse to checking it in (belt and suspenders.)
(In reply to DRC from comment #10)
> I'm the maintainer of libjpeg-turbo, so consider me informed.  :)  This
> isn't reproducible with the latest release (1.5 beta1, which just landed
> this week), and apparently this fix for an unrelated security issue:
> 
> https://github.com/libjpeg-turbo/libjpeg-turbo/commit/
> 2d56acb84083ba17cc26db2eb9c2616db0feb9bc
> (that is a merge commit, so refer to
> https://github.com/libjpeg-turbo/libjpeg-turbo/commit/
> 0463f7c9aad060fcd56e98d025ce16185279e2bc for description)
> 
> also fixed this issue.  But your patch seems innocuous, so I wouldn't be
> averse to checking it in (belt and suspenders.)

Interesting, I didn't know about the commit. It looks like it fixes this bug because it forces the slow huffman decoder for short files. If we had a longer file, couldn't we still hit the same problem using the fast huffman decoder?
Noel, you might be interested in this.
Thanks Timothy.  I filed Chromium security issue https://crbug.com/591927 to track this issue.
Flags: needinfo?(dveditz)
> Interesting, I didn't know about the commit. It looks like it fixes this bug
> because it forces the slow huffman decoder for short files. If we had a
> longer file, couldn't we still hit the same problem using the fast huffman
> decoder?

This fix was prompted by a Cure53 security report.  Basically, the report found that it was possible to generate "worst-case" Huffman blocks of 430-440 bytes, so the Huffman decode buffer in decode_mcu_fast() was extended to 512 bytes to accommodate this worst case.  But you are correct that that seems to only accidentally fix this, and only because the test image is so small.  If you were to extend the image so that the error is encountered before reaching the last 512 bytes of the file, then this new bug should still trigger.  Can you create such an image?
Attached image test_case_bigger.jpg
(In reply to DRC from comment #14)
> extended to 512 bytes to accommodate this worst case.  But you are correct
> that that seems to only accidentally fix this, and only because the test
> image is so small.  If you were to extend the image so that the error is
> encountered before reaching the last 512 bytes of the file, then this new
> bug should still trigger.  Can you create such an image?

Yes, here is a file that reproduces the same problem with the BUFSIZE increase patch. I just copied some FF DA segments from the file and appended them to the end.
Comment on attachment 8726005 [details] [diff] [review]
patch

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

Looks good to me.
Attachment #8726005 - Flags: review?(dcommander) → review+
The proposed patch has been integrated into my local sandbox.  Let me know when everyone has signed off on it and when I should push it to the public libjpeg-turbo repo.
Attachment #8726005 - Flags: review?(jmuizelaar) → review+
Added Timothy to chrome sec-bug https://crbug.com/591927 (let me know if you can't see it).  Analysis there suggests the patch looks fine, but it doesn't apply to Chrome since we don't use jsthuff.c at all.  Best we can tell at this time, Chrome is not affected by this bug.
Yes, unless you integrated the code from libjpeg-turbo 1.4.x that allow decoding of motion-JPEG frames, then you won't be affected.
Comment on attachment 8726005 [details] [diff] [review]
patch

Not sure what the procedure is for coordinate with libjpeg-turbo landing. I guess we decide when we land this and then libjpeg-turbo lands at the same time?

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

pretty easy to tell that memory in the default hufftable is accessed from the patch. there's still more work to get an exploit, but that's a very big head start.


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

the check-in comment needs to be strip before landing, it was there for reviewing purposes only. 

As I said in the first question, it's pretty hard to hide the flaw based on the patch. So definitely consider that an attacker could construct an exploit once they know about this patch.


Which older supported branches are affected by this flaw?

bug 963907 has milestone mozilla29, so I think everything supported is affected.


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

bug 963907


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

trivial


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

safe, it just zeroes memory
Attachment #8726005 - Flags: sec-approval?
(In reply to Timothy Nikkel (:tnikkel) from comment #20)
> Not sure what the procedure is for coordinate with libjpeg-turbo landing. I
> guess we decide when we land this and then libjpeg-turbo lands at the same
> time?

I understand the desire to avoid publicizing security issues until a fix is available, but in the case of libjpeg-turbo, making the fix available means checking it in publicly.  Most application developers will neither be aware of the issue nor willing to apply the fix until the fix is pushed to our public repository.  Builds of Firefox that are supplied by Linux vendors are going to rely on the system's version of libjpeg-turbo, so in order for those builds to obtain this fix, the fix has to be checked in publicly first (the O/S integrators can then apply the patch to their system-installed builds of libjpeg-turbo.)

In this case, since the issue is basically a different facet of an existing issue, it seems like it would be pretty easy for someone to stumble upon the issue without us giving them any hints.  Thus, I am personally in favor of pushing the fix to the libjpeg-turbo repository as soon as possible.  Otherwise, we're essentially leaving other applications vulnerable in order to allow you time to shore up Firefox.  If it takes, say, a week to shore up Firefox, then fine.  If it takes months, then not so fine.  libjpeg-turbo 1.5 is in beta right now, so this is the perfect time to introduce bug fixes.
This can't get sec-approval without a security rating. Can you suggest one for this issue?
Flags: needinfo?(tnikkel)
It can leak the contents if uninitialized memory to the web. So whatever rating that is.
Flags: needinfo?(tnikkel) → needinfo?(abillings)
sec-approval+ to go in.
Flags: needinfo?(abillings)
Keywords: sec-high
Comment on attachment 8726005 [details] [diff] [review]
patch

Do we want to backport this to branches?
Attachment #8726005 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #25)
> Comment on attachment 8726005 [details] [diff] [review]
> patch
> 
> Do we want to backport this to branches?

Yes, we want to land it everywhere. Patch is trivial, so porting is trivial.
Comment on attachment 8726005 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: some contents of memory leaked to the web using malicious jpeg
Fix Landed on Version: 48
Risk to taking this patch (and alternatives if risky): safe
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.


Approval Request Comment
[Feature/regressing bug #]: bug 963907
[User impact if declined]: some contents of memory leaked to the web using malicious jpeg
[Describe test coverage new/current, TreeHerder]: none, can't land testcase because it's a security bug
[Risks and why]: safe
[String/UUID change made/needed]: none
Attachment #8726005 - Flags: approval-mozilla-esr45?
Attachment #8726005 - Flags: approval-mozilla-esr38?
Attachment #8726005 - Flags: approval-mozilla-beta?
Attachment #8726005 - Flags: approval-mozilla-aurora?
Patch has been made public now, libjpeg-turbo should land it.
Flags: needinfo?(dcommander)
Landed
Flags: needinfo?(dcommander)
https://hg.mozilla.org/mozilla-central/rev/0ced55976538
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8726005 [details] [diff] [review]
patch

Fix for sec-high issue, let's take this in aurora and beta.
Attachment #8726005 - Flags: approval-mozilla-beta?
Attachment #8726005 - Flags: approval-mozilla-beta+
Attachment #8726005 - Flags: approval-mozilla-aurora?
Attachment #8726005 - Flags: approval-mozilla-aurora+
Group: gfx-core-security → core-security-release
Belatedly tracking for 46
Comment on attachment 8726005 [details] [diff] [review]
patch

Fix a sec-high, taking it.
Should be in 45.1.0 & 38.8.0
Attachment #8726005 - Flags: approval-mozilla-esr45?
Attachment #8726005 - Flags: approval-mozilla-esr45+
Attachment #8726005 - Flags: approval-mozilla-esr38?
Attachment #8726005 - Flags: approval-mozilla-esr38+
Whiteboard: [adv-main46+][adv-esr38.8+][adv-esr45.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.