Bug 1444394 - Remove Element::UnsafeSetInnerHTML. r=bz,kmag draft
authorJohann Hofmann <jhofmann@mozilla.com>
Mon, 28 May 2018 22:55:52 +0200
changeset 802007 5def3abb50ed8f1b43e17072088e38a44394488b
parent 801916 3931f461c8e8668a264d52b51a4524aac39a7a16
push id111804
push userjhofmann@mozilla.com
push dateThu, 31 May 2018 08:41:37 +0000
reviewersbz, kmag
bugs1444394, 1444395
milestone62.0a1
Bug 1444394 - Remove Element::UnsafeSetInnerHTML. r=bz,kmag The last remaining user is already turned off and being removed in bug 1444395 so that we can finally remove this unsafe code and sleep a little better knowing that XSS through markup injections will be impossible in chrome contexts. MozReview-Commit-ID: KcZq8fRPiD4
dom/base/Element.cpp
dom/base/Element.h
dom/base/FragmentOrElement.cpp
dom/base/FragmentOrElement.h
dom/base/nsContentUtils.cpp
dom/base/nsContentUtils.h
dom/webidl/Element.webidl
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -3778,22 +3778,16 @@ Element::GetInnerHTML(nsAString& aInnerH
 
 void
 Element::SetInnerHTML(const nsAString& aInnerHTML, nsIPrincipal* aSubjectPrincipal, ErrorResult& aError)
 {
   SetInnerHTMLInternal(aInnerHTML, aError);
 }
 
 void
-Element::UnsafeSetInnerHTML(const nsAString& aInnerHTML, ErrorResult& aError)
-{
-  SetInnerHTMLInternal(aInnerHTML, aError, true);
-}
-
-void
 Element::GetOuterHTML(nsAString& aOuterHTML)
 {
   GetMarkup(true, aOuterHTML);
 }
 
 void
 Element::SetOuterHTML(const nsAString& aOuterHTML, ErrorResult& aError)
 {
--- a/dom/base/Element.h
+++ b/dom/base/Element.h
@@ -1364,17 +1364,16 @@ public:
   void GetAnimations(const AnimationFilter& filter,
                      nsTArray<RefPtr<Animation>>& aAnimations);
   static void GetAnimationsUnsorted(Element* aElement,
                                     CSSPseudoElementType aPseudoType,
                                     nsTArray<RefPtr<Animation>>& aAnimations);
 
   virtual void GetInnerHTML(nsAString& aInnerHTML, OOMReporter& aError);
   virtual void SetInnerHTML(const nsAString& aInnerHTML, nsIPrincipal* aSubjectPrincipal, ErrorResult& aError);
-  void UnsafeSetInnerHTML(const nsAString& aInnerHTML, ErrorResult& aError);
   void GetOuterHTML(nsAString& aOuterHTML);
   void SetOuterHTML(const nsAString& aOuterHTML, ErrorResult& aError);
   void InsertAdjacentHTML(const nsAString& aPosition, const nsAString& aText,
                           ErrorResult& aError);
 
   //----------------------------------------
 
   /**
--- a/dom/base/FragmentOrElement.cpp
+++ b/dom/base/FragmentOrElement.cpp
@@ -2271,18 +2271,17 @@ ContainsMarkup(const nsAString& aStr)
     }
     ++start;
   }
 
   return false;
 }
 
 void
-FragmentOrElement::SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult& aError,
-                                        bool aNeverSanitize)
+FragmentOrElement::SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult& aError)
 {
   FragmentOrElement* target = this;
   // Handle template case.
   if (nsNodeUtils::IsTemplateElement(target)) {
     DocumentFragment* frag =
       static_cast<HTMLTemplateElement*>(target)->Content();
     MOZ_ASSERT(frag);
     target = frag;
@@ -2324,37 +2323,33 @@ FragmentOrElement::SetInnerHTMLInternal(
   int32_t contextNameSpaceID = GetNameSpaceID();
 
   if (ShadowRoot* shadowRoot = ShadowRoot::FromNode(this)) {
     // Fix up the context to be the host of the ShadowRoot.
     contextLocalName = shadowRoot->GetHost()->NodeInfo()->NameAtom();
     contextNameSpaceID = shadowRoot->GetHost()->GetNameSpaceID();
   }
 
-  auto sanitize = (aNeverSanitize ? nsContentUtils::NeverSanitize
-                                  : nsContentUtils::SanitizeSystemPrivileged);
-
   if (doc->IsHTMLDocument()) {
     int32_t oldChildCount = target->GetChildCount();
     aError = nsContentUtils::ParseFragmentHTML(aInnerHTML,
                                                target,
                                                contextLocalName,
                                                contextNameSpaceID,
                                                doc->GetCompatibilityMode() ==
                                                  eCompatibility_NavQuirks,
-                                               true,
-                                               sanitize);
+                                               true);
     mb.NodesAdded();
     // HTML5 parser has notified, but not fired mutation events.
     nsContentUtils::FireMutationEventsForDirectParsing(doc, target,
                                                        oldChildCount);
   } else {
     RefPtr<DocumentFragment> df =
       nsContentUtils::CreateContextualFragment(target, aInnerHTML, true,
-                                               sanitize, aError);
+                                               aError);
     if (!aError.Failed()) {
       // Suppress assertion about node removal mutation events that can't have
       // listeners anyway, because no one has had the chance to register mutation
       // listeners on the fragment that comes from the parser.
       nsAutoScriptBlockerSuppressNodeRemoved scriptBlocker;
 
       static_cast<nsINode*>(target)->AppendChild(*df, aError);
       mb.NodesAdded();
--- a/dom/base/FragmentOrElement.h
+++ b/dom/base/FragmentOrElement.h
@@ -263,18 +263,17 @@ public:
     /**
      * An object implementing the .classList property for this element.
      */
     RefPtr<nsDOMTokenList> mClassList;
   };
 
 protected:
   void GetMarkup(bool aIncludeSelf, nsAString& aMarkup);
-  void SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult& aError,
-                            bool aNeverSanitize = false);
+  void SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult& aError);
 
   // Override from nsINode
   nsIContent::nsContentSlots* CreateSlots() override
   {
     return new nsDOMSlots();
   }
 
   nsIContent::nsExtendedContentSlots* CreateExtendedSlots() final
