Exploitable crash happens when an SVG contains an indirect resource inheritance cycle
authorsaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 29 Oct 2015 17:12:43 +0000 (17:12 +0000)
committersaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 29 Oct 2015 17:12:43 +0000 (17:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=150203

Reviewed by Brent Fulgham.

Source/WebCore:

Detecting cycles in SVG resource references happens in two places.
1. In SVGResourcesCycleSolver::resolveCycles() which it is called from
   SVGResourcesCache::addResourcesFromRenderer(). When a cycle is deleted,
   SVGResourcesCycleSolver::breakCycle() is called to break the link. In
   the case of a cyclic resource inheritance, SVGResources::resetLinkedResource()
   is called to break this cycle.
2. SVGPatternElement::collectPatternAttributes() which is called from
   RenderSVGResourcePattern::buildPattern(). The purpose is to resolve
   the pattern attributes and to build a tile image which can be used to
   fill the SVG element renderer. Detecting the cyclic resource reference
   in this function is not sufficient and can detect simple cycles like
    <pattern id="a" xlink:href="#b"/>
    <pattern id="b" xlink:href="#a"/>.
   But it does not detect cycles like:
    <pattern id="a">
        <rect fill="url(#b)"/>
    </pattern>
    <pattern id="b" xlink:href="#a"/>.

The fix is to get rid of SVGPatternElement::collectPatternAttributes() which
uses SVGURIReference::targetElementFromIRIString() to navigates through the
referenced resource elements and tries to detect cycles. Instead we can
implement RenderSVGResourcePattern::collectPatternAttributes() which calls
SVGResourcesCache::cachedResourcesForRenderer() to get the SVGResources
of the pattern. Then we use SVGResources::linkedResource() to navigate the
resource inheritance tree. The cached SVGResources is guaranteed to be free
of cycles.

Tests: svg/custom/pattern-content-inheritance-cycle.svg

* rendering/svg/RenderSVGResourcePattern.cpp:
(WebCore::RenderSVGResourcePattern::collectPatternAttributes):
Collect the pattern attributes through the cachedResourcesForRenderer().

(WebCore::RenderSVGResourcePattern::buildPattern):
Direct the call to the renderer function.

* rendering/svg/RenderSVGResourcePattern.h:

* rendering/svg/RenderSVGRoot.cpp:
(WebCore::RenderSVGRoot::layout):
RenderSVGRoot needs to call SVGResourcesCache::clientStyleChanged() for all
the invalidated resources. If an attribute of an SVG resource was updated
dynamically, the cached SVGResources associated with the renderer of this
resource was stale.

* rendering/svg/SVGRenderTreeAsText.cpp:
(WebCore::writeSVGResourceContainer):
Direct the call to the renderer function.

* svg/SVGPatternElement.cpp:
(WebCore::SVGPatternElement::collectPatternAttributes):
(WebCore::setPatternAttributes): Deleted.
collectPatternAttributes() is a replacement of setPatternAttributes().

LayoutTests:

Ensure that we do not crash when an SVG has an indirect cyclic resource
inheritance. Make sure the cyclic resource was just ignored as if it did
not exist.

* svg/custom/pattern-content-inheritance-cycle-expected.svg: Added.
* svg/custom/pattern-content-inheritance-cycle.svg: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@191731 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/svg/custom/pattern-content-inheritance-cycle-expected.svg [new file with mode: 0644]
LayoutTests/svg/custom/pattern-content-inheritance-cycle.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp
Source/WebCore/rendering/svg/RenderSVGResourcePattern.h
Source/WebCore/rendering/svg/RenderSVGRoot.cpp
Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp
Source/WebCore/svg/SVGPatternElement.cpp

index 3f1e368412a87378fc59be36e31c603372e597c4..883de4775d9c74316ad41da6b84b932d0179fc2d 100644 (file)
@@ -1,3 +1,17 @@
+2015-10-29  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Exploitable crash happens when an SVG contains an indirect resource inheritance cycle
+        https://bugs.webkit.org/show_bug.cgi?id=150203
+
+        Reviewed by Brent Fulgham.
+
+        Ensure that we do not crash when an SVG has an indirect cyclic resource
+        inheritance. Make sure the cyclic resource was just ignored as if it did
+        not exist.
+
+        * svg/custom/pattern-content-inheritance-cycle-expected.svg: Added.
+        * svg/custom/pattern-content-inheritance-cycle.svg: Added.
+
 2015-10-29  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         Unreviewed. GTK+ gardening: rebaseline more tests after r191623.
diff --git a/LayoutTests/svg/custom/pattern-content-inheritance-cycle-expected.svg b/LayoutTests/svg/custom/pattern-content-inheritance-cycle-expected.svg
new file mode 100644 (file)
index 0000000..6a50e6d
--- /dev/null
@@ -0,0 +1,16 @@
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <g fill="none" stroke="black" stroke-width="1">
+        <circle cx="75" cy="75" fill="lime" r="50"/>
+        <circle cx="200" cy="75" fill="none" r="50"/>
+
+        <circle cx="75" cy="200" fill="lime" r="50"/>
+        <circle cx="200" cy="200" fill="none" r="50"/>
+        <circle cx="325" cy="200" fill="none" r="50"/>
+    
+        <circle cx="75" cy="325" fill="lime" r="50"/>
+        <circle cx="200" cy="325" fill="none" r="50"/>
+    
+        <circle cx="75" cy="450" fill="lime" r="50"/>
+        <circle cx="200" cy="450" fill="none" r="50"/>
+    </g>
+</svg>
diff --git a/LayoutTests/svg/custom/pattern-content-inheritance-cycle.svg b/LayoutTests/svg/custom/pattern-content-inheritance-cycle.svg
new file mode 100644 (file)
index 0000000..43020ed
--- /dev/null
@@ -0,0 +1,56 @@
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <defs>
+        <!-- a => b => a -->
+        <pattern id="a" x="0" y="0" width=".25" height=".25">
+            <rect fill="lime" width="100%" height="100%"/>
+            <rect fill="url(#b)" width="100%" height="100%"/>
+        </pattern>
+        <pattern id="b" xlink:href="#a"/>
+        
+        <!-- l => m => n => l -->
+        <pattern id="l" x="0" y="0" width=".25" height=".25">
+            <rect fill="lime" width="100%" height="100%"/>
+            <rect fill="url(#m)" width="100%" height="100%"/>
+        </pattern>
+        <pattern id="m" xlink:href="#n"/>
+        <pattern id="n" xlink:href="#l"/>
+        
+        <!-- p => q -->
+        <pattern id="p" x="0" y="0" width=".25" height=".25">
+            <rect fill="lime" width="100%" height="100%"/>
+            <rect fill="url(#q)" width="100%" height="100%"/>
+        </pattern>
+        <pattern id="q"/>
+        
+        <!-- t => s -->
+        <pattern id="s" x="0" y="0" width=".25" height=".25">
+            <rect fill="lime" width="100%" height="100%"/>
+            <rect id="r" width="100%" height="100%"/>
+        </pattern>
+        <pattern id="t" xlink:href="#s"/>
+    </defs>
+    <g fill="none" stroke="black" stroke-width="1">
+        <circle cx="75" cy="75" fill="url(#a)" r="50"/>
+        <circle cx="200" cy="75" fill="url(#b)" r="50"/>
+
+        <circle cx="75" cy="200" fill="url(#l)" r="50"/>
+        <circle cx="200" cy="200" fill="url(#m)" r="50"/>
+        <circle cx="325" cy="200" fill="url(#n)" r="50"/>
+    
+        <circle cx="75" cy="325" fill="url(#p)" r="50"/>
+        <circle cx="200" cy="325" fill="url(#q)" r="50"/>
+    
+        <circle cx="75" cy="450" fill="url(#s)" r="50"/>
+        <circle cx="200" cy="450" fill="url(#t)" r="50"/>
+    </g>
+    <script>
+        // Add q => p to get p => q => p
+        document.getElementById("q").setAttributeNS("http://www.w3.org/1999/xlink", "href", "#p");        
+        
+        // Add s => t to get s => t => s
+        document.getElementById("r").setAttribute("fill", "url(#t)");
+        
+        // Force layout
+        document.documentElement.removeAttribute("class");
+    </script>
+</svg>
index 5b6205e368852b6fc7c34a0c79bba08bbcc3dc3a..58e6dd174756e1bb302120d935fc9ecb26ec9ed4 100644 (file)
@@ -1,3 +1,65 @@
+2015-10-29  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Exploitable crash happens when an SVG contains an indirect resource inheritance cycle
+        https://bugs.webkit.org/show_bug.cgi?id=150203
+
+        Reviewed by Brent Fulgham.
+
+        Detecting cycles in SVG resource references happens in two places.
+        1. In SVGResourcesCycleSolver::resolveCycles() which it is called from 
+           SVGResourcesCache::addResourcesFromRenderer(). When a cycle is deleted,
+           SVGResourcesCycleSolver::breakCycle() is called to break the link. In
+           the case of a cyclic resource inheritance, SVGResources::resetLinkedResource()
+           is called to break this cycle.
+        2. SVGPatternElement::collectPatternAttributes() which is called from
+           RenderSVGResourcePattern::buildPattern(). The purpose is to resolve
+           the pattern attributes and to build a tile image which can be used to
+           fill the SVG element renderer. Detecting the cyclic resource reference
+           in this function is not sufficient and can detect simple cycles like
+            <pattern id="a" xlink:href="#b"/>
+            <pattern id="b" xlink:href="#a"/>.
+           But it does not detect cycles like:
+            <pattern id="a">
+                <rect fill="url(#b)"/>
+            </pattern>
+            <pattern id="b" xlink:href="#a"/>.
+   
+        The fix is to get rid of SVGPatternElement::collectPatternAttributes() which
+        uses SVGURIReference::targetElementFromIRIString() to navigates through the
+        referenced resource elements and tries to detect cycles. Instead we can
+        implement RenderSVGResourcePattern::collectPatternAttributes() which calls
+        SVGResourcesCache::cachedResourcesForRenderer() to get the SVGResources
+        of the pattern. Then we use SVGResources::linkedResource() to navigate the
+        resource inheritance tree. The cached SVGResources is guaranteed to be free
+        of cycles.
+
+        Tests: svg/custom/pattern-content-inheritance-cycle.svg
+
+        * rendering/svg/RenderSVGResourcePattern.cpp:
+        (WebCore::RenderSVGResourcePattern::collectPatternAttributes):
+        Collect the pattern attributes through the cachedResourcesForRenderer().
+        
+        (WebCore::RenderSVGResourcePattern::buildPattern):
+        Direct the call to the renderer function.
+        
+        * rendering/svg/RenderSVGResourcePattern.h:
+        
+        * rendering/svg/RenderSVGRoot.cpp:
+        (WebCore::RenderSVGRoot::layout):
+        RenderSVGRoot needs to call SVGResourcesCache::clientStyleChanged() for all
+        the invalidated resources. If an attribute of an SVG resource was updated
+        dynamically, the cached SVGResources associated with the renderer of this
+        resource was stale.
+        
+        * rendering/svg/SVGRenderTreeAsText.cpp:
+        (WebCore::writeSVGResourceContainer):
+        Direct the call to the renderer function.        
+        
+        * svg/SVGPatternElement.cpp:
+        (WebCore::SVGPatternElement::collectPatternAttributes):
+        (WebCore::setPatternAttributes): Deleted.
+        collectPatternAttributes() is a replacement of setPatternAttributes().
+        
 2015-10-29  Xabier Rodriguez Calvar  <calvaris@igalia.com>
 
         [Streams API] Turn WS states into integers and fix state initialization
index bd5e13d9c3ba78a495b5c5fd1ada002e2be60392..eaa1e84c6f7304a0c9937659933f0cba5cc2ae49 100644 (file)
@@ -54,6 +54,19 @@ void RenderSVGResourcePattern::removeClientFromCache(RenderElement& client, bool
     markClientForInvalidation(client, markForInvalidation ? RepaintInvalidation : ParentOnlyInvalidation);
 }
 
+void RenderSVGResourcePattern::collectPatternAttributes(PatternAttributes& attributes) const
+{
+    const RenderSVGResourcePattern* current = this;
+
+    while (current) {
+        const SVGPatternElement& pattern = current->patternElement();
+        pattern.collectPatternAttributes(attributes);
+
+        auto* resources = SVGResourcesCache::cachedResourcesForRenderer(*current);
+        current = resources ? downcast<RenderSVGResourcePattern>(resources->linkedResource()) : nullptr;
+    }
+}
+
 PatternData* RenderSVGResourcePattern::buildPattern(RenderElement& renderer, unsigned short resourceMode, GraphicsContext& context)
 {
     PatternData* currentData = m_patternMap.get(&renderer);
@@ -64,7 +77,7 @@ PatternData* RenderSVGResourcePattern::buildPattern(RenderElement& renderer, uns
         patternElement().synchronizeAnimatedSVGAttribute(anyQName());
 
         m_attributes = PatternAttributes();
-        patternElement().collectPatternAttributes(m_attributes);
+        collectPatternAttributes(m_attributes);
         m_shouldCollectPatternAttributes = false;
     }
 
index f0cac465242c72ae90a8b441bf41d97635cedb92..5eeaa2442402e704d3828751602b1e3cd8e5e93a 100644 (file)
@@ -53,6 +53,8 @@ public:
 
     virtual RenderSVGResourceType resourceType() const override { return PatternResourceType; }
 
+    void collectPatternAttributes(PatternAttributes&) const;
+
 private:
     void element() const = delete;
     virtual const char* renderName() const override { return "RenderSVGResourcePattern"; }
index 9fef1d37a6ce3768f0973d6f345cf0baba395e01..7852f889daca14c669d47240a0fccd37dfd1554e 100644 (file)
@@ -180,9 +180,10 @@ void RenderSVGRoot::layout()
 
     if (!m_resourcesNeedingToInvalidateClients.isEmpty()) {
         // Invalidate resource clients, which may mark some nodes for layout.
-        HashSet<RenderSVGResourceContainer*>::iterator end = m_resourcesNeedingToInvalidateClients.end();
-        for (HashSet<RenderSVGResourceContainer*>::iterator it = m_resourcesNeedingToInvalidateClients.begin(); it != end; ++it)
-            (*it)->removeAllClientsFromCache();
+        for (auto& resource :  m_resourcesNeedingToInvalidateClients) {
+            resource->removeAllClientsFromCache();
+            SVGResourcesCache::clientStyleChanged(*resource, StyleDifferenceLayout, resource->style());
+        }
 
         m_isLayoutSizeChanged = false;
         SVGRenderSupport::layoutChildren(*this, false);
index b6dee261a61d9f5d0dcba00bfec780e30e6f0fa1..b6c6382bc5e7aa5a0f1ba91cb36ba2d972aada57 100644 (file)
@@ -432,7 +432,7 @@ void writeSVGResourceContainer(TextStream& ts, const RenderSVGResourceContainer&
         // Dump final results that are used for rendering. No use in asking SVGPatternElement for its patternUnits(), as it may
         // link to other patterns using xlink:href, we need to build the full inheritance chain, aka. collectPatternProperties()
         PatternAttributes attributes;
-        pattern.patternElement().collectPatternAttributes(attributes);
+        pattern.collectPatternAttributes(attributes);
 
         writeNameValuePair(ts, "patternUnits", attributes.patternUnits());
         writeNameValuePair(ts, "patternContentUnits", attributes.patternContentUnits());
index a07c8750a7ab86487854d4c806bf814e18af89f9..df2c9bab4f7e93f90cb5f80c40a501aa87a4fa3b 100644 (file)
@@ -187,63 +187,40 @@ RenderPtr<RenderElement> SVGPatternElement::createElementRenderer(Ref<RenderStyl
     return createRenderer<RenderSVGResourcePattern>(*this, WTF::move(style));
 }
 
-static void setPatternAttributes(const SVGPatternElement& element, PatternAttributes& attributes)
+void SVGPatternElement::collectPatternAttributes(PatternAttributes& attributes) const
 {
-    if (!attributes.hasX() && element.hasAttribute(SVGNames::xAttr))
-        attributes.setX(element.x());
+    if (!attributes.hasX() && hasAttribute(SVGNames::xAttr))
+        attributes.setX(x());
 
-    if (!attributes.hasY() && element.hasAttribute(SVGNames::yAttr))
-        attributes.setY(element.y());
+    if (!attributes.hasY() && hasAttribute(SVGNames::yAttr))
+        attributes.setY(y());
 
-    if (!attributes.hasWidth() && element.hasAttribute(SVGNames::widthAttr))
-        attributes.setWidth(element.width());
+    if (!attributes.hasWidth() && hasAttribute(SVGNames::widthAttr))
+        attributes.setWidth(width());
 
-    if (!attributes.hasHeight() && element.hasAttribute(SVGNames::heightAttr))
-        attributes.setHeight(element.height());
+    if (!attributes.hasHeight() && hasAttribute(SVGNames::heightAttr))
+        attributes.setHeight(height());
 
-    if (!attributes.hasViewBox() && element.hasAttribute(SVGNames::viewBoxAttr) && element.viewBoxIsValid())
-        attributes.setViewBox(element.viewBox());
+    if (!attributes.hasViewBox() && hasAttribute(SVGNames::viewBoxAttr) && viewBoxIsValid())
+        attributes.setViewBox(viewBox());
 
-    if (!attributes.hasPreserveAspectRatio() && element.hasAttribute(SVGNames::preserveAspectRatioAttr))
-        attributes.setPreserveAspectRatio(element.preserveAspectRatio());
+    if (!attributes.hasPreserveAspectRatio() && hasAttribute(SVGNames::preserveAspectRatioAttr))
+        attributes.setPreserveAspectRatio(preserveAspectRatio());
 
-    if (!attributes.hasPatternUnits() && element.hasAttribute(SVGNames::patternUnitsAttr))
-        attributes.setPatternUnits(element.patternUnits());
+    if (!attributes.hasPatternUnits() && hasAttribute(SVGNames::patternUnitsAttr))
+        attributes.setPatternUnits(patternUnits());
 
-    if (!attributes.hasPatternContentUnits() && element.hasAttribute(SVGNames::patternContentUnitsAttr))
-        attributes.setPatternContentUnits(element.patternContentUnits());
+    if (!attributes.hasPatternContentUnits() && hasAttribute(SVGNames::patternContentUnitsAttr))
+        attributes.setPatternContentUnits(patternContentUnits());
 
-    if (!attributes.hasPatternTransform() && element.hasAttribute(SVGNames::patternTransformAttr)) {
+    if (!attributes.hasPatternTransform() && hasAttribute(SVGNames::patternTransformAttr)) {
         AffineTransform transform;
-        element.patternTransform().concatenate(transform);
+        patternTransform().concatenate(transform);
         attributes.setPatternTransform(transform);
     }
 
-    if (!attributes.hasPatternContentElement() && element.childElementCount())
-        attributes.setPatternContentElement(&element);
-}
-
-void SVGPatternElement::collectPatternAttributes(PatternAttributes& attributes) const
-{
-    HashSet<const SVGPatternElement*> processedPatterns;
-    const SVGPatternElement* current = this;
-
-    while (true) {
-        setPatternAttributes(*current, attributes);
-        processedPatterns.add(current);
-
-        // Respect xlink:href, take attributes from referenced element
-        Element* refElement = SVGURIReference::targetElementFromIRIString(current->href(), document());
-        if (is<SVGPatternElement>(refElement)) {
-            current = downcast<SVGPatternElement>(refElement);
-
-            // Cycle detection
-            if (processedPatterns.contains(current))
-                return;
-        } else
-            return;
-    }
-    ASSERT_NOT_REACHED();
+    if (!attributes.hasPatternContentElement() && childElementCount())
+        attributes.setPatternContentElement(this);
 }
 
 AffineTransform SVGPatternElement::localCoordinateSpaceTransform(SVGLocatable::CTMScope) const