New issue
Advanced search Search tips

Issue 41860 link

Starred by 3 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

WebCore::RenderPath::paint RecursionSOV (22f809badd4bb5ceb0dd6f3c9bd56277)

Reported by tk.chrom...@gmail.com, Apr 17 2010

Issue description

Opening the attached SVG file causes infinite recursion in 
WebCore::SVGPatternElement::buildPattern, which leads to stack exhaustion 
(sad tab).

Code snippet:

http://svn.webkit.org/repository/webkit/trunk/WebCore/svg/SVGPatternElement
.cpp

[..]
void SVGPatternElement::buildPattern(const FloatRect& targetRect) const
{
[..]
    // Render subtree into ImageBuffer
    for (Node* n = attributes.patternContentElement()->firstChild(); n; n = 
n->nextSibling()) {
        if (!n->isSVGElement() || !static_cast<SVGElement*>(n)->isStyled() 
|| !n->renderer())
            continue;
        renderSubtreeToImage(patternImage.get(), n->renderer());
    }
[..]

I filed a bug upstream: https://bugs.webkit.org/show_bug.cgi?id=37751

 
testcase.xhtml
346 bytes View Download
Labels: -Area-Undefined Area-WebKit WebKit-Core Security SecSeverity-Low
Status: Available
Summary: WebCore::RenderPath::paint RecursionSOV (22f809badd4bb5ceb0dd6f3c9bd56277)
Confirmed on Chromium 6.0.411.0 (47700), details attached.

Repro code for your viewing pleasure (the "pattern" contains an "ellipse", which "fill" attributes references the "pattern", making an 
infinite loop):

<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
 <defs>
  <pattern id="test" x="10" y="10" width="10" height="10">
   <ellipse cx="10" cy="10" rx="10" ry="10" fill="url(#test)" />
  </pattern>
 </defs>
 <rect x="10" y="10" width="10" height="10" rx="10" ry="10" fill="url(#test)" />
</svg>

Crash in SVG => potential DoS on certain websites => security.


WebCore..RenderPath..paint RecursionSOV (22f809badd4bb5ceb0dd6f3c9bd56277).html
671 KB Download
Main bug upstream: https://bugs.webkit.org/show_bug.cgi?id=32171

Comment 3 by karen@chromium.org, May 26 2010

Labels: Mstone-6

Comment 4 by jsc...@chromium.org, May 26 2010

 Issue 44995  has been merged into this issue.

Comment 5 by jsc...@chromium.org, May 26 2010

 Issue 44998  has been merged into this issue.

Comment 6 by jsc...@chromium.org, May 26 2010

The previously landed fix < http://trac.webkit.org/changeset/57263 > fails for nested 
instantiations of objects with extremely deep stacks. I can think of only two 
possible solutions to this:

1) bite the bullet and cap the nesting at a much lower value (maybe 300).
2) Find a good metric for weighting the size of objects on the stack and use that, 
rather than a simple count.

I don't like solution #1 and I'm not sure of how feasible solution #2 is.

Comment 7 by jsc...@chromium.org, May 26 2010

Labels: Restrict-View-SecurityTeam

Comment 8 by jsc...@chromium.org, May 27 2010

A few more thoughts. We can detect and fail on recursion for any sort of linking 
element, which would catch these cases. Although that doesn't fix general deep nesting.

We might want a cap on nesting while painting, since that's what's causing these 
crashes.

Here's a thought: probe for imminent stack-overflow by allocating a large chunk of 
memory on the stack (say 1K) and stop if it fails. If it succeeds, free the chunk and 
continue.

Cleaner would be to actually calculate the ammount of free space on the stack, but I 
bet that can only be done in a platform dependant way, whereas functions such as alloca 
or creating an array of chars should be a cross-platform way to probe the stack.
As I explained in  bug 44995 , there's no generic way to catch a stack overflow 
exception/signal, and there's no other way to know you've hit the end of the stack. 
So you'd need to add hairy, platform-specific code in the WebCore painting chain to 
probe the stack and catch the exception. And I expect that anywhere you put that 
check would be better handled with a simple nesting test.

If you think I'm mistaken, then code up your approach and test it against the 
platforms WebKit supports. I'm definitely not going to argue when there's a good, 
implemented solution in front of me.

After a quick grep it looks like RenderObject::PaintInfo is instantiated only on the 
stack and only in painting routines. So, it seems like we could either add a depth 
check and bailout at each of these instantiation points, or track depth in the 
constructors/destructor and add bailout tests on the appropriate method calls. Since 
painting occurs only on the main thread we can safely store the current depth in a 
global or static member variable.

I have a number of other things on my plate and don't expect to be implementing this 
in the near future. So, if anyone else wants to take a shot at it feel free, but this 
seems like the best approach to me.

Sorry, I misread that you had a few ideas and was unaware that you had a working 
solution. I was trying to suggest a generic solution that can easily be reused in any 
recursive call, because I'd like to apply it to the many recursions I found in HTML as 
well.

I should have checked if alloca returns NULL on failure in Windows as it does in Linux 
- turns out it does not :(.

Do let me know when this lands and I'll remove the relevant exceptions from my "known 
issues" list so my framework stops ignoring them. That way it should report any 
variations it can find that are not covered by your fix.
Not to beat a dead horse here, but alloca() is a commonly misunderstood and often 
dangerous function. On Windows it doesn't return NULL on failure; it just bubbles up 
the stack overflow system exception. In POSIX its stack overflow behavior is 
undefined. Ideally you'll immediately fire a SIGSEGV, but some implementations just 
increment the stack pointer without committing pages. So, you may not hit the SIGSEGV 
until later code accesses the buffer. Worse yet, you may end up with the start of 
your buffer past the stack guard page and in previously allocated memory. And while 
probably not relevant in this case, you should know that some alloca() 
implementations historically don't check for stack wrapping, which is an obvious 
security when size is unbounded.

Basically, any mention of alloca() or stack manipulation immediately sets off alarms 
for me.

Labels: -Pri-2 Pri-3
Labels: -Security -SecSeverity-Low -Restrict-View-SecurityTeam
Lifting security flags because we are no longer tracking any non-exploitable renderer crashes as security bugs.

Comment 16 by karen@chromium.org, Jun 28 2010

Labels: -Mstone-6 Mstone-X
Project Member

Comment 17 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-WebKit -WebKit-Core Cr-Content Cr-Content-Core
Project Member

Comment 18 by bugdroid1@chromium.org, Apr 6 2013

Labels: -Cr-Content Cr-Blink

Comment 19 by tkent@chromium.org, Jul 15 2015

Labels: -Cr-Content-Core

Comment 20 by tkent@chromium.org, Mar 23 2016

Components: -Blink Blink>SVG
Labels: -Mstone-X mstone-X
Status: WontFix (was: Available)
I assume this was fixed.

Sign in to add a comment