Last Comment Bug 782141 - (CVE-2012-3969) Heap-buffer-overflow in nsSVGFEMorphologyElement::Filter
(CVE-2012-3969)
: Heap-buffer-overflow in nsSVGFEMorphologyElement::Filter
Status: VERIFIED FIXED
[advisory-tracking+][qa-][asan]
: sec-critical, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 17 Branch
: x86_64 All
: -- normal (vote)
: mozilla17
Assigned To: Jonathan Watt [:jwatt]
:
Mentors:
Depends on:
Blocks: 734079
  Show dependency treegraph
 
Reported: 2012-08-12 12:05 PDT by Arthur Gerkis
Modified: 2014-07-24 13:43 PDT (History)
12 users (show)
rforbes: sec‑bounty+
jwatt: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
verified
verified
15+
fixed


Attachments
test-case triggering the crash (*.zip) (633 bytes, application/octet-stream)
2012-08-12 12:05 PDT, Arthur Gerkis
no flags Details
ASan log (6.02 KB, text/plain)
2012-08-12 12:06 PDT, Arthur Gerkis
no flags Details
patch - avoid the use of PRUint32 (3.40 KB, patch)
2012-08-13 14:46 PDT, Jonathan Watt [:jwatt]
roc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review
crashtest - to be checked in later (954 bytes, patch)
2012-08-14 02:20 PDT, Jonathan Watt [:jwatt]
jwatt: checkin+
Details | Diff | Review
patch for esr10 (4.17 KB, patch)
2012-08-22 15:11 PDT, Jonathan Watt [:jwatt]
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Review

Description Arthur Gerkis 2012-08-12 12:05:20 PDT
Created attachment 651213 [details]
test-case triggering the crash (*.zip)

ASan detected heap-buffer-overflow on opt build f24229bc0ec8.
Comment 1 Arthur Gerkis 2012-08-12 12:06:37 PDT
Created attachment 651214 [details]
ASan log
Comment 2 Jonathan Watt [:jwatt] 2012-08-13 13:48:27 PDT
The problem is at this line:

http://hg.mozilla.org/mozilla-central/annotate/75cdb3f932c6/content/svg/content/src/nsSVGFilters.cpp#l3830


  PRUint32 endX = NS_MIN(x + rx, instance->GetSurfaceWidth() - 1);

When things go bad we have:

  x == 256
  rx == 2147483392
  instance->GetSurfaceWidth() == 308

Since all three of these variables are of type PRInt32, we end up calling NS_MIN<PRInt32>(), but the sum of x and rx is too large to be stored in a PRInt32, so it overflows and the first argument to NS_MIN ends up being negative (-2147483648). That negative argument is then returned by NS_MIN and assigned to the
Comment 3 Jonathan Watt [:jwatt] 2012-08-13 13:55:28 PDT
[hit submit by accident. last paragraph should be the following]

Since all three of these variables are of type PRInt32 we end up calling NS_MIN<PRInt32>(), but the sum of x and rx overflows PRInt32 and the first argument to NS_MIN ends up being negative (-2147483648). That negative argument is then returned by NS_MIN and when assigned to the unsigned variable endX ends up being 2147483648. That causes us to loop over bad memory.
Comment 4 Jonathan Watt [:jwatt] 2012-08-13 14:46:59 PDT
Created attachment 651538 [details] [diff] [review]
patch - avoid the use of PRUint32
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-13 16:05:40 PDT
Comment on attachment 651538 [details] [diff] [review]
patch - avoid the use of PRUint32

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

This looks fine but a) don't check this test in and b) let's clamp the radius to some reasonable value, say 100000. In fact, let's go through all the filter primitives and clamp all their number parameter inputs to reasonable values.
Comment 6 Jonathan Watt [:jwatt] 2012-08-14 02:19:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f41b9ebdba

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> In fact, let's go through all the filter primitives and clamp all their
> number parameter inputs to reasonable values.

I'll do that in a separate bug.
Comment 7 Jonathan Watt [:jwatt] 2012-08-14 02:20:16 PDT
Created attachment 651671 [details] [diff] [review]
crashtest - to be checked in later
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-08-14 17:56:42 PDT
https://hg.mozilla.org/mozilla-central/rev/a2f41b9ebdba
Comment 9 Jonathan Watt [:jwatt] 2012-08-16 02:39:31 PDT
Comment on attachment 651538 [details] [diff] [review]
patch - avoid the use of PRUint32

