Bug 1470367 - Refactor nsTreeSanitizer::SanitizeAttributes to accept a struct. r?hsivonen draft
authorXidorn Quan <me@upsuper.org>
Fri, 22 Jun 2018 14:58:40 +1000
changeset 809455 24e362de3c0a7cc688528d5d2e34960160f3b8b7
parent 809454 c817a5c4f6ae8687a9374b4bc4d1d7c3ed572880
child 809456 70056a83e7f3d22f8105d6d67b5c0020ebf258a9
push id113682
push userxquan@mozilla.com
push dateFri, 22 Jun 2018 05:05:39 +0000
reviewershsivonen
bugs1470367
milestone62.0a1
Bug 1470367 - Refactor nsTreeSanitizer::SanitizeAttributes to accept a struct. r?hsivonen MozReview-Commit-ID: FvWpzeSo38d
dom/base/nsTreeSanitizer.cpp
dom/base/nsTreeSanitizer.h
--- a/dom/base/nsTreeSanitizer.cpp
+++ b/dom/base/nsTreeSanitizer.cpp
@@ -1149,31 +1149,27 @@ nsTreeSanitizer::SanitizeStyleSheet(cons
   if (didSanitize && mLogRemovals) {
     LogMessage("Removed some rules and/or properties from stylesheet.", aDocument);
   }
   return didSanitize;
 }
 
 void
 nsTreeSanitizer::SanitizeAttributes(mozilla::dom::Element* aElement,
-                                    AtomsTable* aAllowed,
-                                    const nsStaticAtom* const* aURLs,
-                                    bool aAllowXLink,
-                                    bool aAllowStyle,
-                                    bool aAllowDangerousSrc)
+                                    AllowedAttributes aAllowed)
 {
   uint32_t ac = aElement->GetAttrCount();
 
   for (int32_t i = ac - 1; i >= 0; --i) {
     const nsAttrName* attrName = aElement->GetAttrNameAt(i);
     int32_t attrNs = attrName->NamespaceID();
     RefPtr<nsAtom> attrLocal = attrName->LocalName();
 
     if (kNameSpaceID_None == attrNs) {
-      if (aAllowStyle && nsGkAtoms::style == attrLocal) {
+      if (aAllowed.mStyle && nsGkAtoms::style == attrLocal) {
         nsAutoString value;
         aElement->GetAttr(attrNs, attrLocal, value);
         nsIDocument* document = aElement->OwnerDoc();
         RefPtr<URLExtraData> urlExtra(aElement->GetURLDataForStyleAttr());
         RefPtr<DeclarationBlock> decl =
           DeclarationBlock::FromCssText(
             value,
             urlExtra,
@@ -1190,37 +1186,37 @@ nsTreeSanitizer::SanitizeAttributes(mozi
             if (mLogRemovals) {
               LogMessage("Removed -moz-binding styling from element style attribute.",
                          aElement->OwnerDoc(), aElement);
             }
           }
         }
         continue;
       }
-      if (aAllowDangerousSrc && nsGkAtoms::src == attrLocal) {
+      if (aAllowed.mDangerousSrc && nsGkAtoms::src == attrLocal) {
         continue;
       }
-      if (IsURL(aURLs, attrLocal)) {
+      if (IsURL(aAllowed.mURLs, 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;
         }
         // else fall through to see if there's another reason to drop this
         // attribute (in particular if the attribute is background="" on an
         // HTML element)
       }
       if (!mDropNonCSSPresentation &&
-          (aAllowed == sAttributesHTML) && // element is HTML
+          (aAllowed.mNames == sAttributesHTML) && // element is HTML
           sPresAttributesHTML->Contains(attrLocal)) {
         continue;
       }
-      if (aAllowed->Contains(attrLocal) &&
+      if (aAllowed.mNames->Contains(attrLocal) &&
           !((attrLocal == nsGkAtoms::rel &&
              aElement->IsHTMLElement(nsGkAtoms::link)) ||
             (!mFullDocument &&
              attrLocal == nsGkAtoms::name &&
              aElement->IsHTMLElement(nsGkAtoms::meta)))) {
         // name="" and rel="" are whitelisted, but treat them as blacklisted
         // for <meta name> (fragment case) and <link rel> (all cases) to avoid
         // document-wide metadata or styling overrides with non-conforming
@@ -1246,17 +1242,17 @@ nsTreeSanitizer::SanitizeAttributes(mozi
           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) {
+    } else if (aAllowed.mXLink && kNameSpaceID_XLink == attrNs) {
       if (nsGkAtoms::href == 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;
@@ -1423,30 +1419,27 @@ nsTreeSanitizer::SanitizeChildren(nsINod
         } else {
           // If the node had non-text child nodes, this operation zaps those.
           //XXXgijs: if we're logging, we should theoretically report about
           // this, but this way of removing those items doesn't allow for that
           // to happen. Seems less likely to be a problem for actual chrome
           // consumers though.
           nsContentUtils::SetNodeTextContent(node, styleText, true);
         }
+        AllowedAttributes allowed;
+        allowed.mStyle = mAllowStyles;
         if (ns == kNameSpaceID_XHTML) {
-          SanitizeAttributes(elt,
-                             sAttributesHTML,
-                             kURLAttributesHTML,
-                             false,
-                             mAllowStyles,
-                             false);
+          allowed.mNames = sAttributesHTML;
+          allowed.mURLs = kURLAttributesHTML;
+          SanitizeAttributes(elt, allowed);
         } else {
-          SanitizeAttributes(elt,
-                             sAttributesSVG,
-                             kURLAttributesSVG,
-                             true,
-                             mAllowStyles,
-                             false);
+          allowed.mNames = sAttributesSVG;
+          allowed.mURLs = kURLAttributesSVG;
+          allowed.mXLink = true;
+          SanitizeAttributes(elt, allowed);
         }
         node = node->GetNextNonChildNode(aRoot);
         continue;
       }
       if (MustFlatten(ns, localName)) {
         if (mLogRemovals) {
           LogMessage("Flattening unsafe node (descendants are preserved).",
                      elt->OwnerDoc(), elt);
@@ -1466,37 +1459,34 @@ nsTreeSanitizer::SanitizeChildren(nsINod
         node->RemoveFromParent();
         node = next;
         continue;
       }
       NS_ASSERTION(ns == kNameSpaceID_XHTML ||
                    ns == kNameSpaceID_SVG ||
                    ns == kNameSpaceID_MathML,
           "Should have only HTML, MathML or SVG here!");
+      AllowedAttributes allowed;
       if (ns == kNameSpaceID_XHTML) {
-        SanitizeAttributes(elt,
-                           sAttributesHTML,
-                           kURLAttributesHTML,
-                           false, mAllowStyles,
-                           (nsGkAtoms::img == localName) &&
-                           !mCidEmbedsOnly);
+        allowed.mNames = sAttributesHTML;
+        allowed.mURLs = kURLAttributesHTML;
+        allowed.mStyle = mAllowStyles;
+        allowed.mDangerousSrc = nsGkAtoms::img == localName && !mCidEmbedsOnly;
+        SanitizeAttributes(elt, allowed);
       } else if (ns == kNameSpaceID_SVG) {
-        SanitizeAttributes(elt,
-                           sAttributesSVG,
-                           kURLAttributesSVG,
-                           true,
-                           mAllowStyles,
-                           false);
+        allowed.mNames = sAttributesSVG;
+        allowed.mURLs = kURLAttributesSVG;
+        allowed.mXLink = true;
+        allowed.mStyle = mAllowStyles;
+        SanitizeAttributes(elt, allowed);
       } else {
-        SanitizeAttributes(elt,
-                           sAttributesMathML,
-                           kURLAttributesMathML,
-                           true,
-                           false,
-                           false);
+        allowed.mNames = sAttributesMathML;
+        allowed.mURLs = kURLAttributesMathML;
+        allowed.mXLink = true;
+        SanitizeAttributes(elt, allowed);
       }
       node = node->GetNextNode(aRoot);
       continue;
     }
     NS_ASSERTION(!node->GetFirstChild(), "How come non-element node had kids?");
     nsIContent* next = node->GetNextNonChildNode(aRoot);
     if (!mAllowComments && node->IsComment()) {
       node->RemoveFromParent();
--- a/dom/base/nsTreeSanitizer.h
+++ b/dom/base/nsTreeSanitizer.h
@@ -131,35 +131,43 @@ class MOZ_STACK_CLASS nsTreeSanitizer {
      * of URL attribute names.
      * @param aURLs the list of URL attribute names
      * @param aLocalName the name to search on the list
      * @return true if aLocalName is on the aURLs list and false otherwise
      */
     bool IsURL(const nsStaticAtom* const* aURLs, nsAtom* aLocalName);
 
     /**
+     * Struct for what attributes and their values are allowed.
+     */
+    struct AllowedAttributes
+    {
+      // The whitelist of permitted local names to use.
+      AtomsTable* mNames = nullptr;
+      // The local names of URL-valued attributes for URL checking.
+      const nsStaticAtom* const* mURLs = nullptr;
+      // Whether XLink attributes are allowed.
+      bool mXLink = false;
+      // Whether the style attribute is allowed.
+      bool mStyle = false;
+      // Whether to leave the value of the src attribute unsanitized.
+      bool mDangerousSrc = false;
+    };
+
+    /**
      * Removes dangerous attributes from the element. If the style attribute
      * is allowed, its value is sanitized. The values of URL attributes are
      * sanitized, except src isn't sanitized when it is allowed to remain
      * potentially dangerous.
      *
      * @param aElement the element whose attributes should be sanitized
-     * @param aAllowed the whitelist of permitted local names to use
-     * @param aURLs the local names of URL-valued attributes
-     * @param aAllowXLink whether XLink attributes are allowed
-     * @param aAllowStyle whether the style attribute is allowed
-     * @param aAllowDangerousSrc whether to leave the value of the src
-     *                           attribute unsanitized
+     * @param aAllowed options for sanitizing attributes
      */
     void SanitizeAttributes(mozilla::dom::Element* aElement,
-                            AtomsTable* aAllowed,
-                            const nsStaticAtom* const* aURLs,
-                            bool aAllowXLink,
-                            bool aAllowStyle,
-                            bool aAllowDangerousSrc);
+                            AllowedAttributes aAllowed);
 
     /**
      * 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