[Backport] CVE-2020-6391 - Insufficient validation of untrusted input in Blink (1/3)

Manual backport of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/1934650:
Reland "Sanitize style elements in clipboard markup"

This reverts commit f6953a5e9d62cde66ea6edd2f4f46d1dcee7940b.

Reason for revert: Manually destroyed the dummy page to ensure no leak

Original change's description:
> Revert "Sanitize style elements in clipboard markup"
>
> This reverts commit d96236b5d2bad68a0cc8f62501ba15c38c8cf96a.
>
> Reason for revert: This may cause "WebKit Linux Leak" failure
> First failure: https://ci.chromium.org/p/chromium/builders/ci/WebKit%20Linux%20Leak/7276
>
> Original change's description:
> > Sanitize style elements in clipboard markup
> >
> > This patch sanitizes clipboard markup before pasting it into document
> > by removing all pasted style elements and serializing them onto
> > elements as inline style. In this way, we stop stylesheets in clipboard
> > markup from being applied to the original elements in the document.
> >
> > This patch follows the same approach as in WebKit [1]:
> > - First create a dummy document to insert the markup
> > - Then computes style and layout in the dummy document
> > - Re-serialize the dummy document as the markup to be inserted. This
> >   reuses the code path that we serialize a selection range into
> >   clipboard, where we need to serialize element computed style into
> >   inline styles so that the element styles are preserved.
> > - Make sure all style elements are removed before inserting markup
> >   into document
> >
> > This patch also adds a complete test to ensure that content pasted from
> > Excel is still properly styled, which is the main reason we used to
> > preserve style elements in clipboard markup [2].
> >
> > [1] https://trac.webkit.org/changeset/223440
> > [2] http://crbug.com/121163
> >
> > Bug: 1017871
> > Change-Id: I3bb5a4ae7530a3fdef5ba251975e004857c06f1e
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1922919
> > Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
> > Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
> > Reviewed-by: Kent Tamura <tkent@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#718281}
>
> TBR=yosin@chromium.org,tkent@chromium.org,xiaochengh@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug: 1017871, 1027386
> Change-Id: I1d500647d6227c9be3ae14d9604ba702e9c29834
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1933452
> Reviewed-by: Owen Min <zmin@chromium.org>
> Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
> Commit-Queue: Owen Min <zmin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#718778}

TBR=yosin@chromium.org,tkent@chromium.org,zmin@chromium.org,xiaochengh@chromium.org

Cq-Include-Trybots=luci.chromium.try:layout_test_leak_detection

Bug: 1017871, 1027386

Change-Id: I3828df13d2c3ddf90df49b948302e5b59452ddfa
Reviewed-by: Jüri Valdmann <juri.valdmann@qt.io>

11 files changed, 135 insertions, 10 deletions

