From f720be4aac53e2bcd93622a24a653aa85e12f7be Mon Sep 17 00:00:00 2001 From: Xiaocheng Hu Date: Mon, 25 Nov 2019 23:54:46 +0000 Subject: [Backport] CVE-2020-6391 - Insufficient validation of untrusted input in Blink (1/3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 > > Reviewed-by: Yoshifumi Inoue > > Reviewed-by: Kent Tamura > > 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 > Reviewed-by: Xiaocheng Hu > Commit-Queue: Owen Min > 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 --- .../core/editing/commands/clipboard_commands.cc | 10 ++-- .../editing/commands/replace_selection_command.cc | 3 +- .../core/editing/editing_style_utilities.cc | 11 +++++ .../core/editing/editing_style_utilities.h | 2 + .../editing/serializers/create_markup_options.cc | 6 +++ .../editing/serializers/create_markup_options.h | 3 ++ .../core/editing/serializers/serialization.cc | 57 ++++++++++++++++++++++ .../core/editing/serializers/serialization.h | 4 ++ .../serializers/styled_markup_accumulator.cc | 7 ++- .../serializers/styled_markup_accumulator.h | 3 ++ .../serializers/styled_markup_serializer.cc | 39 +++++++++++++-- 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 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( + MakeGarbageCollected(), *page, + nullptr, // FrameOwner* + nullptr, // WindowAgentFactory* + nullptr // InterfaceRegistry* + ); + // Don't leak the actual viewport size to unsanitized markup + LocalFrameView* frame_view = + MakeGarbageCollected(*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; template class CORE_TEMPLATE_EXPORT CreateMarkupAlgorithm; 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(""); 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 last_closed_; @@ -114,6 +116,11 @@ bool StyledMarkupTraverser::ShouldAnnotate() const { return accumulator_->ShouldAnnotate(); } +template +bool StyledMarkupTraverser::IsForMarkupSanitization() const { + return accumulator_ && accumulator_->IsForMarkupSanitization(); +} + template bool StyledMarkupTraverser::ShouldConvertBlocksToInlines() const { return accumulator_->ShouldConvertBlocksToInlines(); @@ -362,10 +369,7 @@ Node* StyledMarkupTraverser::Traverse(Node* start_node, } auto* element = DynamicTo(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::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::CreateInlineStyle( inline_style->MergeStyleFromRulesForSerialization(html_element); } + if (IsForMarkupSanitization()) + EditingStyleUtilities::StripUAStyleRulesForMarkupSanitization(inline_style); + return inline_style; } @@ -581,6 +593,25 @@ bool StyledMarkupTraverser::ContainsOnlyBRElement( return IsHTMLBRElement(first_child) && first_child == element.lastChild(); } +template +bool StyledMarkupTraverser::ShouldSerializeUnrenderedElement( + const Node& node) const { + DCHECK(!node.GetLayoutObject()); + if (node.IsElementNode() && To(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(node)) + return true; + } + return false; +} + template class StyledMarkupSerializer; template class StyledMarkupSerializer; -- cgit v1.2.1