696579 - carefully chosen values of kernelUnitLength can cause lighting filters to overwrite memory they don't own

Status: VERIFIED FIXED

(Core :: SVG, defect)

Comment 1

[Robert Longson [:longsonr]]

Attachment: testcase(crashes)

Comment 2

[Robert Longson [:longsonr]]

Crashes Firefox 7 and trunk for me. The lighting filter scaling code was introduced in bug 416305. There's a modification to that code in bug 610466 that may be significant.

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Comment 4

[Robert Longson [:longsonr]]

Attachment: right patch

Comment 5

[Robert Longson [:longsonr]]

bug 610466 made it easier to trigger this bug but I suspect you can crash without it although you'd need to use different and even more carefully chosen values.

Comment 6

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

Comment on attachment 568874 [details] [diff] [review]
right patch

Why does this fix the bug? It's not even clear to me what the problem is.

Comment 7

[Robert Longson [:longsonr]]

In the testcase we have these values...

aTarget->mImage width, height = 300
aDataRect = 0, 0, 300, 300
kernelX, kernelY = 1.6552

300/1.6552 = 181.246

scaledSize = 181 x 181 as nsSVGUtils::ConvertToSurfaceSize does a NS_lround to round to the nearest integer.

gfxRect r(aDataRect.x, aDataRect.y, aDataRect.width, aDataRect.height);

r = 0, 0, 300, 300

r.Scale(gfxFloat(scaledSize.width)/aTarget->mImage->Width(),
        gfxFloat(scaledSize.height)/aTarget->mImage->Height());

r = 0, 0, 181.00000000000003, 181.00000000000003

r.RoundOut();

r = 0, 0, 182, 182

r > scaledSize and we blow up.

I think we need the same rounding as ConvertToSurfaceSize.

Comment 8

[Robert Longson [:longsonr]]

Maybe nsSVGUtils::ConvertToSurfaceSize should round out which I could look into as a follow up if you like but we'd still need to be careful not to overflow later so we'd still need it to be Round rather than RoundOut even if it did.

Comment 9

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

(In reply to Robert Longson from comment #8)
> Maybe nsSVGUtils::ConvertToSurfaceSize should round out which I could look
> into as a follow up if you like

That seems like the right thing.

> but we'd still need to be careful not to
> overflow later so we'd still need it to be Round rather than RoundOut even
> if it did.

Not sure why.

Comment 10

[Robert Longson [:longsonr]]

We'd have scaledSize greater by 1 so

r.Scale(gfxFloat(scaledSize.width)/aTarget->mImage->Width(),
        gfxFloat(scaledSize.height)/aTarget->mImage->Height());

might then give us 

r = 0, 0, 182.00000000000003, 182.00000000000003

which we'd not want to round out.

Comment 11

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

Then we should make sure that if it's within epsilon of an integer, it shouldn't get rounded out.

Comment 12

[Robert Longson [:longsonr]]

Attachment: updated patch

This seems to work. We scale the surface and the rect in the same way and then round them both out so that they always match.

Comment 13

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd6501de883

Comment 14

[Robert Longson [:longsonr]]

We allocate a slightly larger region for non-integer sized filters now so that when we calculated the dirty region we want to repaint we don't find that that extends outside the bounds of the surface. I think this is a pretty low risk change.

Comment 15

[LegNeato]

Dan, can we get a security rating on this so we can reason about it?

Comment 16

[Daniel Holbert [:dholbert]]

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

Comment 17

[Alex Keybl [:akeybl]]

Daniel H - in DanV's absence, if you can provide any insight (however rough) about the criticality of this bug for FF8, it would be greatly appreciated. Thanks!

Comment 18

[Robert Longson [:longsonr]]

I haven't actually tried 3.6 to see if it crashes with the testcase, nor have I checked that this patch fixes it if it does. 3.6 does not have bug 610466 and that may make it safe from this crash. On the other hand it may need that bug too in order to be safe.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Alex Keybl [:akeybl] from comment #17)
> Daniel H - in DanV's absence, if you can provide any insight (however rough)
> about the criticality of this bug for FF8

roc / longsonr can probably provide more educated feedback, but here are my thoughts:

Overwriting random memory is scary... I imagine this could potentially overwrite a vtable pointer or something, which would be exploitable. Given that, I'd think we'd want to aggressively get this onto our affected branches (i.e. at least back to Firefox 8, and back to 3.6 if we determine it to be affected).

It does seem like this might cause a behavior change, but I think it'd be minimal (1px larger/smaller in filter-rendering), and worth the security win.

Comment 20

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

marking bug as sg:critical given description and what not.
In discussing this during the beta/aurora meting, at this point this does not seem worthy to respin the beta, however, if we do respin this should go in for sure. Not chaning the flag for now and leaving this.

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 569326 [details] [diff] [review]
updated patch

[Triage Comment]
* Approving for Aurora since this is sg:critical
* Denying for Beta since we don't expect to re-roll

Comment 22

[Daniel Holbert [:dholbert]]

Landed on Aurora:
  https://hg.mozilla.org/releases/mozilla-aurora/rev/152e68819fd7

Setting status-firefox9 --> Fixed.

Comment 23

[juan becerra [:juanb]]

Adding [qa+] for bug verification. It's crasher, it has a testcase, it should be easy to verify.

Comment 24

[Daniel Veditz [:dveditz]]

qawanted: Since the feature was introduced in 2008 I'm guessing this affects the 1.9.2 branch?

Comment 25

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

No crash when I run the attached testcase on 3.6.24 on OS X 10.7: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.7; en-US; rv:1.9.2.24) Gecko/20111103 Firefox/3.6.24.

Comment 26

[u279076]

Verified fixed on Firefox 9.0b3 and 10.0a2.

Comment 27

[Daniel Veditz [:dveditz]]

I don't crash on 3.6.24 either, apparently the comment 2 guess that the bug 610466 changes "may be significant" was correct.