Closed
Bug 1187420
Opened 9 years ago
Closed 8 years ago
MSan use-of-uninitialized-value jdhuff.c:668 in decode_mcu_fast
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
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)
1.69 KB,
text/plain
|
Details | |
498 bytes,
image/jpeg
|
Details | |
6.23 KB,
text/plain
|
Details | |
1.54 KB,
patch
|
jrmuizel
:
review+
dcommander
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr38+
Sylvestre
:
approval-mozilla-esr45+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
5.00 KB,
image/jpeg
|
Details |
Found while using djpeg as a fuzzing target with afl.
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Comment 2•9 years ago
|
||
(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.
Comment 3•9 years ago
|
||
Seth, could you figure out what to do with this bug? Thanks.
Flags: needinfo?(seth)
Updated•9 years ago
|
Group: core-security → gfx-core-security
Comment 4•8 years ago
|
||
Please check w/valgrind or equivalent in Firefox to see if this code is actually used.
Flags: needinfo?(twsmith)
Comment 6•8 years ago
|
||
I'll just mark this incomplete given that Tyson can't reproduce the issue.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(seth)
Resolution: --- → INCOMPLETE
Comment 7•8 years ago
|
||
Tyson points out that I misread his last comment, so I'm reopening...
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Updated•8 years ago
|
Flags: needinfo?(seth)
Comment 8•8 years ago
|
||
Maybe you could look at this, Timothy? Thanks.
Flags: needinfo?(seth) → needinfo?(tnikkel)
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8726005 -
Flags: review?(dcommander)
Comment 10•8 years ago
|
||
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.)
Assignee | ||
Comment 11•8 years ago
|
||
(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?
Assignee | ||
Comment 12•8 years ago
|
||
Noel, you might be interested in this.
Comment 13•8 years ago
|
||
Thanks Timothy. I filed Chromium security issue https://crbug.com/591927 to track this issue.
Updated•8 years ago
|
Flags: needinfo?(dveditz)
Comment 14•8 years ago
|
||
> 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?
Assignee | ||
Comment 15•8 years ago
|
||
(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 16•8 years ago
|
||
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+
Comment 17•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8726005 -
Flags: review?(jmuizelaar) → review+
Comment 18•8 years ago
|
||
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.
Comment 19•8 years ago
|
||
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.
Assignee | ||
Comment 20•8 years ago
|
||
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?
Comment 21•8 years ago
|
||
(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.
Comment 22•8 years ago
|
||
This can't get sec-approval without a security rating. Can you suggest one for this issue?
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 23•8 years ago
|
||
It can leak the contents if uninitialized memory to the web. So whatever rating that is.
Flags: needinfo?(tnikkel) → needinfo?(abillings)
Comment 25•8 years ago
|
||
Comment on attachment 8726005 [details] [diff] [review] patch Do we want to backport this to branches?
Attachment #8726005 -
Flags: sec-approval? → sec-approval+
Updated•8 years ago
|
status-firefox42:
affected → ---
status-firefox45:
--- → wontfix
status-firefox46:
--- → affected
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox-esr38:
--- → affected
status-firefox-esr45:
--- → affected
tracking-firefox46:
--- → ?
tracking-firefox47:
--- → ?
tracking-firefox48:
--- → +
tracking-firefox-esr38:
--- → ?
tracking-firefox-esr45:
--- → ?
Assignee | ||
Comment 26•8 years ago
|
||
(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.
Assignee | ||
Comment 27•8 years ago
|
||
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?
Assignee | ||
Comment 28•8 years ago
|
||
Patch has been made public now, libjpeg-turbo should land it.
Flags: needinfo?(dcommander)
Assignee | ||
Comment 29•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ced55976538
Comment 31•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0ced55976538
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 32•8 years ago
|
||
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/d5911159db9e https://hg.mozilla.org/releases/mozilla-beta/rev/0eb4538fbdcf
Updated•8 years ago
|
Group: gfx-core-security → core-security-release
Comment 35•8 years ago
|
||
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+
Updated•8 years ago
|
Updated•8 years ago
|
Whiteboard: [adv-main46+][adv-esr38.8+][adv-esr45.1+]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•