Bug 782141 (CVE-2012-3969)

Heap-buffer-overflow in nsSVGFEMorphologyElement::Filter

VERIFIED FIXED in Firefox 15

Status

()

Core
SVG
VERIFIED FIXED
5 years ago
6 months ago

People

(Reporter: Arthur Gerkis, Assigned: jwatt)

Tracking

({csectype-bounds, sec-critical, testcase})

17 Branch
mozilla17
x86_64
All
csectype-bounds, sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox14 wontfix, firefox15+ fixed, firefox16+ verified, firefox17 verified, firefox-esr1015+ fixed)

Details

(Whiteboard: [advisory-tracking+][qa-][asan])

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
Created attachment 651213 [details]
test-case triggering the crash (*.zip)

ASan detected heap-buffer-overflow on opt build f24229bc0ec8.
(Reporter)

Comment 1

5 years ago
Created attachment 651214 [details]
ASan log
Component: Untriaged → SVG
Product: Firefox → Core
(Assignee)

Updated

5 years ago
Assignee: nobody → jwatt
Blocks: 734079
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
[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.
(Assignee)

Comment 4

5 years ago
Created attachment 651538 [details] [diff] [review]
patch - avoid the use of PRUint32
Attachment #651538 - Flags: review?(roc)
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.
Attachment #651538 - Flags: review?(roc) → review+
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Created attachment 651671 [details] [diff] [review]
crashtest - to be checked in later
(Assignee)

Updated

5 years ago
Attachment #651671 - Attachment is patch: true
(Assignee)

Updated

5 years ago
Attachment #651671 - Flags: checkin?(jwatt)
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/a2f41b9ebdba
Flags: in-testsuite?

Updated

5 years ago
Keywords: sec-critical
(Assignee)

Updated

5 years ago
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → fixed
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
(Assignee)

Comment 9

5 years ago
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
Attachment #651538 - Flags: approval-mozilla-beta?
Attachment #651538 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
tracking-firefox14: ? → ---
status-firefox14: affected → wontfix
tracking-firefox15: ? → +
tracking-firefox16: ? → +
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.
Attachment #651538 - Flags: approval-mozilla-beta?
Attachment #651538 - Flags: approval-mozilla-beta+
Attachment #651538 - Flags: approval-mozilla-aurora?
Attachment #651538 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/4baf783709af
https://hg.mozilla.org/releases/mozilla-beta/rev/8f291048784f
status-firefox15: affected → fixed
status-firefox16: affected → fixed
Whiteboard: [leave open] → [leave open][advisory-tracking+]
This needs to be resolved and closed. Don't leave this open.
(Assignee)

Comment 13

5 years ago
Still need to remember to check the testcase in after 15 goes out the door.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open][advisory-tracking+] → [advisory-tracking+]
(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.
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → 15+
Keywords: testcase, verifyme
Alias: CVE-2012-3969
(Assignee)

Comment 16

5 years ago
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.
Attachment #654379 - Flags: approval-mozilla-esr10?
Comment on attachment 654379 [details] [diff] [review]
patch for esr10

Will set checkin-needed so this gets landed.
Attachment #654379 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-esr10/rev/41fd68bda27f
status-firefox-esr10: affected → fixed
Keywords: checkin-needed
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.
Status: RESOLVED → VERIFIED
status-firefox16: fixed → verified
status-firefox17: fixed → verified
Keywords: verifyme
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa-]
(Assignee)

Comment 20

5 years ago
Is it okay to land the test now?
Please wait until the security team unhides the bug.  We released the fixed version less than 24 hours ago; users haven't updated yet.
(Assignee)

Comment 22

5 years ago
Will do. Thanks.
Group: core-security
I'm thinking it's probably safe to land the crashtest now :)
Target Milestone: --- → mozilla17
(In reply to Ryan VanderMeulen from comment #23)
> I'm thinking it's probably safe to land the crashtest now :)

Yes, it is.
Keywords: checkin-needed
(Assignee)

Comment 25

5 years ago
Pushed crashtest:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1dc22a0e39
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Attachment #651671 - Flags: checkin?(jwatt) → checkin+
(Assignee)

Updated

5 years ago
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/7c1dc22a0e39
Whiteboard: [advisory-tracking+][qa-] → [advisory-tracking+][qa-][asan]
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
Keywords: csectype-bounds
You need to log in before you can comment on or make changes to this bug.