--- a/dom/base/nsContentUtils.cpp
+++ b/dom/base/nsContentUtils.cpp
@@ -4901,17 +4901,16 @@ nsContentUtils::IsValidNodeName(nsAtom *
   return aPrefix != nsGkAtoms::xmlns &&
          (aNamespaceID == kNameSpaceID_XML || aPrefix != nsGkAtoms::xml);
 }
 
 already_AddRefed<DocumentFragment>
 nsContentUtils::CreateContextualFragment(nsINode* aContextNode,
                                          const nsAString& aFragment,
                                          bool aPreventScriptExecution,
-                                         SanitizeFragments aSanitize,
                                          ErrorResult& aRv)
 {
   if (!aContextNode) {
     aRv.Throw(NS_ERROR_INVALID_ARG);
     return nullptr;
   }
 
   // If we don't have a document here, we can't get the right security context
@@ -4937,26 +4936,24 @@ nsContentUtils::CreateContextualFragment
     }
 
     if (contextAsContent && !contextAsContent->IsHTMLElement(nsGkAtoms::html)) {
       aRv = ParseFragmentHTML(aFragment, frag,
                               contextAsContent->NodeInfo()->NameAtom(),
                               contextAsContent->GetNameSpaceID(),
                               (document->GetCompatibilityMode() ==
                                eCompatibility_NavQuirks),
-                              aPreventScriptExecution,
-                              aSanitize);
+                              aPreventScriptExecution);
     } else {
       aRv = ParseFragmentHTML(aFragment, frag,
                               nsGkAtoms::body,
                               kNameSpaceID_XHTML,
                               (document->GetCompatibilityMode() ==
                                eCompatibility_NavQuirks),
-                              aPreventScriptExecution,
-                              aSanitize);
+                              aPreventScriptExecution);
     }
 
     return frag.forget();
   }
 
   AutoTArray<nsString, 32> tagStack;
   nsAutoString uriStr, nameStr;
   nsCOMPtr<nsIContent> content = do_QueryInterface(aContextNode);
