Bug 675916 - Restart iteration over attributes in the sanitizer when URL check ends up removing an attribute. r=bzbarsky.
authorHenri Sivonen <hsivonen@iki.fi>
Tue, 02 Aug 2011 20:45:38 +0300
changeset 73679 987e42035f38c6df919f0884617ac085842695f9
parent 73678 8a54bd8aa5c88f9aa6cdacaa088ea90193361c67
child 73680 78efa76773a68ec89e026e1ad31296205f0e5520
push id916
push userhsivonen@iki.fi
push dateTue, 02 Aug 2011 17:46:35 +0000
treeherdermozilla-inbound@8e4e181a9ce9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs675916
milestone8.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 675916 - Restart iteration over attributes in the sanitizer when URL check ends up removing an attribute. r=bzbarsky.
content/base/public/nsTreeSanitizer.h
content/base/src/nsTreeSanitizer.cpp
--- a/content/base/public/nsTreeSanitizer.h
+++ b/content/base/public/nsTreeSanitizer.h
@@ -132,20 +132,21 @@ class NS_STACK_CLASS nsTreeSanitizer {
 
     /**
      * Remove the named URL attribute from the element if the URL fails a
      * security check.
      *
      * @param aElement the element whose attribute to possibly modify
      * @param aNamespace the namespace of the URL attribute
      * @param aLocalName the local name of the URL attribute
+     * @return true if the attribute was removed and false otherwise
      */
-    void SanitizeURL(mozilla::dom::Element* aElement,
-                     PRInt32 aNamespace,
-                     nsIAtom* aLocalName);
+    PRBool SanitizeURL(mozilla::dom::Element* aElement,
+                       PRInt32 aNamespace,
+                       nsIAtom* aLocalName);
 
     /**
      * Checks a style rule for the presence of the 'binding' CSS property and
      * removes that property from the rule and reserializes in case the
      * property was found.
      *
      * @param aRule The style rule to check
      * @param aRuleText the serialized mutated rule if the method returns true
--- a/content/base/src/nsTreeSanitizer.cpp
+++ b/content/base/src/nsTreeSanitizer.cpp
@@ -1222,17 +1222,22 @@ nsTreeSanitizer::SanitizeAttributes(mozi
           }
         }
         continue;
       }
       if (aAllowDangerousSrc && nsGkAtoms::src == attrLocal) {
         continue;
       }
       if (IsURL(aURLs, attrLocal)) {
-        SanitizeURL(aElement, attrNs, attrLocal);
+        if (SanitizeURL(aElement, attrNs, attrLocal)) {
+          // in case the attribute removal shuffled the attribute order, start
+          // the loop again.
+          --ac;
+          i = ac; // i will be decremented immediately thanks to the for loop
+        }
         continue;
       }
       if (aAllowed->GetEntry(attrLocal) &&
           !(attrLocal == nsGkAtoms::rel &&
             aElement->IsHTML(nsGkAtoms::link)) &&
           !(attrLocal == nsGkAtoms::name &&
             aElement->IsHTML(nsGkAtoms::meta))) {
         // name="" and rel="" are whitelisted, but treat them as blacklisted
@@ -1247,26 +1252,36 @@ nsTreeSanitizer::SanitizeAttributes(mozi
       if (*localStr == '_' || (attrLocal->GetLength() > 5 && localStr[0] == 'd'
           && localStr[1] == 'a' && localStr[2] == 't' && localStr[3] == 'a'
           && localStr[4] == '-')) {
         continue;
       }
       // else not allowed
     } else if (kNameSpaceID_XML == attrNs) {
       if (nsGkAtoms::base == attrLocal) {
-        SanitizeURL(aElement, attrNs, attrLocal);
+        if (SanitizeURL(aElement, attrNs, attrLocal)) {
+          // in case the attribute removal shuffled the attribute order, start
+          // the loop again.
+          --ac;
+          i = ac; // i will be decremented immediately thanks to the for loop
+        }
         continue;
       }
       if (nsGkAtoms::lang == attrLocal || nsGkAtoms::space == attrLocal) {
         continue;
       }
       // else not allowed
     } else if (aAllowXLink && kNameSpaceID_XLink == attrNs) {
       if (nsGkAtoms::href == attrLocal) {
-        SanitizeURL(aElement, attrNs, attrLocal);
+        if (SanitizeURL(aElement, attrNs, attrLocal)) {
+          // in case the attribute removal shuffled the attribute order, start
+          // the loop again.
+          --ac;
+          i = ac; // i will be decremented immediately thanks to the for loop
+        }
         continue;
       }
       if (nsGkAtoms::type == attrLocal || nsGkAtoms::title == attrLocal
           || nsGkAtoms::show == attrLocal || nsGkAtoms::actuate == attrLocal) {
         continue;
       }
       // else not allowed
     }
@@ -1283,17 +1298,17 @@ nsTreeSanitizer::SanitizeAttributes(mozi
       aElement->IsHTML(nsGkAtoms::audio)) {
     aElement->SetAttr(kNameSpaceID_None,
                       nsGkAtoms::controls,
                       EmptyString(),
                       PR_FALSE);
   }
 }
 
-void
+PRBool
 nsTreeSanitizer::SanitizeURL(mozilla::dom::Element* aElement,
                              PRInt32 aNamespace,
                              nsIAtom* aLocalName)
 {
   nsAutoString value;
   aElement->GetAttr(aNamespace, aLocalName, value);
 
   // Get value and remove mandatory quotes
@@ -1307,17 +1322,19 @@ nsTreeSanitizer::SanitizeURL(mozilla::do
   nsCOMPtr<nsIURI> baseURI = aElement->GetBaseURI();
   nsCOMPtr<nsIURI> attrURI;
   nsresult rv = NS_NewURI(getter_AddRefs(attrURI), v, nsnull, baseURI);
   if (NS_SUCCEEDED(rv)) {
     rv = secMan->CheckLoadURIWithPrincipal(sNullPrincipal, attrURI, flags);
   }
   if (NS_FAILED(rv)) {
     aElement->UnsetAttr(aNamespace, aLocalName, PR_FALSE);
+    return PR_TRUE;
   }
+  return PR_FALSE;
 }
 
 void
 nsTreeSanitizer::Sanitize(nsIContent* aFragment) {
   // If you want to relax these preconditions, be sure to check the code in
   // here that notifies / does not notify or that fires mutation events if
   // in tree.
   NS_PRECONDITION(aFragment->IsNodeOfType(nsINode::eDOCUMENT_FRAGMENT),