Closed Bug 696579 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla10
Tracking Status
firefox8 - affected
firefox9 - fixed
status1.9.2 --- unaffected

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

(Keywords: verified-aurora, verified-beta, Whiteboard: [sg:critical][qa!])

Attachments

(2 files, 2 obsolete files)

No description provided.
Attached image testcase(crashes)
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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #568873 - Flags: review?(roc)
Attached patch right patch (obsolete) — Splinter Review
Attachment #568873 - Attachment is obsolete: true
Attachment #568873 - Flags: review?(roc)
Attachment #568874 - Flags: review?(roc)
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 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.
Attachment #568874 - Attachment is patch: true
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.
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.
(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.
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.
Then we should make sure that if it's within epsilon of an integer, it shouldn't get rounded out.
Attached patch updated patchSplinter Review
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.
Attachment #568874 - Attachment is obsolete: true
Attachment #568874 - Flags: review?(roc)
Attachment #569326 - Flags: review?(roc)
Attachment #569326 - Attachment is patch: true
Attachment #569326 - Flags: approval1.9.2.24?
Attachment #569326 - Flags: approval-mozilla-beta?
Attachment #569326 - Flags: approval-mozilla-aurora?
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.
Dan, can we get a security rating on this so we can reason about it?
https://hg.mozilla.org/mozilla-central/rev/ebd6501de883
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
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!
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.
(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.
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.
Whiteboard: [sg:critical]
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
Attachment #569326 - Flags: approval1.9.2.24?
Attachment #569326 - Flags: approval-mozilla-beta?
Attachment #569326 - Flags: approval-mozilla-beta-
Attachment #569326 - Flags: approval-mozilla-aurora?
Attachment #569326 - Flags: approval-mozilla-aurora+
Adding [qa+] for bug verification. It's crasher, it has a testcase, it should be easy to verify.
Whiteboard: [sg:critical] → [sg:critical][qa+]
qawanted: Since the feature was introduced in 2008 I'm guessing this affects the 1.9.2 branch?
blocking1.9.2: --- → ?
status1.9.2: --- → ?
Keywords: qawanted
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.
Keywords: qawanted
Verified fixed on Firefox 9.0b3 and 10.0a2.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
I don't crash on 3.6.24 either, apparently the comment 2 guess that the bug 610466 changes "may be significant" was correct.
blocking1.9.2: ? → ---
Group: core-security
You need to log in before you can comment on or make changes to this bug.