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 --- 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 ++- .../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 ++ .../blink/renderer/core/page/frame_tree.cc | 16 +++++++++++++++ .../platform/runtime_enabled_features.json5 | 10 ++++++++-- 11 files changed, 103 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 @@ -5043,7 +5043,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 @@ -4480,6 +4480,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); } @@ -4498,6 +4506,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(); } } @@ -4517,6 +4533,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 @@ -2531,8 +2531,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 @@ -568,7 +568,8 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable { virtual const QualifiedName& SubResourceAttributeName() 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/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 @@ -167,6 +167,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; @@ -181,6 +187,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/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" @@ -239,6 +242,19 @@ FrameTree::FindResult FrameTree::FindOrCreateFrameForNavigation( LogDanglingMarkupHistogram(current_frame->GetDocument(), name); } + 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 @@ -1493,8 +1493,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", }, { // Non-standard API Event.path. Should be replaced by Event.composedPath. @@ -2196,6 +2196,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