@@ -5010,18 +5007,17 @@ nsContentUtils::CreateContextualFragment
       }
     }
 
     content = content->GetParent();
   }
 
   RefPtr<DocumentFragment> frag;
   aRv = ParseFragmentXML(aFragment, document, tagStack,
-                         aPreventScriptExecution, getter_AddRefs(frag),
-                         aSanitize);
+                         aPreventScriptExecution, getter_AddRefs(frag));
   return frag.forget();
 }
 
 /* static */
 void
 nsContentUtils::DropFragmentParsers()
 {
   NS_IF_RELEASE(sHTMLFragmentParser);
@@ -5038,18 +5034,17 @@ nsContentUtils::XPCOMShutdown()
 
 /* static */
 nsresult
 nsContentUtils::ParseFragmentHTML(const nsAString& aSourceBuffer,
                                   nsIContent* aTargetNode,
                                   nsAtom* aContextLocalName,
                                   int32_t aContextNamespace,
                                   bool aQuirks,
-                                  bool aPreventScriptExecution,
-                                  SanitizeFragments aSanitize)
+                                  bool aPreventScriptExecution)
 {
   AutoTimelineMarker m(aTargetNode->OwnerDoc()->GetDocShell(), "Parse HTML");
 
   if (nsContentUtils::sFragmentParsingActive) {
     NS_NOTREACHED("Re-entrant fragment parsing attempted.");
     return NS_ERROR_DOM_INVALID_STATE_ERR;
   }
   mozilla::AutoRestore<bool> guard(nsContentUtils::sFragmentParsingActive);
@@ -5059,18 +5054,17 @@ nsContentUtils::ParseFragmentHTML(const 
     // Now sHTMLFragmentParser owns the object
   }
 
   nsIContent* target = aTargetNode;
 
   // If this is a chrome-privileged document, create a fragment first, and
   // sanitize it before insertion.
   RefPtr<DocumentFragment> fragment;
-  if (aSanitize != NeverSanitize &&
-      IsSystemPrincipal(aTargetNode->NodePrincipal())) {
+  if (IsSystemPrincipal(aTargetNode->NodePrincipal())) {
     fragment = new DocumentFragment(aTargetNode->OwnerDoc()->NodeInfoManager());
     target = fragment;
   }
 
   nsresult rv =
     sHTMLFragmentParser->ParseFragment(aSourceBuffer,
                                        target,
                                        aContextLocalName,
@@ -5123,18 +5117,17 @@ nsContentUtils::ParseDocumentHTML(const 
 }
 
 /* static */
 nsresult
 nsContentUtils::ParseFragmentXML(const nsAString& aSourceBuffer,
                                  nsIDocument* aDocument,
                                  nsTArray<nsString>& aTagStack,
                                  bool aPreventScriptExecution,
-                                 DocumentFragment** aReturn,
-                                 SanitizeFragments aSanitize)
+                                 DocumentFragment** aReturn)
 {
   AutoTimelineMarker m(aDocument->GetDocShell(), "Parse XML");
 
   if (nsContentUtils::sFragmentParsingActive) {
     NS_NOTREACHED("Re-entrant fragment parsing attempted.");
     return NS_ERROR_DOM_INVALID_STATE_ERR;
   }
   mozilla::AutoRestore<bool> guard(nsContentUtils::sFragmentParsingActive);
@@ -5167,18 +5160,17 @@ nsContentUtils::ParseFragmentXML(const n
 
   rv = sXMLFragmentSink->FinishFragmentParsing(aReturn);
 
   sXMLFragmentParser->Reset();
   NS_ENSURE_SUCCESS(rv, rv);
 
   // If this is a chrome-privileged document, sanitize the fragment before
   // returning.
-  if (aSanitize != NeverSanitize &&
-      IsSystemPrincipal(aDocument->NodePrincipal())) {
+  if (IsSystemPrincipal(aDocument->NodePrincipal())) {
     // Don't fire mutation events for nodes removed by the sanitizer.
     nsAutoScriptBlockerSuppressNodeRemoved scriptBlocker;
 
     nsTreeSanitizer sanitizer(nsIParserUtils::SanitizerAllowStyle |
                               nsIParserUtils::SanitizerAllowComments |
                               nsIParserUtils::SanitizerDropForms |
                               nsIParserUtils::SanitizerLogRemovals);
     sanitizer.Sanitize(*aReturn);
--- a/dom/base/nsContentUtils.h
+++ b/dom/base/nsContentUtils.h
@@ -1588,95 +1588,85 @@ public:
    *
    * @param aLocalname localname of the node
    * @param aPrefix prefix of the node
    * @param aNamespaceID namespace of the node
    */
   static bool IsValidNodeName(nsAtom *aLocalName, nsAtom *aPrefix,
                                 int32_t aNamespaceID);
 
-  enum SanitizeFragments {
-    SanitizeSystemPrivileged,
-    NeverSanitize,
-  };
-
   /**
    * Creates a DocumentFragment from text using a context node to resolve
    * namespaces.
    *
+   * Please note that for safety reasons, if the node principal of
+   * aContextNode is the system principal, this function will automatically
+   * sanitize its input using nsTreeSanitizer.
+   *
    * Note! In the HTML case with the HTML5 parser enabled, this is only called
    * from Range.createContextualFragment() and the implementation here is
    * quirky accordingly (html context node behaves like a body context node).
    * If you don't want that quirky behavior, don't use this method as-is!
    *
    * @param aContextNode the node which is used to resolve namespaces
    * @param aFragment the string which is parsed to a DocumentFragment
    * @param aReturn the resulting fragment
    * @param aPreventScriptExecution whether to mark scripts as already started
-   * @param aSanitize whether the fragment should be sanitized prior to
-   *        injection
    */
   static already_AddRefed<mozilla::dom::DocumentFragment>
   CreateContextualFragment(nsINode* aContextNode, const nsAString& aFragment,
                            bool aPreventScriptExecution,
-                           SanitizeFragments aSanitize,
                            mozilla::ErrorResult& aRv);
-  static already_AddRefed<mozilla::dom::DocumentFragment>
-  CreateContextualFragment(nsINode* aContextNode, const nsAString& aFragment,
-                           bool aPreventScriptExecution,
-                           mozilla::ErrorResult& aRv)
-  {
-    return CreateContextualFragment(aContextNode, aFragment, aPreventScriptExecution,
-                                    SanitizeSystemPrivileged, aRv);
-  }
 
   /**
    * Invoke the fragment parsing algorithm (innerHTML) using the HTML parser.
    *
+   * Please note that for safety reasons, if the node principal of aTargetNode
+   * is the system principal, this function will automatically sanitize its
+   * input using nsTreeSanitizer.
+   *
    * @param aSourceBuffer the string being set as innerHTML
    * @param aTargetNode the target container
    * @param aContextLocalName local name of context node
    * @param aContextNamespace namespace of context node
    * @param aQuirks true to make <table> not close <p>
    * @param aPreventScriptExecution true to prevent scripts from executing;
    *        don't set to false when parsing into a target node that has been
    *        bound to tree.
-   * @param aSanitize whether the fragment should be sanitized prior to
-   *        injection
    * @return NS_ERROR_DOM_INVALID_STATE_ERR if a re-entrant attempt to parse
    *         fragments is made, NS_ERROR_OUT_OF_MEMORY if aSourceBuffer is too
    *         long and NS_OK otherwise.
    */
   static nsresult ParseFragmentHTML(const nsAString& aSourceBuffer,
                                     nsIContent* aTargetNode,
                                     nsAtom* aContextLocalName,
                                     int32_t aContextNamespace,
                                     bool aQuirks,
-                                    bool aPreventScriptExecution,
-                                    SanitizeFragments aSanitize = SanitizeSystemPrivileged);
+                                    bool aPreventScriptExecution);
 
   /**
    * Invoke the fragment parsing algorithm (innerHTML) using the XML parser.
    *
+   * Please note that for safety reasons, if the node principal of aDocument
+   * is the system principal, this function will automatically sanitize its
+   * input using nsTreeSanitizer.
+   *
    * @param aSourceBuffer the string being set as innerHTML
-   * @param aTargetNode the target container
+   * @param aDocument the target document
    * @param aTagStack the namespace mapping context
    * @param aPreventExecution whether to mark scripts as already started
    * @param aReturn the result fragment
-   * @param aSanitize whether the fragment should be sanitized prior to
-   *        injection
    * @return NS_ERROR_DOM_INVALID_STATE_ERR if a re-entrant attempt to parse
    *         fragments is made, a return code from the XML parser.
    */
   static nsresult ParseFragmentXML(const nsAString& aSourceBuffer,
                                    nsIDocument* aDocument,
                                    nsTArray<nsString>& aTagStack,
                                    bool aPreventScriptExecution,
-                                   mozilla::dom::DocumentFragment** aReturn,
-                                   SanitizeFragments aSanitize = SanitizeSystemPrivileged);
+                                   mozilla::dom::DocumentFragment** aReturn);
 
   /**
    * Parse a string into a document using the HTML parser.
    * Script elements are marked unexecutable.
    *
    * @param aSourceBuffer the string to parse as an HTML document
    * @param aTargetDocument the document object to parse into. Must not have
    *                        child nodes.
--- a/dom/webidl/Element.webidl
+++ b/dom/webidl/Element.webidl
@@ -231,26 +231,16 @@ partial interface Element {
 // http://domparsing.spec.whatwg.org/#extensions-to-the-element-interface
 partial interface Element {
   [CEReactions, SetterNeedsSubjectPrincipal=NonSystem, Pure, SetterThrows, GetterCanOOM, TreatNullAs=EmptyString]
   attribute DOMString innerHTML;
   [CEReactions, Pure,SetterThrows,TreatNullAs=EmptyString]
   attribute DOMString outerHTML;
   [CEReactions, Throws]
   void insertAdjacentHTML(DOMString position, DOMString text);
-
-  /**
-   * Like the innerHTML setter, but does not sanitize its values, even in
-   * chrome-privileged documents.
-   *
-   * If you're thinking about using this, don't. You have many, much better
-   * options.
-   */
-  [ChromeOnly, Throws]
-  void unsafeSetInnerHTML(DOMString html);
 };
 
 // http://www.w3.org/TR/selectors-api/#interface-definitions
 partial interface Element {
   [Throws, Pure]
   Element?  querySelector(DOMString selectors);
   [Throws, Pure]
   NodeList  querySelectorAll(DOMString selectors);