Diff
Modified: trunk/Source/WebCore/ChangeLog (159488 => 159489)
--- trunk/Source/WebCore/ChangeLog 2013-11-19 09:28:38 UTC (rev 159488)
+++ trunk/Source/WebCore/ChangeLog 2013-11-19 09:43:22 UTC (rev 159489)
@@ -1,3 +1,40 @@
+2013-11-19 Ryosuke Niwa <[email protected]>
+
+ Add more assertions with security implications in DocumentOrderedMap
+ https://bugs.webkit.org/show_bug.cgi?id=124559
+
+ Reviewed by Antti Koivisto.
+
+ Assert that newly added elements and existing elements in the document ordered map are in the same tree scope
+ as the document ordered map. Also exit early if we're about to add an element in a wrong document to the map.
+ We don't exit early in get() because the damage has already been done at that point (the element may have been
+ deleted already).
+
+ * dom/Document.cpp:
+ (WebCore::Document::addImageElementByLowercasedUsemap):
+ * dom/DocumentOrderedMap.cpp:
+ (WebCore::DocumentOrderedMap::add): Assert that the newly added element is in the current tree scope.
+ Also exit early if either the element is not in the tree scope or not in the right document.
+ While this doesn't make the function completely fault safe, it'll catch when we try to add a detached node.
+ (WebCore::DocumentOrderedMap::remove): Convert existing assertions to ones with security implication.
+ (WebCore::DocumentOrderedMap::get): Assert with security implication that the element we're about to return
+ is in the current tree scope. The element may have already been deleted if we ever hit these assertions.
+ (WebCore::DocumentOrderedMap::getAllElementsById): Convert an existing assertion to an assertion with security
+ implication.
+ * dom/DocumentOrderedMap.h:
+ * dom/TreeScope.cpp:
+ (WebCore::TreeScope::addElementById):
+ (WebCore::TreeScope::addElementByName):
+ (WebCore::TreeScope::addImageMap):
+ (WebCore::TreeScope::addLabel):
+ * html/HTMLDocument.cpp:
+ (WebCore::HTMLDocument::addDocumentNamedItem):
+ (WebCore::HTMLDocument::addWindowNamedItem):
+ * html/HTMLImageElement.cpp:
+ (WebCore::HTMLImageElement::insertedInto): Set InTreeScope flag before calling addImageElementByLowercasedUsemap.
+ * html/HTMLMapElement.cpp:
+ (WebCore::HTMLMapElement::insertedInto): Ditto for addImageMap.
+
2013-11-18 Commit Queue <[email protected]>
Unreviewed, rolling out r159455.
Modified: trunk/Source/WebCore/dom/Document.cpp (159488 => 159489)
--- trunk/Source/WebCore/dom/Document.cpp 2013-11-19 09:28:38 UTC (rev 159488)
+++ trunk/Source/WebCore/dom/Document.cpp 2013-11-19 09:43:22 UTC (rev 159489)
@@ -675,7 +675,7 @@
void Document::addImageElementByLowercasedUsemap(const AtomicStringImpl& name, HTMLImageElement& element)
{
- return m_imagesByUsemap.add(name, element);
+ return m_imagesByUsemap.add(name, element, *this);
}
void Document::removeImageElementByLowercasedUsemap(const AtomicStringImpl& name, HTMLImageElement& element)
Modified: trunk/Source/WebCore/dom/DocumentOrderedMap.cpp (159488 => 159489)
--- trunk/Source/WebCore/dom/DocumentOrderedMap.cpp 2013-11-19 09:28:38 UTC (rev 159488)
+++ trunk/Source/WebCore/dom/DocumentOrderedMap.cpp 2013-11-19 09:43:22 UTC (rev 159489)
@@ -87,14 +87,18 @@
m_map.clear();
}
-void DocumentOrderedMap::add(const AtomicStringImpl& key, Element& element)
+void DocumentOrderedMap::add(const AtomicStringImpl& key, Element& element, const TreeScope& treeScope)
{
+ ASSERT_WITH_SECURITY_IMPLICATION(element.isInTreeScope());
+ ASSERT_WITH_SECURITY_IMPLICATION(treeScope.rootNode()->containsIncludingShadowDOM(&element));
+ if (!element.isInTreeScope() || &element.document() != treeScope.documentScope())
+ return;
Map::AddResult addResult = m_map.add(&key, MapEntry(&element));
if (addResult.isNewEntry)
return;
MapEntry& entry = addResult.iterator->value;
- ASSERT(entry.count);
+ ASSERT_WITH_SECURITY_IMPLICATION(entry.count);
entry.element = 0;
entry.count++;
entry.orderedList.clear();
@@ -104,15 +108,14 @@
{
m_map.checkConsistency();
auto it = m_map.find(&key);
- ASSERT(it != m_map.end());
+ ASSERT_WITH_SECURITY_IMPLICATION(it != m_map.end());
if (it == m_map.end())
return;
-
MapEntry& entry = it->value;
- ASSERT(entry.count);
+ ASSERT_WITH_SECURITY_IMPLICATION(entry.count);
if (entry.count == 1) {
- ASSERT(!entry.element || entry.element == &element);
+ ASSERT_WITH_SECURITY_IMPLICATION(!entry.element || entry.element == &element);
m_map.remove(it);
} else {
if (entry.element == &element)
@@ -133,14 +136,19 @@
MapEntry& entry = it->value;
ASSERT(entry.count);
- if (entry.element)
+ if (entry.element) {
+ ASSERT_WITH_SECURITY_IMPLICATION(entry.element->isInTreeScope());
+ ASSERT_WITH_SECURITY_IMPLICATION(&entry.element->treeScope() == &scope);
return entry.element;
+ }
// We know there's at least one node that matches; iterate to find the first one.
for (Element* element = ElementTraversal::firstWithin(scope.rootNode()); element; element = ElementTraversal::next(element)) {
if (!keyMatches(key, element))
continue;
entry.element = element;
+ ASSERT_WITH_SECURITY_IMPLICATION(element->isInTreeScope());
+ ASSERT_WITH_SECURITY_IMPLICATION(&element->treeScope() == &scope);
return element;
}
ASSERT_NOT_REACHED();
@@ -196,7 +204,7 @@
return 0;
MapEntry& entry = it->value;
- ASSERT(entry.count);
+ ASSERT_WITH_SECURITY_IMPLICATION(entry.count);
if (!entry.count)
return 0;
Modified: trunk/Source/WebCore/dom/DocumentOrderedMap.h (159488 => 159489)
--- trunk/Source/WebCore/dom/DocumentOrderedMap.h 2013-11-19 09:28:38 UTC (rev 159488)
+++ trunk/Source/WebCore/dom/DocumentOrderedMap.h 2013-11-19 09:43:22 UTC (rev 159489)
@@ -46,7 +46,7 @@
class DocumentOrderedMap {
public:
- void add(const AtomicStringImpl&, Element&);
+ void add(const AtomicStringImpl&, Element&, const TreeScope&);
void remove(const AtomicStringImpl&, Element&);
void clear();
Modified: trunk/Source/WebCore/dom/TreeScope.cpp (159488 => 159489)
--- trunk/Source/WebCore/dom/TreeScope.cpp 2013-11-19 09:28:38 UTC (rev 159488)
+++ trunk/Source/WebCore/dom/TreeScope.cpp 2013-11-19 09:43:22 UTC (rev 159489)
@@ -154,7 +154,7 @@
{
if (!m_elementsById)
m_elementsById = adoptPtr(new DocumentOrderedMap);
- m_elementsById->add(elementId, element);
+ m_elementsById->add(elementId, element, *this);
m_idTargetObserverRegistry->notifyObservers(elementId);
}
@@ -179,7 +179,7 @@
{
if (!m_elementsByName)
m_elementsByName = adoptPtr(new DocumentOrderedMap);
- m_elementsByName->add(name, element);
+ m_elementsByName->add(name, element, *this);
}
void TreeScope::removeElementByName(const AtomicStringImpl& name, Element& element)
@@ -207,7 +207,7 @@
return;
if (!m_imageMapsByName)
m_imageMapsByName = adoptPtr(new DocumentOrderedMap);
- m_imageMapsByName->add(*name, imageMap);
+ m_imageMapsByName->add(*name, imageMap, *this);
}
void TreeScope::removeImageMap(HTMLMapElement& imageMap)
@@ -276,7 +276,7 @@
void TreeScope::addLabel(const AtomicStringImpl& forAttributeValue, HTMLLabelElement& element)
{
ASSERT(m_labelsByForAttribute);
- m_labelsByForAttribute->add(forAttributeValue, element);
+ m_labelsByForAttribute->add(forAttributeValue, element, *this);
}
void TreeScope::removeLabel(const AtomicStringImpl& forAttributeValue, HTMLLabelElement& element)
Modified: trunk/Source/WebCore/html/HTMLDocument.cpp (159488 => 159489)
--- trunk/Source/WebCore/html/HTMLDocument.cpp 2013-11-19 09:28:38 UTC (rev 159488)
+++ trunk/Source/WebCore/html/HTMLDocument.cpp 2013-11-19 09:43:22 UTC (rev 159489)
@@ -357,7 +357,7 @@
void HTMLDocument::addDocumentNamedItem(const AtomicStringImpl& name, Element& item)
{
- m_documentNamedItem.add(name, item);
+ m_documentNamedItem.add(name, item, *this);
}
void HTMLDocument::removeDocumentNamedItem(const AtomicStringImpl& name, Element& item)
@@ -367,7 +367,7 @@
void HTMLDocument::addWindowNamedItem(const AtomicStringImpl& name, Element& item)
{
- m_windowNamedItem.add(name, item);
+ m_windowNamedItem.add(name, item, *this);
}
void HTMLDocument::removeWindowNamedItem(const AtomicStringImpl& name, Element& item)
Modified: trunk/Source/WebCore/html/HTMLImageElement.cpp (159488 => 159489)
--- trunk/Source/WebCore/html/HTMLImageElement.cpp 2013-11-19 09:28:38 UTC (rev 159488)
+++ trunk/Source/WebCore/html/HTMLImageElement.cpp 2013-11-19 09:43:22 UTC (rev 159489)
@@ -218,13 +218,13 @@
m_form->registerImgElement(this);
}
- if (insertionPoint.inDocument() && !m_lowercasedUsemap.isNull())
- document().addImageElementByLowercasedUsemap(*m_lowercasedUsemap.impl(), *this);
-
// Insert needs to complete first, before we start updating the loader. Loader dispatches events which could result
// in callbacks back to this node.
Node::InsertionNotificationRequest insertNotificationRequest = HTMLElement::insertedInto(insertionPoint);
+ if (insertionPoint.inDocument() && !m_lowercasedUsemap.isNull())
+ document().addImageElementByLowercasedUsemap(*m_lowercasedUsemap.impl(), *this);
+
// If we have been inserted from a renderer-less document,
// our loader may have not fetched the image, so do it now.
if (insertionPoint.inDocument() && !m_imageLoader.image())
Modified: trunk/Source/WebCore/html/HTMLMapElement.cpp (159488 => 159489)
--- trunk/Source/WebCore/html/HTMLMapElement.cpp 2013-11-19 09:28:38 UTC (rev 159488)
+++ trunk/Source/WebCore/html/HTMLMapElement.cpp 2013-11-19 09:43:22 UTC (rev 159489)
@@ -118,9 +118,10 @@
Node::InsertionNotificationRequest HTMLMapElement::insertedInto(ContainerNode& insertionPoint)
{
+ Node::InsertionNotificationRequest request = HTMLElement::insertedInto(insertionPoint);
if (insertionPoint.inDocument())
treeScope().addImageMap(*this);
- return HTMLElement::insertedInto(insertionPoint);
+ return request;
}
void HTMLMapElement::removedFrom(ContainerNode& insertionPoint)
