Closed Bug 782141 (CVE-2012-3969) Opened 12 years ago Closed 12 years ago

Heap-buffer-overflow in nsSVGFEMorphologyElement::Filter

Categories

(Core :: SVG, defect)

17 Branch
x86_64
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox14 --- wontfix
firefox15 + fixed
firefox16 + verified
firefox17 --- verified
firefox-esr10 15+ fixed

People

(Reporter: ax330d, Assigned: jwatt)

References

Details

(Keywords: csectype-bounds, sec-critical, testcase, Whiteboard: [advisory-tracking+][qa-][asan])

Attachments

(5 files)

ASan detected heap-buffer-overflow on opt build f24229bc0ec8.
Attached file ASan log
Component: Untriaged → SVG
Product: Firefox → Core
Assignee: nobody → jwatt
Blocks: 734079
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
[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.
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+
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.
Attachment #651671 - Attachment is patch: true
Attachment #651671 - Flags: checkin?(jwatt)
Whiteboard: [leave open]
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?
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+
Whiteboard: [leave open] → [leave open][advisory-tracking+]
This needs to be resolved and closed. Don't leave this open.
Still need to remember to check the testcase in after 15 goes out the door.
Status: NEW → RESOLVED
Closed: 12 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.
Keywords: testcase, verifyme
Alias: CVE-2012-3969
Attached patch patch for esr10Splinter Review
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+
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
Keywords: verifyme
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa-]
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.
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.
Attachment #651671 - Flags: checkin?(jwatt) → checkin+
Flags: in-testsuite? → in-testsuite+
Whiteboard: [advisory-tracking+][qa-] → [advisory-tracking+][qa-][asan]
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.