- Revision
- 191731
- Author
- s...@apple.com
- Date
- 2015-10-29 10:12:43 -0700 (Thu, 29 Oct 2015)
Log Message
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.
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=""
<pattern id="b" xlink:href=""
But it does not detect cycles like:
<pattern id="a">
<rect fill="url(#b)"/>
</pattern>
<pattern id="b" xlink:href=""
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (191730 => 191731)
--- trunk/LayoutTests/ChangeLog 2015-10-29 16:51:41 UTC (rev 191730)
+++ trunk/LayoutTests/ChangeLog 2015-10-29 17:12:43 UTC (rev 191731)
@@ -1,3 +1,17 @@
+2015-10-29 Said Abou-Hallawa <sabouhall...@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 <cgar...@igalia.com>
Unreviewed. GTK+ gardening: rebaseline more tests after r191623.
Added: trunk/LayoutTests/svg/custom/pattern-content-inheritance-cycle-expected.svg (0 => 191731)
--- trunk/LayoutTests/svg/custom/pattern-content-inheritance-cycle-expected.svg (rev 0)
+++ trunk/LayoutTests/svg/custom/pattern-content-inheritance-cycle-expected.svg 2015-10-29 17:12:43 UTC (rev 191731)
@@ -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>
Added: trunk/LayoutTests/svg/custom/pattern-content-inheritance-cycle.svg (0 => 191731)
--- trunk/LayoutTests/svg/custom/pattern-content-inheritance-cycle.svg (rev 0)
+++ trunk/LayoutTests/svg/custom/pattern-content-inheritance-cycle.svg 2015-10-29 17:12:43 UTC (rev 191731)
@@ -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=""
+
+ <!-- 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=""
+ <pattern id="n" xlink:href=""
+
+ <!-- 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=""
+ </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>
Modified: trunk/Source/WebCore/ChangeLog (191730 => 191731)
--- trunk/Source/WebCore/ChangeLog 2015-10-29 16:51:41 UTC (rev 191730)
+++ trunk/Source/WebCore/ChangeLog 2015-10-29 17:12:43 UTC (rev 191731)
@@ -1,3 +1,65 @@
+2015-10-29 Said Abou-Hallawa <sabouhall...@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=""
+ <pattern id="b" xlink:href=""
+ But it does not detect cycles like:
+ <pattern id="a">
+ <rect fill="url(#b)"/>
+ </pattern>
+ <pattern id="b" xlink:href=""
+
+ 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 <calva...@igalia.com>
[Streams API] Turn WS states into integers and fix state initialization
Modified: trunk/Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp (191730 => 191731)
--- trunk/Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp 2015-10-29 16:51:41 UTC (rev 191730)
+++ trunk/Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp 2015-10-29 17:12:43 UTC (rev 191731)
@@ -54,6 +54,19 @@
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 @@
patternElement().synchronizeAnimatedSVGAttribute(anyQName());
m_attributes = PatternAttributes();
- patternElement().collectPatternAttributes(m_attributes);
+ collectPatternAttributes(m_attributes);
m_shouldCollectPatternAttributes = false;
}
Modified: trunk/Source/WebCore/rendering/svg/RenderSVGResourcePattern.h (191730 => 191731)
--- trunk/Source/WebCore/rendering/svg/RenderSVGResourcePattern.h 2015-10-29 16:51:41 UTC (rev 191730)
+++ trunk/Source/WebCore/rendering/svg/RenderSVGResourcePattern.h 2015-10-29 17:12:43 UTC (rev 191731)
@@ -53,6 +53,8 @@
virtual RenderSVGResourceType resourceType() const override { return PatternResourceType; }
+ void collectPatternAttributes(PatternAttributes&) const;
+
private:
void element() const = delete;
virtual const char* renderName() const override { return "RenderSVGResourcePattern"; }
Modified: trunk/Source/WebCore/rendering/svg/RenderSVGRoot.cpp (191730 => 191731)
--- trunk/Source/WebCore/rendering/svg/RenderSVGRoot.cpp 2015-10-29 16:51:41 UTC (rev 191730)
+++ trunk/Source/WebCore/rendering/svg/RenderSVGRoot.cpp 2015-10-29 17:12:43 UTC (rev 191731)
@@ -180,9 +180,10 @@
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);
Modified: trunk/Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp (191730 => 191731)
--- trunk/Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp 2015-10-29 16:51:41 UTC (rev 191730)
+++ trunk/Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp 2015-10-29 17:12:43 UTC (rev 191731)
@@ -432,7 +432,7 @@
// 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());
Modified: trunk/Source/WebCore/svg/SVGPatternElement.cpp (191730 => 191731)
--- trunk/Source/WebCore/svg/SVGPatternElement.cpp 2015-10-29 16:51:41 UTC (rev 191730)
+++ trunk/Source/WebCore/svg/SVGPatternElement.cpp 2015-10-29 17:12:43 UTC (rev 191731)
@@ -187,65 +187,42 @@
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);
+ if (!attributes.hasPatternContentElement() && childElementCount())
+ attributes.setPatternContentElement(this);
}
-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();
-}
-
AffineTransform SVGPatternElement::localCoordinateSpaceTransform(SVGLocatable::CTMScope) const
{
AffineTransform matrix;