[Approval Request Comment]
Bug caused by (feature/regressing bug #): exposed by 734079 somehow
User impact if declined: security vuln.
Testing completed (on m-c, etc.): been on m-c for a few days
Risk to taking this patch (and alternatives if risky): low (no alternative)
String or UUID changes made by this patch: none
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-16 11:10:22 PDT
Comment on attachment 651538 [details] [diff] [review]
patch - avoid the use of PRUint32

approving for branches, marking wontfix for 14 since we won't chemspill for this.
Comment 12 Al Billings [:abillings] 2012-08-17 07:24:37 PDT
This needs to be resolved and closed. Don't leave this open.
Comment 13 Jonathan Watt [:jwatt] 2012-08-17 07:26:47 PDT
Still need to remember to check the testcase in after 15 goes out the door.
Comment 14 Daniel Veditz [:dveditz] 2012-08-17 08:12:17 PDT
(In reply to Jonathan Watt [:jwatt] from comment #13)
> Still need to remember to check the testcase in after 15 goes out the door.

Typical options are to query for your fixed bugs with the in-testsuite? flag, or clone the bug into a new "check in testcase for bug 782141" bug and set the target milestone out a release or whatever is necessary.

Since the code looks the same at a quick glance I'm going to assume we need this fixed on the ESR-10 branch for the corresponding release. Please unset the status if not.
Comment 16 Jonathan Watt [:jwatt] 2012-08-22 15:11:21 PDT
Created attachment 654379 [details] [diff] [review]
patch for esr10

The crash is not reproducible on esr10 with the testcase supplied, or with the reduced testcase. It doesn't seem to be reproducible on nightlies until the patch for bug 734079 landed. It's not clear to me why that patch would expose the vulnerability, since it didn't touch the vulnerable code. It seems most likely that it indirectly changed the input values at which a crash can be reproduced. Most likely there is some permutation of the numbers in the test that could be found that would reproduce the crash prior to the patch for bug 734079, and on the esr10 branch too. Hence, here is a patch for the esr10 branch to fix the vulnerable code even though we don't have a test to reproduce the crash.

I'm currently away on a course and not back properly until Friday, so if this gets approved it would be great if someone could land it for me if I don't get to it first. The patch builds for me on esr10 branch locally on Mac.
Comment 17 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-22 15:16:03 PDT
Comment on attachment 654379 [details] [diff] [review]
patch for esr10

Will set checkin-needed so this gets landed.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-08-22 15:20:31 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/41fd68bda27f
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-23 15:54:22 PDT
Confirmed testcase reproducible with try-server ASan build from decoder with changeset 9f3cc040e41a.

Verified testcase not reproducible with: 
 * 17.0a1: 198ca6edd0ae (debug) built on 20120823 by decoder
 * 16.0a2: 805e936380ab (debug) built on 20120823 by decoder

qa- for Firefox 15 and ESR15 as builds are not available.
Comment 20 Jonathan Watt [:jwatt] 2012-08-29 03:09:58 PDT
Is it okay to land the test now?
Comment 21 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-08-29 06:06:53 PDT
Please wait until the security team unhides the bug.  We released the fixed version less than 24 hours ago; users haven't updated yet.
Comment 22 Jonathan Watt [:jwatt] 2012-08-29 06:19:24 PDT
Will do. Thanks.
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-11-04 17:31:23 PST
I'm thinking it's probably safe to land the crashtest now :)
Comment 24 Al Billings [:abillings] 2012-11-05 14:33:29 PST
(In reply to Ryan VanderMeulen from comment #23)
> I'm thinking it's probably safe to land the crashtest now :)

Yes, it is.
Comment 25 Jonathan Watt [:jwatt] 2012-11-06 06:07:19 PST
Pushed crashtest:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1dc22a0e39
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-11-06 18:24:41 PST
https://hg.mozilla.org/mozilla-central/rev/7c1dc22a0e39
Comment 27 Raymond Forbes[:rforbes] 2013-07-19 18:27:47 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719

Note You need to log in before you can comment on or make changes to this bug.