Closed Bug 963223 Opened 11 years ago Closed 3 years ago

SVG: stack-buffer-overflow / OOM crash [@unw_init_local/mozilla::gfx::FlattenedPath::LineTo]

Categories

(Core :: Graphics, defect, P2)

x86_64
All
defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: posidron, Assigned: bas.schouten)

References

Details

(5 keywords)

Attachments

(5 files)

Attached file testcase (deleted) —
Takes a few minutes to trigger. With my MacOS build the testcase results in a stack-buffer-overflow crash (read) while on Linux in an OOM crash. Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/c0df37c18e54
Attached file callstack-linux (deleted) —
Attached file callstack-osx.txt (deleted) —
This might be a false positive, my sec-rating might not be appropriate.
Summary: SVG: stack-buffer-overflow / OOM crash [@unw_init_local] → SVG: stack-buffer-overflow / OOM crash [@unw_init_local/mozilla::gfx::FlattenedPath::LineTo]
I see this crash on Windows 7 too. Bas might know about this. I saw a stack overflow with the original patch for bug 941585 although stack looks somewhat different.
Flags: needinfo?(bas)
OS: Mac OS X → All
Regression window on Linux64: 2013-11-17-03-02-03 -- 2013-11-18-03-02-03 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a475f94bb1b1&tochange=beddd6d4bcdf This looks like a possible candidate: changeset: 155112:f2e964f10799 user: Bas Schouten <bschouten@mozilla.com> date: Mon Nov 11 12:42:07 2013 +1300 summary: Bug 939049 - Part 2: Add generic ComputeLength code for backends with no such functionality. r=jrmuizel It seems the bug number in that summary is wrong though.
Keywords: regression, testcase
Priority: -- → P2
Actually, one of these is probably the more likely culprit: changeset: 155121:af0931327e49 user: Jonathan Watt <jwatt@jwatt.org> date: Mon Nov 18 01:29:06 2013 +0000 summary: Bug 938388 - Convert all remaining code for calculating path lengths and position at an offset along a path in content/svg to Moz2D (kill off all uses of gfxPath). r=dholbert changeset: 155120:4f086025350f user: Jonathan Watt <jwatt@jwatt.org> date: Mon Nov 18 01:29:06 2013 +0000 summary: Bug 930577 - Convert much of the SVG code for calculating path lengths and position at an offset along a path to Moz2D. r=heycam
Flags: needinfo?(jwatt)
Seems like there may still be some degeneracies that Bas's algo doesn't handle. Bas, can you take a look at this one?
Flags: needinfo?(jwatt)
(In reply to Jonathan Watt [:jwatt] from comment #6) > Seems like there may still be some degeneracies that Bas's algo doesn't > handle. Bas, can you take a look at this one? I'll have a look later, in the meanwhile could somebody post the stack overflow's stack trace?
Flags: needinfo?(bas)
Attached patch debug (deleted) — Splinter Review
On Linux, it's an infinite loop. I added this printf and get: t=0.000361 s3=-1020.856873 aTolerance=0.000100 currentCP.mCP1=-45533196.000000,21729924.000000 currentCP.mCP2=-45534984.000000,21727158.000000 currentCP.mCP3=-45535716.000000,21724392.000000 currentCP.mCP4=-45530196.000000,21721626.000000 The values does not change... let me know if you need more data.
Flags: needinfo?(bas)
(In reply to Mats Palmgren (:mats) from comment #8) > Created attachment 8365682 [details] [diff] [review] > debug > > On Linux, it's an infinite loop. I added this printf and get: > > t=0.000361 s3=-1020.856873 aTolerance=0.000100 > currentCP.mCP1=-45533196.000000,21729924.000000 > currentCP.mCP2=-45534984.000000,21727158.000000 > currentCP.mCP3=-45535716.000000,21724392.000000 > currentCP.mCP4=-45530196.000000,21721626.000000 > > The values does not change... let me know if you need more data. For now, no, that's very interesting! Thank you.
Flags: needinfo?(bas)
During Critsmash we thought this should be lowered to sec-high. Bas?
Keywords: sec-criticalsec-high
Flags: needinfo?(bas)
(In reply to David Bolter [:davidb] from comment #10) > During Critsmash we thought this should be lowered to sec-high. Bas? I agree, I don't see any obvious ways this is exploitable since it seems to be a stack overflow from what I read here. I'll look into this soon.
Flags: needinfo?(bas)
Component: SVG → Graphics
Any update here?
Flags: needinfo?(bas)
I looked at this a little, but not in any great detail, it has some SVG and JS stuff in there that I'm not completely sure what it will do. I still think this is not exploitable. When JWatt comes back, or somebody else, could somebody provide me with a more reduced, straight-forward test case that causes a path that has this issue? Fwiw, this causes an OOM on Windows too. If it's not reducible any further I'll try and figure out from the stack I have currently.
Flags: needinfo?(bas)
Group: gfx-core-security
JWatt, wondering if you might have a reduced test case idea? (Bas is wanting one in comment 13).
Flags: needinfo?(jwatt)
Flags: needinfo?(jwatt)
Bas, can you take a look?
Flags: needinfo?(bas)
Assignee: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas)
Bas, you mentioned in comment 13 that you felt that this was probably not exploitable. The CritSmash team wanted to know if you could take one last look. If this is the case, we'd probably change the security rating, but we'd feel more comfortable with your input. Thank you.
Flags: needinfo?(bas)
(In reply to Matt Wobensmith from comment #17) > Bas, you mentioned in comment 13 that you felt that this was probably not > exploitable. The CritSmash team wanted to know if you could take one last > look. If this is the case, we'd probably change the security rating, but > we'd feel more comfortable with your input. Thank you. Yeah, I really don't see how this could be exploitable in any way. We simply get stuck in a loop, allocate more and more memory, and then crash. I should fix this anyway, so it's good you drew my attention to it :)
Flags: needinfo?(bas)
So the problem here is we're using numbers that are so big, that single precision floating point can't represent them very well. I believe SVG doesn't guarantee double precision, so this is probably okay. But we shouldn't crash, there's a couple of things we could do here: 1. Force the algorithm along, i.e. if (t == oldt) t += epsilon; 2. Cap coordinates at some value somewhere 3. Bail if the algorithm notes it doesn't have precision to continue i.e. if (t == oldt) return; 4. Suggestions welcome... Do you have a preference Jonathan?
Flags: needinfo?(jwatt)
I don't really mind, as long as we have a debug assertion or something that makes in quick to locate the issue if someone files a bug about content mis-rendering.
Flags: needinfo?(jwatt)
s/assertion/warning
Group: core-security

I cannot reproduce this, I suspect we fixed this elsewhere.

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
Group: gfx-core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: