diff --git a/server/src/webview2/skin_css_policy.cpp b/server/src/webview2/skin_css_policy.cpp index 7e0aace0..6821940a 100644 --- a/server/src/webview2/skin_css_policy.cpp +++ b/server/src/webview2/skin_css_policy.cpp @@ -74,6 +74,31 @@ UrlAction ClassifyUrl(std::wstring url) return UrlAction::Embed; } +namespace +{ + +// `@import` only begins an at-rule at the top level of a stylesheet, which means at the very start +// or after a `;` or `}`. Matching it anywhere would let `content: "@import //x";` be treated as one +// and cut away everything up to that declaration's semicolon, leaving an unterminated string and a +// broken stylesheet -- a legitimate skin damaged in the name of protecting it. +bool BeginsAtRule(const std::wstring &css, size_t at) +{ + size_t before = at; + while (before > 0 && iswspace(css[before - 1])) + { + --before; + } + if (before != 0 && css[before - 1] != L';' && css[before - 1] != L'}') + { + return false; + } + // `@importsomething` is a different at-rule, or nothing at all. The keyword has to end here. + const size_t after = at + 7; + return after >= css.size() || !iswalnum(css[after]); +} + +} // namespace + std::wstring StripRemoteImports(const std::wstring &css) { std::wstring result; @@ -84,7 +109,7 @@ std::wstring StripRemoteImports(const std::wstring &css) size_t importPos = std::wstring::npos; for (size_t i = pos; i + 7 <= css.size(); ++i) { - if (css[i] == L'@' && MatchesFoldedAt(css, i + 1, L"import")) + if (css[i] == L'@' && MatchesFoldedAt(css, i + 1, L"import") && BeginsAtRule(css, i)) { importPos = i; break; diff --git a/server/src/webview2/skin_css_policy.h b/server/src/webview2/skin_css_policy.h index 241f6782..322ca532 100644 --- a/server/src/webview2/skin_css_policy.h +++ b/server/src/webview2/skin_css_policy.h @@ -9,6 +9,17 @@ namespace msime::skin_css { +// A note on CSS escapes, because it is not obvious that they are not a hole here. A stylesheet may +// write `url("https:\/\/host/x.png")`, and the browser unescapes that to a working remote URL. The +// literal text carries no `://`, so ClassifyUrl says Embed rather than Drop. That is safe, but only +// because of what Embed can emit: it either inlines the file as a data: payload or falls back to a +// URL under the candidate-skins virtual host. Neither can name an origin the skin author chose. The +// same reasoning covers an escaped `..`: it reaches the filesystem lookup as literal escape text, +// which does not traverse, and is never written back into the document. +// +// So the invariant that actually protects the candidate window is "Embed never emits an +// attacker-named origin", not "Drop catches every spelling of a remote URL". A change that made +// Embed fall back to writing the original URL through would reopen this. enum class UrlAction { // A path relative to the skin package: read the file and inline it. diff --git a/server/tests/src/test_skin_css_policy.cpp b/server/tests/src/test_skin_css_policy.cpp index 1f7e182d..9507783d 100644 --- a/server/tests/src/test_skin_css_policy.cpp +++ b/server/tests/src/test_skin_css_policy.cpp @@ -45,6 +45,23 @@ TEST_CASE(skin_css_paths_leaving_the_package_are_dropped) REQUIRE(ClassifyUrl(L"#../../x") == UrlAction::Drop); } +TEST_CASE(skin_css_escaped_spellings_never_reach_the_keep_branch) +{ + // Keep is the only action that writes the author's text back into the document, so it is the + // only one an escaped remote URL could exploit. Escapes hide `://` from the classifier, which + // sends them to Embed -- safe, because Embed emits either a data: payload or a candidate-skins + // URL. Pinning that here so a future change to Embed's fallback cannot quietly reopen it. + for (const wchar_t *escaped : {L"https:\\/\\/attacker.example/x.png", L"https\\3a //attacker.example/x.png", + L"\\2e \\2e /secrets.png", L"x\\../../secrets.png"}) + { + REQUIRE(ClassifyUrl(escaped) != UrlAction::Keep); + } + // A leading backslash is still an absolute reference and is dropped outright. + REQUIRE(ClassifyUrl(L"\\..\\../secrets.png") == UrlAction::Drop); + // An unescaped traversal anywhere is dropped, escaped or not. + REQUIRE(ClassifyUrl(L"x\\../../secrets.png") == UrlAction::Drop); +} + TEST_CASE(skin_css_remote_imports_are_stripped) { REQUIRE_EQ(StripRemoteImports(L"@import url(https://attacker.example/x.css);\n.a{color:red}"), @@ -55,6 +72,21 @@ TEST_CASE(skin_css_remote_imports_are_stripped) REQUIRE_EQ(StripRemoteImports(L"@IMPORT '//attacker.example/x.css';.a{}"), std::wstring(L".a{}")); } +TEST_CASE(skin_css_import_stripping_only_matches_a_real_at_rule) +{ + // The text of a declaration is not an at-rule. Cutting to the next semicolon here would leave + // an unterminated string and break a stylesheet that was doing nothing wrong. + const std::wstring inString = L".a{content:\"@import //attacker.example/x\";color:red}"; + REQUIRE_EQ(StripRemoteImports(inString), inString); + // A different at-rule that merely starts with the same letters. + const std::wstring other = L"@importantthing \"//x\";.a{}"; + REQUIRE_EQ(StripRemoteImports(other), other); + // After a closing brace and after a semicolon are both valid places for one, and both strip. + REQUIRE_EQ(StripRemoteImports(L".a{}@import \"https://attacker.example/x\";.b{}"), std::wstring(L".a{}.b{}")); + REQUIRE_EQ(StripRemoteImports(L"@charset \"utf-8\";@import \"//attacker.example/x\";.b{}"), + std::wstring(L"@charset \"utf-8\";.b{}")); +} + TEST_CASE(skin_css_local_imports_and_plain_stylesheets_are_untouched) { REQUIRE_EQ(StripRemoteImports(L"@import \"parts.css\";.a{}"), std::wstring(L"@import \"parts.css\";.a{}"));