diff --git a/third_party/blink/renderer/core/css/build.gni b/third_party/blink/renderer/core/css/build.gni index be84a2ccdadfa432a6e4b5784d12cf6de53eab09..f47e8472cb7e030a61491bf85c0ef57d0f215eb0 100644 --- a/third_party/blink/renderer/core/css/build.gni +++ b/third_party/blink/renderer/core/css/build.gni @@ -784,6 +784,7 @@ blink_core_tests_css = [ "container_selector_test.cc", "counter_style_map_test.cc", "counter_style_test.cc", + "css_attr_value_tainting_test.cc", "css_basic_shape_values_test.cc", "css_computed_style_declaration_test.cc", "css_container_values_test.cc", diff --git a/third_party/blink/renderer/core/css/css_attr_value_tainting.h b/third_party/blink/renderer/core/css/css_attr_value_tainting.h index 2e269f65738cc42b4334d42b7b92a70a4f3053a9..751d5fc822d70171eed7fceeee758e8632f2442b 100644 --- a/third_party/blink/renderer/core/css/css_attr_value_tainting.h +++ b/third_party/blink/renderer/core/css/css_attr_value_tainting.h @@ -15,6 +15,7 @@ #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_ATTR_VALUE_TAINTING_H_ #define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_ATTR_VALUE_TAINTING_H_ +#include "third_party/blink/renderer/core/core_export.h" #include "third_party/blink/renderer/platform/wtf/text/string_view.h" #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h" #include "third_party/blink/renderer/platform/wtf/wtf_size_t.h" @@ -23,7 +24,7 @@ namespace blink { class CSSParserTokenStream; -StringView GetCSSAttrTaintToken(); +CORE_EXPORT StringView GetCSSAttrTaintToken(); // NOTE: IsAttrTainted() means “is this value tainted by an attr()”, // not “is this attr() value tainted” (it always is). In other words, diff --git a/third_party/blink/renderer/core/css/css_attr_value_tainting_test.cc b/third_party/blink/renderer/core/css/css_attr_value_tainting_test.cc new file mode 100644 index 0000000000000000000000000000000000000000..34b6ec67cebb9f05b5cce83e3ff8d21358a63349 --- /dev/null +++ b/third_party/blink/renderer/core/css/css_attr_value_tainting_test.cc @@ -0,0 +1,66 @@ +// Copyright 2024 The Chromium Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "third_party/blink/renderer/core/css/css_attr_value_tainting.h" + +#include "testing/gtest/include/gtest/gtest.h" +#include "third_party/blink/renderer/core/css/css_string_value.h" +#include "third_party/blink/renderer/core/css/css_test_helpers.h" +#include "third_party/blink/renderer/core/css/css_value.h" +#include "third_party/blink/renderer/core/css/css_value_list.h" +#include "third_party/blink/renderer/core/testing/page_test_base.h" + +namespace blink { + +namespace { + +class CSSAttrValueTaintingTest : public PageTestBase {}; + +TEST_F(CSSAttrValueTaintingTest, StringValue) { + String text = String("\"abc\"%T").Replace("%T", GetCSSAttrTaintToken()); + const CSSValue* value = + css_test_helpers::ParseValue(GetDocument(), "<string>", text); + ASSERT_NE(value, nullptr); + EXPECT_EQ(text, value->CssText()); + EXPECT_EQ("\"abc\"", value->UntaintedCopy()->CssText()); +} + +TEST_F(CSSAttrValueTaintingTest, CommaSeparatedStrings) { + String text = + String("\"a\", \"b\"%T, \"c\"").Replace("%T", GetCSSAttrTaintToken()); + const CSSValue* value = + css_test_helpers::ParseValue(GetDocument(), "<string>#", text); + ASSERT_NE(value, nullptr); + EXPECT_EQ(text, value->CssText()); + EXPECT_EQ("\"a\", \"b\", \"c\"", value->UntaintedCopy()->CssText()); +} + +TEST_F(CSSAttrValueTaintingTest, SpaceSeparatedStrings) { + String text = + String("\"a\" \"b\"%T \"c\"").Replace("%T", GetCSSAttrTaintToken()); + const CSSValue* value = + css_test_helpers::ParseValue(GetDocument(), "<string>+", text); + ASSERT_NE(value, nullptr); + EXPECT_EQ(text, value->CssText()); + EXPECT_EQ("\"a\" \"b\" \"c\"", value->UntaintedCopy()->CssText()); +} + +TEST_F(CSSAttrValueTaintingTest, Equality) { + String tainted_text = + String("\"abc\"%T").Replace("%T", GetCSSAttrTaintToken()); + const CSSValue* tainted_value = + css_test_helpers::ParseValue(GetDocument(), "<string>", tainted_text); + + String non_tainted_text = String("\"abc\""); + const CSSValue* non_tainted_value = + css_test_helpers::ParseValue(GetDocument(), "<string>", non_tainted_text); + + ASSERT_NE(tainted_value, nullptr); + ASSERT_NE(non_tainted_value, nullptr); + EXPECT_NE(*tainted_value, *non_tainted_value); +} + +} // namespace + +} // namespace blink diff --git a/third_party/blink/renderer/core/css/css_string_value.cc b/third_party/blink/renderer/core/css/css_string_value.cc index ad48a332f71b66c282f9508ddfd59691506aa272..0e1fcceb8187b6d5ac8604fe966fdc355022ba6e 100644 --- a/third_party/blink/renderer/core/css/css_string_value.cc +++ b/third_party/blink/renderer/core/css/css_string_value.cc @@ -4,6 +4,7 @@ #include "third_party/blink/renderer/core/css/css_string_value.h" +#include "third_party/blink/renderer/core/css/css_attr_value_tainting.h" #include "third_party/blink/renderer/core/css/css_markup.h" #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h" @@ -13,7 +14,30 @@ CSSStringValue::CSSStringValue(const String& str) : CSSValue(kStringClass), string_(str) {} String CSSStringValue::CustomCSSText() const { - return SerializeString(string_); + StringBuilder builder; + SerializeString(string_, builder); + if (attr_tainted_) { + builder.Append(GetCSSAttrTaintToken()); + } + return builder.ReleaseString(); +} + +const CSSValue* CSSStringValue::TaintedCopy() const { + if (attr_tainted_) { + return this; + } + CSSStringValue* new_value = MakeGarbageCollected<CSSStringValue>(*this); + new_value->attr_tainted_ = true; + return new_value; +} + +const CSSValue* CSSStringValue::UntaintedCopy() const { + if (!attr_tainted_) { + return this; + } + CSSStringValue* new_value = MakeGarbageCollected<CSSStringValue>(*this); + new_value->attr_tainted_ = false; + return new_value; } void CSSStringValue::TraceAfterDispatch(blink::Visitor* visitor) const { diff --git a/third_party/blink/renderer/core/css/css_string_value.h b/third_party/blink/renderer/core/css/css_string_value.h index 854217049fdda9b429dc9e7790ef99c0b40d5672..4ee3acf7bbc2e8900f5667beeeab008cf78f0453 100644 --- a/third_party/blink/renderer/core/css/css_string_value.h +++ b/third_party/blink/renderer/core/css/css_string_value.h @@ -8,6 +8,7 @@ #include "third_party/blink/renderer/core/core_export.h" #include "third_party/blink/renderer/core/css/css_value.h" #include "third_party/blink/renderer/platform/wtf/casting.h" +#include "third_party/blink/renderer/platform/wtf/text/string_builder.h" #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h" namespace blink { @@ -24,6 +25,9 @@ class CORE_EXPORT CSSStringValue : public CSSValue { return string_ == other.string_; } + const CSSValue* TaintedCopy() const; + const CSSValue* UntaintedCopy() const; + void TraceAfterDispatch(blink::Visitor*) const; private: diff --git a/third_party/blink/renderer/core/css/css_syntax_definition.cc b/third_party/blink/renderer/core/css/css_syntax_definition.cc index bd83f04696a42279f45921cfb44c800bbfd4f24e..dcfd55cc8ff01dc5de08d13d689184d285c69ded 100644 --- a/third_party/blink/renderer/core/css/css_syntax_definition.cc +++ b/third_party/blink/renderer/core/css/css_syntax_definition.cc @@ -6,6 +6,7 @@ #include <utility> +#include "third_party/blink/renderer/core/css/css_attr_value_tainting.h" #include "third_party/blink/renderer/core/css/css_string_value.h" #include "third_party/blink/renderer/core/css/css_syntax_component.h" #include "third_party/blink/renderer/core/css/css_unparsed_declaration_value.h" @@ -19,9 +20,9 @@ namespace blink { namespace { -const CSSValue* ConsumeSingleType(const CSSSyntaxComponent& syntax, - CSSParserTokenStream& stream, - const CSSParserContext& context) { +const CSSValue* ConsumeSingleTypeInternal(const CSSSyntaxComponent& syntax, + CSSParserTokenStream& stream, + const CSSParserContext& context) { switch (syntax.GetType()) { case CSSSyntaxType::kIdent: if (stream.Peek().GetType() == kIdentToken && @@ -84,6 +85,29 @@ const CSSValue* ConsumeSingleType(const CSSSyntaxComponent& syntax, } } +const CSSValue* TaintedCopyIfNeeded(const CSSValue* value) { + if (const auto* v = DynamicTo<CSSStringValue>(value)) { + return v->TaintedCopy(); + } + // Only needed for CSSStringValue for now. + return value; +} + +const CSSValue* ConsumeSingleType(const CSSSyntaxComponent& syntax, + CSSParserTokenStream& stream, + const CSSParserContext& context) { + wtf_size_t offset_before = stream.Offset(); + const CSSValue* value = ConsumeSingleTypeInternal(syntax, stream, context); + if (value) { + stream.EnsureLookAhead(); + wtf_size_t offset_after = stream.LookAheadOffset(); + if (IsAttrTainted(stream.StringRangeAt( + offset_before, /* length */ offset_after - offset_before))) { + value = TaintedCopyIfNeeded(value); + } + } + return value; +} const CSSValue* ConsumeSyntaxComponent(const CSSSyntaxComponent& syntax, CSSParserTokenStream& stream, const CSSParserContext& context) { diff --git a/third_party/blink/renderer/core/css/css_value.cc b/third_party/blink/renderer/core/css/css_value.cc index fc072b70889a8622ea63c5236206711ed99014b8..f0e82cecc289256105ff169abd43d07200356cfe 100644 --- a/third_party/blink/renderer/core/css/css_value.cc +++ b/third_party/blink/renderer/core/css/css_value.cc @@ -28,6 +28,7 @@ #include "third_party/blink/renderer/core/css/css_alternate_value.h" #include "third_party/blink/renderer/core/css/css_appearance_auto_base_select_value_pair.h" +#include "third_party/blink/renderer/core/css/css_attr_value_tainting.h" #include "third_party/blink/renderer/core/css/css_axis_value.h" #include "third_party/blink/renderer/core/css/css_basic_shape_values.h" #include "third_party/blink/renderer/core/css/css_border_image_slice_value.h" @@ -178,6 +179,9 @@ inline static bool CompareCSSValues(const CSSValue& first, } bool CSSValue::operator==(const CSSValue& other) const { + if (attr_tainted_ != other.attr_tainted_) { + return false; + } if (class_type_ == other.class_type_) { switch (GetClassType()) { case kAxisClass: @@ -507,6 +511,16 @@ String CSSValue::CssText() const { return String(); } +const CSSValue* CSSValue::UntaintedCopy() const { + if (const auto* v = DynamicTo<CSSValueList>(this)) { + return v->UntaintedCopy(); + } + if (const auto* v = DynamicTo<CSSStringValue>(this)) { + return v->UntaintedCopy(); + } + return this; +} + const CSSValue& CSSValue::PopulateWithTreeScope( const TreeScope* tree_scope) const { switch (GetClassType()) { diff --git a/third_party/blink/renderer/core/css/css_value.h b/third_party/blink/renderer/core/css/css_value.h index 1237891fc7c5e2df2dc7d1bc2619540f7d66fe70..788db077ff6879d4e22e7331f38317e25e5eccd0 100644 --- a/third_party/blink/renderer/core/css/css_value.h +++ b/third_party/blink/renderer/core/css/css_value.h @@ -232,6 +232,8 @@ class CORE_EXPORT CSSValue : public GarbageCollected<CSSValue> { } bool IsScopedValue() const { return !needs_tree_scope_population_; } + const CSSValue* UntaintedCopy() const; + #if DCHECK_IS_ON() WTF::String ClassTypeToString() const; #endif @@ -382,6 +384,9 @@ class CORE_EXPORT CSSValue : public GarbageCollected<CSSValue> { // the functionality). uint8_t was_quirky_ : 1 = false; + // See css_attr_value_tainting.h. + uint8_t attr_tainted_ : 1 = false; + private: const uint8_t class_type_; // ClassType }; diff --git a/third_party/blink/renderer/core/css/css_value_list.cc b/third_party/blink/renderer/core/css/css_value_list.cc index 7a7e3d3f250a7f551e18de450d0b91b054bcb0c2..28ef7e3b5a8ab3ff2bf20b0f3b7295bf7d4598de 100644 --- a/third_party/blink/renderer/core/css/css_value_list.cc +++ b/third_party/blink/renderer/core/css/css_value_list.cc @@ -112,6 +112,23 @@ CSSValueList* CSSValueList::Copy() const { return new_list; } +const CSSValue* CSSValueList::UntaintedCopy() const { + bool changed = false; + HeapVector<Member<const CSSValue>, 4> untainted_values; + for (const CSSValue* value : values_) { + untainted_values.push_back(value->UntaintedCopy()); + if (value != untainted_values.back().Get()) { + changed = true; + } + } + if (!changed) { + return this; + } + return MakeGarbageCollected<CSSValueList>( + static_cast<ValueListSeparator>(value_list_separator_), + std::move(untainted_values)); +} + const CSSValueList& CSSValueList::PopulateWithTreeScope( const TreeScope* tree_scope) const { // Note: this will be changed if any subclass also involves values that need diff --git a/third_party/blink/renderer/core/css/css_value_list.h b/third_party/blink/renderer/core/css/css_value_list.h index dca948405b28f001efb58bcfb8422395a7b2ab2b..58a9d55c2f9279d2f54c3cb9919e78cfbfa2905f 100644 --- a/third_party/blink/renderer/core/css/css_value_list.h +++ b/third_party/blink/renderer/core/css/css_value_list.h @@ -84,6 +84,7 @@ class CORE_EXPORT CSSValueList : public CSSValue { WTF::String CustomCSSText() const; bool Equals(const CSSValueList&) const; + const CSSValue* UntaintedCopy() const; const CSSValueList& PopulateWithTreeScope(const TreeScope*) const; bool HasFailedOrCanceledSubresources() const; diff --git a/third_party/blink/renderer/core/css/properties/longhands/custom_property.cc b/third_party/blink/renderer/core/css/properties/longhands/custom_property.cc index a4df8bd497276bc35099f26672e0cb957a88095e..a2a7ad2b3d434202d26211795a643bc20274c751 100644 --- a/third_party/blink/renderer/core/css/properties/longhands/custom_property.cc +++ b/third_party/blink/renderer/core/css/properties/longhands/custom_property.cc @@ -199,14 +199,27 @@ void CustomProperty::ApplyValue(StyleResolverState& state, bool is_animation_tainted = value_mode == ValueMode::kAnimated; + // Note that the computed value ("SetVariableValue") is stored separately + // from the substitution value ("SetVariableData") on ComputedStyle. + // The substitution value is used for substituting var() references to + // the custom property, and the computed value is generally used in other + // cases (e.g. serialization). + // + // Note also that `registered_value` may be attr-tainted at this point. + // This is what we want when producing the substitution value, + // since any tainting must survive the substitution. However, the computed + // value should serialize without taint-tokens, hence we store an + // UntaintedCopy of `registered_value`. + // + // See also css_attr_tainting.h. registered_value = &StyleBuilderConverter::ConvertRegisteredPropertyValue( state, *registered_value, context); CSSVariableData* data = StyleBuilderConverter::ConvertRegisteredPropertyVariableData( *registered_value, is_animation_tainted); - builder.SetVariableData(name_, data, is_inherited_property); - builder.SetVariableValue(name_, registered_value, is_inherited_property); + builder.SetVariableValue(name_, registered_value->UntaintedCopy(), + is_inherited_property); } const CSSValue* CustomProperty::ParseSingleValue( diff --git a/third_party/blink/web_tests/external/wpt/css/css-values/attr-all-types.html b/third_party/blink/web_tests/external/wpt/css/css-values/attr-all-types.html index a2c4272b88994860628e6dc3085c75c06ae9b171..1e1f7d015ccffaa4641602e7aaf02b8128e2d4e8 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-values/attr-all-types.html +++ b/third_party/blink/web_tests/external/wpt/css/css-values/attr-all-types.html @@ -5,12 +5,21 @@ <script src="/resources/testharness.js"></script> <script src="/resources/testharnessreport.js"></script> -<html> - <body> - <div id="attr"></div> - <div id="expected"></div> - </body> -</html> +<style> + @property --string { + syntax: "<string>"; + inherits: false; + initial-value: "none"; + } + @property --string-list { + syntax: "<string>+"; + inherits: false; + initial-value: "none"; + } +</style> + +<div id="attr"></div> +<div id="expected"></div> <script> const dimensionTypeToUnits = { @@ -124,6 +133,9 @@ test_valid_attr('grid-template-columns', 'attr(data-foo fr)', '10', '10fr'); test_valid_attr('grid-template-columns', 'attr(data-foo fr, 3fr)', '10fr', '3fr'); + test_valid_attr('--string', 'attr(data-foo string)', 'hello', '"hello"'); + test_valid_attr('--string-list', 'attr(data-foo string)', 'hello', '"hello"'); + test_dimension_types_and_units(); test_invalid_attr('animation-name', 'attr(data-foo ident)', 'initial'); diff --git a/third_party/blink/web_tests/external/wpt/css/css-values/attr-security.html b/third_party/blink/web_tests/external/wpt/css/css-values/attr-security.html index fef9d80be449816ab2be0624cdd818a2945da5a2..4280bcaeff08e827c9ccc780fcfb3bed599ac8c1 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-values/attr-security.html +++ b/third_party/blink/web_tests/external/wpt/css/css-values/attr-security.html @@ -10,8 +10,14 @@ inherits: false; initial-value: "empty"; } + @property --some-url-list { + syntax: "<string>+"; + inherits: false; + initial-value: "empty"; + } div { --some-url: attr(data-foo); + --some-url-list: attr(data-foo); --some-other-url: attr(data-foo); } </style> @@ -125,6 +131,16 @@ 'https://does-not-exist.test/404.png', 'none'); + // Test via a registered custom property (list). + test_attr('--x', + 'image-set(var(--some-url))', + 'https://does-not-exist.test/404.png', + 'image-set("https://does-not-exist.test/404.png")'); + test_attr('background-image', + 'image-set(var(--some-url))', + 'https://does-not-exist.test/404.png', + 'none'); + // Test via a non-registered custom property. test_attr('--x', 'image-set(var(--some-other-url))', diff --git a/third_party/blink/web_tests/virtual/advanced-attr/external/wpt/css/css-values/attr-security-expected.txt b/third_party/blink/web_tests/virtual/advanced-attr/external/wpt/css/css-values/attr-security-expected.txt index 7ff651ddd0080e495ec988b474ff814b1195fc25..2d9ef2b0fe098eee64457de373b865e4e277b6ed 100644 --- a/third_party/blink/web_tests/virtual/advanced-attr/external/wpt/css/css-values/attr-security-expected.txt +++ b/third_party/blink/web_tests/virtual/advanced-attr/external/wpt/css/css-values/attr-security-expected.txt @@ -7,7 +7,5 @@ This is a testharness.js-based test. assert_equals: 'background-image: image("https://does-not-exist.test/404.png")' with data-foo='https://does-not-exist.test/404.png' expected "image(url(\\"https://does-not-exist.test/404.png\\"))" but got "none" [FAIL] CSS Values and Units Test: attr() security limitations 14 assert_equals: 'background-image: image("https://does-not-exist.test/404.png")' with data-foo='https://does-not-exist.test/404.png' expected "image(url(\\"https://does-not-exist.test/404.png\\"))" but got "none" -[FAIL] CSS Values and Units Test: attr() security limitations 18 - assert_equals: 'background-image: image-set(var(--some-url))' with data-foo='https://does-not-exist.test/404.png' expected "none" but got "image-set(url(\\"https://does-not-exist.test/404.png\\") 1dppx)" Harness: the test ran to completion.