From: uazo Date: Mon, 5 Jun 2023 17:06:03 +0000 Subject: Log dangling attributes in some html elements Log for iframes and the base tag all attributes containing newlines or the less-then sign that can be exploited to extract or send otherwise inaccessible information. under enable-log-dangling-attributes about flag License: GPL-2.0-or-later - https://spdx.org/licenses/GPL-2.0-or-later.html --- chrome/browser/about_flags.cc | 5 ++++- .../blink/renderer/core/dom/document.cc | 18 +++++++++++++++ .../blink/renderer/core/dom/element.cc | 20 ++++++++++++++++- third_party/blink/renderer/core/dom/element.h | 3 ++- .../editing/serializers/markup_formatter.cc | 9 ++++++++ .../renderer/core/html/html_base_element.cc | 6 +++++ .../renderer/core/html/html_base_element.h | 2 ++ .../core/html/html_frame_element_base.cc | 10 +++++++++ .../renderer/core/html/html_iframe_element.cc | 16 ++++++++++++++ .../renderer/core/html/html_iframe_element.h | 2 ++ .../core/loader/frame_load_request.cc | 9 ++++++++ .../blink/renderer/core/page/frame_tree.cc | 22 +++++++++++++++++++ .../platform/runtime_enabled_features.json5 | 10 +++++++-- 13 files changed, 127 insertions(+), 5 deletions(-) diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc --- a/chrome/browser/about_flags.cc +++ b/chrome/browser/about_flags.cc @@ -5361,7 +5361,10 @@ const FeatureEntry kFeatureEntries[] = { flag_descriptions::kWebShareDescription, kOsWin | kOsCrOS | kOsMac, FEATURE_VALUE_TYPE(features::kWebShare)}, #endif // BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) - + {"enable-log-dangling-attributes", + "Log some dangling attributes", + "NOTE: log only", kOsAll, + FEATURE_VALUE_TYPE(blink::features::kLogDanglingAttributes)}, #if BUILDFLAG(IS_LINUX) {"ozone-platform-hint", flag_descriptions::kOzonePlatformHintName, flag_descriptions::kOzonePlatformHintDescription, kOsLinux, diff --git a/third_party/blink/renderer/core/dom/document.cc b/third_party/blink/renderer/core/dom/document.cc --- a/third_party/blink/renderer/core/dom/document.cc +++ b/third_party/blink/renderer/core/dom/document.cc @@ -4580,6 +4580,14 @@ void Document::ProcessBaseElement() { KURL base_element_url; if (href) { String stripped_href = StripLeadingAndTrailingHTMLSpaces(*href); + if (stripped_href.Contains('\n') || stripped_href.Contains('<')) { + AddConsoleMessage(MakeGarbageCollected( + mojom::ConsoleMessageSource::kSecurity, + mojom::ConsoleMessageLevel::kInfo, + "Bromite Dangling Markup Prevention: '" + stripped_href + + "' is not allowed as base href value.")); + //stripped_href = g_empty_atom; + } if (!stripped_href.empty()) base_element_url = KURL(FallbackBaseURL(), stripped_href); } @@ -4598,6 +4606,14 @@ void Document::ProcessBaseElement() { !GetExecutionContext()->GetSecurityOrigin()->CanRequest( base_element_url)) { UseCounter::Count(*this, WebFeature::kBaseWithCrossOriginHref); + if (RuntimeEnabledFeatures::LogDanglingAttributesEnabled()) { + AddConsoleMessage(MakeGarbageCollected( + mojom::ConsoleMessageSource::kSecurity, + mojom::ConsoleMessageLevel::kInfo, + "Bromite Dangling Markup Prevention: '" + base_element_url.GetString() + + "' URL is cross origin and cannot be used as base URLs for a document.")); + } + // base_element_url = BlankURL(); } } @@ -4617,6 +4633,8 @@ void Document::ProcessBaseElement() { if (target->Contains('<')) UseCounter::Count(*this, WebFeature::kBaseWithOpenBracketInTarget); base_target_ = *target; + if (target->Contains('\n') || target->Contains('\r') || target->Contains('<')) + base_target_ = g_null_atom; } else { base_target_ = g_null_atom; } diff --git a/third_party/blink/renderer/core/dom/element.cc b/third_party/blink/renderer/core/dom/element.cc --- a/third_party/blink/renderer/core/dom/element.cc +++ b/third_party/blink/renderer/core/dom/element.cc @@ -2608,8 +2608,26 @@ void Element::StripScriptingAttributes( attribute_vector.Shrink(destination); } +void Element::RemoveDanglingAttributes( + Vector& attribute_vector) { + for (auto& attribute : attribute_vector) { + auto value = attribute.Value(); + if (value.Contains('\n') || value.Contains('<')) { + if (RuntimeEnabledFeatures::LogDanglingAttributesEnabled()) { + GetDocument().AddConsoleMessage(MakeGarbageCollected( + mojom::ConsoleMessageSource::kSecurity, + mojom::ConsoleMessageLevel::kWarning, + "'" + value + "' is removed from attribute '" + + attribute.GetName().ToString() + "' of element '" + + tagName() + "' as may contains dangling markup")); + } + //attribute.SetValue(g_empty_atom); + } + } +} + void Element::ParserSetAttributes( - const Vector& attribute_vector) { + Vector& attribute_vector) { DCHECK(!isConnected()); DCHECK(!parentNode()); DCHECK(!element_data_); diff --git a/third_party/blink/renderer/core/dom/element.h b/third_party/blink/renderer/core/dom/element.h --- a/third_party/blink/renderer/core/dom/element.h +++ b/third_party/blink/renderer/core/dom/element.h @@ -579,7 +579,8 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable { virtual bool HasLegalLinkAttribute(const QualifiedName&) const; // Only called by the parser immediately after element construction. - void ParserSetAttributes(const Vector&); + virtual void ParserSetAttributes(Vector&); + void RemoveDanglingAttributes(Vector&); // Remove attributes that might introduce scripting from the vector leaving // the element unchanged. diff --git a/third_party/blink/renderer/core/editing/serializers/markup_formatter.cc b/third_party/blink/renderer/core/editing/serializers/markup_formatter.cc --- a/third_party/blink/renderer/core/editing/serializers/markup_formatter.cc +++ b/third_party/blink/renderer/core/editing/serializers/markup_formatter.cc @@ -28,6 +28,8 @@ #include "third_party/blink/renderer/core/editing/serializers/markup_formatter.h" #include "third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom-shared.h" +#include "third_party/blink/renderer/platform/heap/garbage_collected.h" +#include "third_party/blink/renderer/core/inspector/console_message.h" #include "third_party/blink/renderer/core/dom/cdata_section.h" #include "third_party/blink/renderer/core/dom/comment.h" #include "third_party/blink/renderer/core/dom/document.h" @@ -216,6 +218,13 @@ void MarkupFormatter::AppendAttributeValue(StringBuilder& result, const Document& document) { if (attribute.Contains('<') || attribute.Contains('>')) { document.CountUse(mojom::blink::WebFeature::kAttributeValueContainsLtOrGt); + if (RuntimeEnabledFeatures::LogDanglingAttributesEnabled()) { + document.AddConsoleMessage(MakeGarbageCollected( + mojom::ConsoleMessageSource::kSecurity, + mojom::ConsoleMessageLevel::kInfo, + "Bromite Dangling Markup Prevention: '" + attribute + + "' is not allowed as parameter value.")); + } } EntityMask entity_mask = diff --git a/third_party/blink/renderer/core/html/html_base_element.cc b/third_party/blink/renderer/core/html/html_base_element.cc --- a/third_party/blink/renderer/core/html/html_base_element.cc +++ b/third_party/blink/renderer/core/html/html_base_element.cc @@ -61,6 +61,12 @@ bool HTMLBaseElement::IsURLAttribute(const Attribute& attribute) const { HTMLElement::IsURLAttribute(attribute); } +void HTMLBaseElement::ParserSetAttributes( + Vector& attribute_vector) { + Element::RemoveDanglingAttributes(attribute_vector); + Element::ParserSetAttributes(attribute_vector); +} + KURL HTMLBaseElement::href() const { // This does not use the GetURLAttribute function because that will resolve // relative to the document's base URL; base elements like this one can be diff --git a/third_party/blink/renderer/core/html/html_base_element.h b/third_party/blink/renderer/core/html/html_base_element.h --- a/third_party/blink/renderer/core/html/html_base_element.h +++ b/third_party/blink/renderer/core/html/html_base_element.h @@ -37,6 +37,8 @@ class CORE_EXPORT HTMLBaseElement final : public HTMLElement { KURL href() const; void setHref(const AtomicString&); + void ParserSetAttributes(Vector&) override; + private: bool IsURLAttribute(const Attribute&) const override; void ParseAttribute(const AttributeModificationParams&) override; diff --git a/third_party/blink/renderer/core/html/html_frame_element_base.cc b/third_party/blink/renderer/core/html/html_frame_element_base.cc --- a/third_party/blink/renderer/core/html/html_frame_element_base.cc +++ b/third_party/blink/renderer/core/html/html_frame_element_base.cc @@ -119,6 +119,16 @@ void HTMLFrameElementBase::ParseAttribute( frame_name_ = value; } else if (name == html_names::kNameAttr) { frame_name_ = value; + if (value.Contains('\n') || value.Contains('<')) { + if (RuntimeEnabledFeatures::LogDanglingAttributesEnabled()) { + GetDocument().AddConsoleMessage(MakeGarbageCollected( + mojom::ConsoleMessageSource::kSecurity, + mojom::ConsoleMessageLevel::kInfo, + "Bromite Dangling Markup Prevention: '" + frame_name_ + + "' is not allowed as name value.")); + } + //frame_name_ = g_empty_atom; + } } else if (name == html_names::kMarginwidthAttr) { SetMarginWidth(value.ToInt()); } else if (name == html_names::kMarginheightAttr) { diff --git a/third_party/blink/renderer/core/html/html_iframe_element.cc b/third_party/blink/renderer/core/html/html_iframe_element.cc --- a/third_party/blink/renderer/core/html/html_iframe_element.cc +++ b/third_party/blink/renderer/core/html/html_iframe_element.cc @@ -157,6 +157,12 @@ void HTMLIFrameElement::CollectStyleForPresentationAttribute( } } +void HTMLIFrameElement::ParserSetAttributes( + Vector& attribute_vector) { + Element::RemoveDanglingAttributes(attribute_vector); + Element::ParserSetAttributes(attribute_vector); +} + void HTMLIFrameElement::ParseAttribute( const AttributeModificationParams& params) { const QualifiedName& name = params.name; @@ -171,6 +177,16 @@ void HTMLIFrameElement::ParseAttribute( } AtomicString old_name = name_; name_ = value; + if (name_.Contains('\n') || name_.Contains('<')) { + if (RuntimeEnabledFeatures::LogDanglingAttributesEnabled()) { + GetDocument().AddConsoleMessage(MakeGarbageCollected( + mojom::ConsoleMessageSource::kSecurity, + mojom::ConsoleMessageLevel::kInfo, + "Bromite Dangling Markup Prevention: '" + name_ + + "' is not allowed as name value.")); + } + //name_ = g_empty_atom; + } if (name_ != old_name) { FrameOwnerPropertiesChanged(); should_call_did_change_attributes = true; diff --git a/third_party/blink/renderer/core/html/html_iframe_element.h b/third_party/blink/renderer/core/html/html_iframe_element.h --- a/third_party/blink/renderer/core/html/html_iframe_element.h +++ b/third_party/blink/renderer/core/html/html_iframe_element.h @@ -61,6 +61,8 @@ class CORE_EXPORT HTMLIFrameElement : public HTMLFrameElementBase, bool Credentialless() const override { return credentialless_; } + void ParserSetAttributes(Vector&) override; + private: void SetCollapsed(bool) override; diff --git a/third_party/blink/renderer/core/loader/frame_load_request.cc b/third_party/blink/renderer/core/loader/frame_load_request.cc --- a/third_party/blink/renderer/core/loader/frame_load_request.cc +++ b/third_party/blink/renderer/core/loader/frame_load_request.cc @@ -11,6 +11,7 @@ #include "third_party/blink/public/platform/web_url_request.h" #include "third_party/blink/renderer/bindings/core/v8/capture_source_location.h" #include "third_party/blink/renderer/core/events/current_input_event.h" +#include "third_party/blink/renderer/core/inspector/console_message.h" #include "third_party/blink/renderer/core/fileapi/public_url_manager.h" #include "third_party/blink/renderer/core/frame/local_dom_window.h" #include "third_party/blink/renderer/core/html/forms/html_form_element.h" @@ -145,6 +146,14 @@ const LocalFrameToken* FrameLoadRequest::GetInitiatorFrameToken() const { const AtomicString& FrameLoadRequest::CleanNavigationTarget( const AtomicString& target) const { if (ContainsNewLineAndLessThan(target)) { + if (RuntimeEnabledFeatures::LogDanglingAttributesEnabled()) { + if (origin_window_->GetFrame() && origin_window_->GetFrame()->GetDocument()) { + origin_window_->GetFrame()->GetDocument()->AddConsoleMessage(MakeGarbageCollected( + mojom::ConsoleMessageSource::kSecurity, + mojom::ConsoleMessageLevel::kWarning, + "Bromite Dangling Markup Prevention: '" + target + "' is not allowed as navigation target")); + } + } LogDanglingMarkupHistogram(origin_window_, target); if (RuntimeEnabledFeatures::RemoveDanglingMarkupInTargetEnabled()) { DEFINE_STATIC_LOCAL(const AtomicString, blank, ("_blank")); diff --git a/third_party/blink/renderer/core/page/frame_tree.cc b/third_party/blink/renderer/core/page/frame_tree.cc --- a/third_party/blink/renderer/core/page/frame_tree.cc +++ b/third_party/blink/renderer/core/page/frame_tree.cc @@ -21,6 +21,9 @@ #include "third_party/blink/renderer/core/page/frame_tree.h" #include "third_party/blink/renderer/core/dom/document.h" +#include "third_party/blink/renderer/core/execution_context/execution_context.h" +#include "third_party/blink/renderer/platform/heap/garbage_collected.h" +#include "third_party/blink/renderer/core/inspector/console_message.h" #include "third_party/blink/renderer/core/frame/frame_client.h" #include "third_party/blink/renderer/core/frame/local_dom_window.h" #include "third_party/blink/renderer/core/frame/local_frame.h" @@ -42,6 +45,12 @@ namespace { const unsigned kInvalidChildCount = ~0U; +bool ContainsNewLineAndLessThan(const AtomicString& target) { + return (target.Contains('\n') || target.Contains('\r') || + target.Contains('\t')) && + target.Contains('<'); +} + } // namespace FrameTree::FrameTree(Frame* this_frame) @@ -210,6 +219,19 @@ FrameTree::FindResult FrameTree::FindOrCreateFrameForNavigation( if (request.GetNavigationPolicy() != kNavigationPolicyCurrentTab) return FindResult(current_frame, false); + if (ContainsNewLineAndLessThan(name)) { + // if the name contains a \n or <, the search is always deactivated + if (RuntimeEnabledFeatures::LogDanglingAttributesEnabled()) { + if (current_frame->GetDocument()) { + current_frame->GetDocument()->AddConsoleMessage(MakeGarbageCollected( + mojom::ConsoleMessageSource::kSecurity, + mojom::ConsoleMessageLevel::kWarning, + "Bromite Dangling Markup Prevention: '" + name.GetString() + "' is not allowed as frame name destination")); + } + // return FindResult(nullptr, false); + } + } + const KURL& url = request.GetResourceRequest().Url(); Frame* frame = FindFrameForNavigationInternal(name, url, &request); bool new_window = false; diff --git a/third_party/blink/renderer/platform/runtime_enabled_features.json5 b/third_party/blink/renderer/platform/runtime_enabled_features.json5 --- a/third_party/blink/renderer/platform/runtime_enabled_features.json5 +++ b/third_party/blink/renderer/platform/runtime_enabled_features.json5 @@ -1525,8 +1525,8 @@ // Experiment with preventing some instances of mutation XSS // by escaping "<" and ">" in attribute values. // See: crbug.com/1175016 - name: "EscapeLtGtInAttributes", - status: "experimental", + name: "EscapeLtGtInAttributes", // enabled by default + status: "stable", }, { name: "EventTimingInteractionCount", @@ -2296,6 +2296,12 @@ // Use LongAnimationFrameMonitor to emit longtask entries name: "LongTaskFromLongAnimationFrame" }, + { + // Enables log of some dangling attributes + // on the javascript console + name: "LogDanglingAttributes", + status: "experimental" + }, { name: "MachineLearningCommon", implied_by: ["MachineLearningModelLoader", "MachineLearningNeuralNetwork"], -- 2.25.1