Closed Bug 696579 Opened 9 years ago Closed 9 years ago
carefully chosen values of kernel
Unit Length can cause lighting filters to overwrite memory they don't own
No description provided.
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.
Assignee: nobody → longsonr
Attachment #568873 - 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.
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.
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 #569326 - Attachment is patch: true
Attachment #569326 - Flags: review?(roc) → review+
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?
Status: NEW → RESOLVED
Closed: 9 years ago
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.
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
Landed on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/152e68819fd7 Setting status-firefox9 --> Fixed.
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?
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:220.127.116.11) Gecko/20111103 Firefox/3.6.24.
Verified fixed on Firefox 9.0b3 and 10.0a2.
I don't crash on 3.6.24 either, apparently the comment 2 guess that the bug 610466 changes "may be significant" was correct.
You need to log in before you can comment on or make changes to this bug.