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 73711 987e42035f38c6df919f0884617ac085842695f9
parent 73710 8a54bd8aa5c88f9aa6cdacaa088ea90193361c67
child 73712 78efa76773a68ec89e026e1ad31296205f0e5520
push id2
push userbsmedberg@mozilla.com
push dateFri, 19 Aug 2011 14:38:13 +0000
reviewersbzbarsky
bugs675916
milestone8.0a1
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),