782141 - (CVE-2012-3969) Heap-buffer-overflow in nsSVGFEMorphologyElement::Filter

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Arthur Gerkis]

Attachment: test-case triggering the crash (*.zip)

ASan detected heap-buffer-overflow on opt build f24229bc0ec8.

Comment 1

[Arthur Gerkis]

Attachment: ASan log

Comment 2

[Jonathan Watt [:jwatt]]

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]]

[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]]

Attachment: patch - avoid the use of PRUint32

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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]]

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]]

Attachment: crashtest - to be checked in later

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2f41b9ebdba

Comment 9

[Jonathan Watt [:jwatt]]

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]

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 11

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4baf783709af
https://hg.mozilla.org/releases/mozilla-beta/rev/8f291048784f

Comment 12

[Al Billings [:abillings - ex-MoCo]]

This needs to be resolved and closed. Don't leave this open.

Comment 13

[Jonathan Watt [:jwatt]]

Still need to remember to check the testcase in after 15 goes out the door.

Comment 14

[Daniel Veditz [:dveditz]]

(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]]

Attachment: 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]

Comment on attachment 654379 [details] [diff] [review]
patch for esr10

Will set checkin-needed so this gets landed.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/41fd68bda27f

Comment 19

[u279076]

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]]

Is it okay to land the test now?

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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]]

Will do. Thanks.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

I'm thinking it's probably safe to land the crashtest now :)

Comment 24

[Al Billings [:abillings - ex-MoCo]]

(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]]

Pushed crashtest:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1dc22a0e39

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c1dc22a0e39

Comment 27

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719