diff --git a/chromium/third_party/blink/renderer/core/editing/commands/clipboard_commands.cc b/chromium/third_party/blink/renderer/core/editing/commands/clipboard_commands.cc
index e44913c6e48..c791e8ed58e 100644
--- a/chromium/third_party/blink/renderer/core/editing/commands/clipboard_commands.cc
+++ b/chromium/third_party/blink/renderer/core/editing/commands/clipboard_commands.cc
@@ -359,11 +359,13 @@ ClipboardCommands::GetFragmentFromClipboard(LocalFrame& frame) {
     KURL url;
     const String markup = SystemClipboard::GetInstance().ReadHTML(
         url, fragment_start, fragment_end);
-    if (!markup.IsEmpty()) {
+    const String sanitized_markup =
+        SanitizeMarkupWithContext(markup, fragment_start, fragment_end);
+    if (!sanitized_markup.IsEmpty()) {
       DCHECK(frame.GetDocument());
-      fragment = CreateFragmentFromMarkupWithContext(
-          *frame.GetDocument(), markup, fragment_start, fragment_end, url,
-          kDisallowScriptingAndPluginContent);
+      fragment =
+          CreateFragmentFromMarkup(*frame.GetDocument(), sanitized_markup, url,
+                                   kDisallowScriptingAndPluginContent);
     }
   }
   if (fragment)
diff --git a/chromium/third_party/blink/renderer/core/editing/commands/replace_selection_command.cc b/chromium/third_party/blink/renderer/core/editing/commands/replace_selection_command.cc
index 3e9be2f1704..9dbba501604 100644
--- a/chromium/third_party/blink/renderer/core/editing/commands/replace_selection_command.cc
+++ b/chromium/third_party/blink/renderer/core/editing/commands/replace_selection_command.cc
@@ -829,7 +829,8 @@ static void RemoveHeadContents(ReplacementFragment& fragment) {
   Node* next = nullptr;
   for (Node* node = fragment.FirstChild(); node; node = next) {
     if (IsHTMLBaseElement(*node) || IsHTMLLinkElement(*node) ||
-        IsHTMLMetaElement(*node) || IsHTMLTitleElement(*node)) {
+        IsHTMLMetaElement(*node) || IsHTMLStyleElement(*node) ||
+        IsHTMLTitleElement(*node)) {
       next = NodeTraversal::NextSkippingChildren(*node);
       fragment.RemoveNode(node);
     } else {
diff --git a/chromium/third_party/blink/renderer/core/editing/editing_style_utilities.cc b/chromium/third_party/blink/renderer/core/editing/editing_style_utilities.cc
index b25aba94c04..21d2245228b 100644
--- a/chromium/third_party/blink/renderer/core/editing/editing_style_utilities.cc
+++ b/chromium/third_party/blink/renderer/core/editing/editing_style_utilities.cc
@@ -238,4 +238,15 @@ const CSSValue* EditingStyleUtilities::BackgroundColorValueInEffect(
   return nullptr;
 }
 
+void EditingStyleUtilities::StripUAStyleRulesForMarkupSanitization(
+    EditingStyle* style) {
+  // This is a hacky approach to avoid 'font-family: ""' appearing in
+  // sanitized markup.
+  // TODO(editing-dev): Implement a non-hacky fix up for all properties
+  String font_family =
+      style->Style()->GetPropertyValue(CSSPropertyID::kFontFamily);
+  if (font_family == "\"\"")
+    style->Style()->RemoveProperty(CSSPropertyID::kFontFamily);
+}
+
 }  // namespace blink
diff --git a/chromium/third_party/blink/renderer/core/editing/editing_style_utilities.h b/chromium/third_party/blink/renderer/core/editing/editing_style_utilities.h
index efaf957f088..1aeae1ae48d 100644
--- a/chromium/third_party/blink/renderer/core/editing/editing_style_utilities.h
+++ b/chromium/third_party/blink/renderer/core/editing/editing_style_utilities.h
@@ -64,6 +64,8 @@ class EditingStyleUtilities {
            unicode_bidi == CSSValueID::kEmbed;
   }
 
+  static void StripUAStyleRulesForMarkupSanitization(EditingStyle* style);
+
   static bool IsTransparentColorValue(const CSSValue*);
   static bool HasTransparentBackgroundColor(CSSStyleDeclaration*);
   static bool HasTransparentBackgroundColor(CSSPropertyValueSet*);
diff --git a/chromium/third_party/blink/renderer/core/editing/serializers/create_markup_options.cc b/chromium/third_party/blink/renderer/core/editing/serializers/create_markup_options.cc
index d726657e2aa..192e509aee2 100644
--- a/chromium/third_party/blink/renderer/core/editing/serializers/create_markup_options.cc
+++ b/chromium/third_party/blink/renderer/core/editing/serializers/create_markup_options.cc
@@ -32,5 +32,11 @@ CreateMarkupOptions::Builder::SetShouldConvertBlocksToInlines(
   data_.should_convert_blocks_to_inlines_ = convert_blocks_to_inlines;
   return *this;
 }
+CreateMarkupOptions::Builder&
+CreateMarkupOptions::Builder::SetIsForMarkupSanitization(
+    bool is_for_sanitization) {
+  data_.is_for_markup_sanitization_ = is_for_sanitization;
+  return *this;
+}
 
 }  // namespace blink
diff --git a/chromium/third_party/blink/renderer/core/editing/serializers/create_markup_options.h b/chromium/third_party/blink/renderer/core/editing/serializers/create_markup_options.h
index 63fae1e2957..ce1c9314ed4 100644
--- a/chromium/third_party/blink/renderer/core/editing/serializers/create_markup_options.h
+++ b/chromium/third_party/blink/renderer/core/editing/serializers/create_markup_options.h
@@ -31,12 +31,14 @@ class CORE_EXPORT CreateMarkupOptions final {
   bool ShouldConvertBlocksToInlines() const {
     return should_convert_blocks_to_inlines_;
   }
+  bool IsForMarkupSanitization() const { return is_for_markup_sanitization_; }
 
  private:
   Member<const Node> constraining_ancestor_;
   AbsoluteURLs should_resolve_urls_ = kDoNotResolveURLs;
   bool should_annotate_for_interchange_ = false;
   bool should_convert_blocks_to_inlines_ = false;
+  bool is_for_markup_sanitization_ = false;
 };
 
 class CORE_EXPORT CreateMarkupOptions::Builder final {
@@ -52,6 +54,7 @@ class CORE_EXPORT CreateMarkupOptions::Builder final {
   Builder& SetShouldResolveURLs(AbsoluteURLs absolute_urls);
   Builder& SetShouldAnnotateForInterchange(bool annotate_for_interchange);
   Builder& SetShouldConvertBlocksToInlines(bool convert_blocks_for_inlines);
+  Builder& SetIsForMarkupSanitization(bool is_for_sanitization);
 
  private:
   CreateMarkupOptions data_;
diff --git a/chromium/third_party/blink/renderer/core/editing/serializers/serialization.cc b/chromium/third_party/blink/renderer/core/editing/serializers/serialization.cc
index 19603ed22c5..4d3a1075398 100644
--- a/chromium/third_party/blink/renderer/core/editing/serializers/serialization.cc
+++ b/chromium/third_party/blink/renderer/core/editing/serializers/serialization.cc
@@ -51,6 +51,7 @@
 #include "third_party/blink/renderer/core/editing/visible_selection.h"
 #include "third_party/blink/renderer/core/editing/visible_units.h"
 #include "third_party/blink/renderer/core/frame/local_frame.h"
+#include "third_party/blink/renderer/core/frame/settings.h"
 #include "third_party/blink/renderer/core/html/html_anchor_element.h"
 #include "third_party/blink/renderer/core/html/html_body_element.h"
 #include "third_party/blink/renderer/core/html/html_br_element.h"
@@ -62,6 +63,8 @@
 #include "third_party/blink/renderer/core/html/html_table_element.h"
 #include "third_party/blink/renderer/core/html_names.h"
 #include "third_party/blink/renderer/core/layout/layout_object.h"
+#include "third_party/blink/renderer/core/loader/empty_clients.h"
+#include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/bindings/runtime_call_stats.h"
 #include "third_party/blink/renderer/platform/bindings/v8_per_isolate_data.h"
@@ -757,6 +760,60 @@ void MergeWithNextTextNode(Text* text_node, ExceptionState& exception_state) {
     text_next->remove(exception_state);
 }
 
+static Document* CreateStagingDocumentForMarkupSanitization() {
+  Page::PageClients page_clients;
+  FillWithEmptyClients(page_clients);
+  Page* page = Page::CreateNonOrdinary(page_clients);
+
+  page->GetSettings().SetScriptEnabled(false);
+  page->GetSettings().SetPluginsEnabled(false);
+  page->GetSettings().SetAcceleratedCompositingEnabled(false);
+
+  LocalFrame* frame = MakeGarbageCollected<LocalFrame>(
+      MakeGarbageCollected<EmptyLocalFrameClient>(), *page,
+      nullptr,  // FrameOwner*
+      nullptr,  // WindowAgentFactory*
+      nullptr   // InterfaceRegistry*
+  );
+  // Don't leak the actual viewport size to unsanitized markup
+  LocalFrameView* frame_view =
+      MakeGarbageCollected<LocalFrameView>(*frame, IntSize(800, 600));
+  frame->SetView(frame_view);
+  frame->Init();
+
+  Document* document = frame->GetDocument();
+  DCHECK(document);
+  DCHECK(document->IsHTMLDocument());
+  DCHECK(document->body());
+
+  return document;
+}
+
+String SanitizeMarkupWithContext(const String& raw_markup,
+                                 unsigned fragment_start,
+                                 unsigned fragment_end) {
+  Document* staging_document = CreateStagingDocumentForMarkupSanitization();
+  Element* body = staging_document->body();
+
+  DocumentFragment* fragment = CreateFragmentFromMarkupWithContext(
+      *staging_document, raw_markup, fragment_start, fragment_end, KURL(),
+      kDisallowScriptingAndPluginContent);
+
+  body->appendChild(fragment);
+  staging_document->UpdateStyleAndLayout();
+
+  // This sanitizes stylesheets in the markup into element inline styles
+  String result = CreateMarkup(Position::FirstPositionInNode(*body),
+                               Position::LastPositionInNode(*body),
+                               CreateMarkupOptions::Builder()
+                                   .SetShouldAnnotateForInterchange(true)
+                                   .SetIsForMarkupSanitization(true)
+                                   .Build());
+
+  staging_document->GetPage()->WillBeDestroyed();
+  return result;
+}
+
 template class CORE_TEMPLATE_EXPORT CreateMarkupAlgorithm<EditingStrategy>;
 template class CORE_TEMPLATE_EXPORT
     CreateMarkupAlgorithm<EditingInFlatTreeStrategy>;
diff --git a/chromium/third_party/blink/renderer/core/editing/serializers/serialization.h b/chromium/third_party/blink/renderer/core/editing/serializers/serialization.h
index 17d89c57aec..38e125e4a25 100644
--- a/chromium/third_party/blink/renderer/core/editing/serializers/serialization.h
+++ b/chromium/third_party/blink/renderer/core/editing/serializers/serialization.h
@@ -96,6 +96,10 @@ CreateMarkup(const PositionInFlatTree& start,
              const PositionInFlatTree& end,
              const CreateMarkupOptions& options = CreateMarkupOptions());
 
+String SanitizeMarkupWithContext(const String& raw_markup,
+                                 unsigned fragment_start,
+                                 unsigned fragment_end);
+
 void MergeWithNextTextNode(Text*, ExceptionState&);
 
 bool PropertyMissingOrEqualToNone(CSSPropertyValueSet*, CSSPropertyID);
diff --git a/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc b/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc
index f4ae1049bf1..96ce4e033f9 100644
--- a/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc
+++ b/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc
@@ -122,7 +122,12 @@ void StyledMarkupAccumulator::AppendTextWithInlineStyle(
     StringBuilder buffer;
     MarkupFormatter::AppendCharactersReplacingEntities(
         buffer, content, 0, content.length(), kEntityMaskInPCDATA);
-    result_.Append(ConvertHTMLTextToInterchangeFormat(buffer.ToString(), text));
+    // Keep collapsible white spaces as is during markup sanitization.
+    const String text_to_append =
+        IsForMarkupSanitization()
+            ? buffer.ToString()
+            : ConvertHTMLTextToInterchangeFormat(buffer.ToString(), text);
+    result_.Append(text_to_append);
   }
   if (inline_style)
     result_.Append("</span>");
diff --git a/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.h b/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.h
index 3632bcfbe28..32cda2c3669 100644
--- a/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.h
+++ b/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.h
@@ -76,6 +76,9 @@ class StyledMarkupAccumulator final {
   bool ShouldConvertBlocksToInlines() const {
     return options_.ShouldConvertBlocksToInlines();
   }
+  bool IsForMarkupSanitization() const {
+    return options_.IsForMarkupSanitization();
+  }
 
  private:
   String RenderedText(Text&);
diff --git a/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_serializer.cc b/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_serializer.cc
index f7106a6a7c8..e61dc42e5d9 100644
--- a/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_serializer.cc
+++ b/chromium/third_party/blink/renderer/core/editing/serializers/styled_markup_serializer.cc
@@ -96,12 +96,14 @@ class StyledMarkupTraverser {
  private:
   bool ShouldAnnotate() const;
   bool ShouldConvertBlocksToInlines() const;
+  bool IsForMarkupSanitization() const;
   void AppendStartMarkup(Node&);
   void AppendEndMarkup(Node&);
   EditingStyle* CreateInlineStyle(Element&);
   bool NeedsInlineStyle(const Element&);
   bool ShouldApplyWrappingStyle(const Node&) const;
   bool ContainsOnlyBRElement(const Element&) const;
+  bool ShouldSerializeUnrenderedElement(const Node&) const;
 
   StyledMarkupAccumulator* accumulator_;
   Member<Node> last_closed_;
@@ -115,6 +117,11 @@ bool StyledMarkupTraverser<Strategy>::ShouldAnnotate() const {
 }
 
 template <typename Strategy>
+bool StyledMarkupTraverser<Strategy>::IsForMarkupSanitization() const {
+  return accumulator_ && accumulator_->IsForMarkupSanitization();
+}
+
+template <typename Strategy>
 bool StyledMarkupTraverser<Strategy>::ShouldConvertBlocksToInlines() const {
   return accumulator_->ShouldConvertBlocksToInlines();
 }
@@ -362,10 +369,7 @@ Node* StyledMarkupTraverser<Strategy>::Traverse(Node* start_node,
       }
 
       auto* element = DynamicTo<Element>(n);
-      if (n->GetLayoutObject() ||
-          (element && element->HasDisplayContentsStyle()) ||
-          EnclosingElementWithTag(FirstPositionInOrBeforeNode(*n),
-                                  kSelectTag)) {
+      if (n->GetLayoutObject() || ShouldSerializeUnrenderedElement(*n)) {
         // Add the node to the markup if we're not skipping the descendants
         AppendStartMarkup(*n);
 
@@ -511,6 +515,11 @@ void StyledMarkupTraverser<Strategy>::AppendStartMarkup(Node& node) {
         // FIXME: Should this be included in forceInline?
         inline_style->Style()->SetProperty(CSSPropertyID::kFloat,
                                            CSSValueID::kNone);
+
+        if (IsForMarkupSanitization()) {
+          EditingStyleUtilities::StripUAStyleRulesForMarkupSanitization(
+              inline_style);
+        }
       }
       accumulator_->AppendTextWithInlineStyle(text, inline_style);
       break;
@@ -569,6 +578,9 @@ EditingStyle* StyledMarkupTraverser<Strategy>::CreateInlineStyle(
     inline_style->MergeStyleFromRulesForSerialization(html_element);
   }
 
+  if (IsForMarkupSanitization())
+    EditingStyleUtilities::StripUAStyleRulesForMarkupSanitization(inline_style);
+
   return inline_style;
 }
 
@@ -581,6 +593,25 @@ bool StyledMarkupTraverser<Strategy>::ContainsOnlyBRElement(
   return IsHTMLBRElement(first_child) && first_child == element.lastChild();
 }
 
+template <typename Strategy>
+bool StyledMarkupTraverser<Strategy>::ShouldSerializeUnrenderedElement(
+    const Node& node) const {
+  DCHECK(!node.GetLayoutObject());
+  if (node.IsElementNode() && To<Element>(node).HasDisplayContentsStyle())
+    return true;
+  if (EnclosingElementWithTag(FirstPositionInOrBeforeNode(node),
+                              html_names::kSelectTag)) {
+    return true;
+  }
+  if (IsForMarkupSanitization()) {
+    // During sanitization, iframes in the staging document haven't loaded and
+    // are hence not rendered. They should still be serialized.
+    if (IsA<HTMLIFrameElement>(node))
+      return true;
+  }
+  return false;
+}
+
 template class StyledMarkupSerializer<EditingStrategy>;
 template class StyledMarkupSerializer<EditingInFlatTreeStrategy>;