From 205cafa4f5e4a446f0fdb2fc80b915d17c18bca4 Mon Sep 17 00:00:00 2001 From: Evan Hu Date: Sun, 6 Sep 2026 19:32:59 +0900 Subject: [PATCH] =?UTF-8?q?fix(skin):=20@import=20=E5=89=A5=E7=A6=BB?= =?UTF-8?q?=E5=8F=AA=E5=8C=B9=E9=85=8D=E7=9C=9F=E6=AD=A3=E7=9A=84=20at-rul?= =?UTF-8?q?e=EF=BC=9B=E8=A1=A5=E8=BD=AC=E4=B9=89=E7=BB=95=E8=BF=87?= =?UTF-8?q?=E7=9A=84=E4=B8=8D=E5=8F=98=E9=87=8F=E8=AF=B4=E6=98=8E=E4=B8=8E?= =?UTF-8?q?=E7=94=A8=E4=BE=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 自查时发现的两处。 **@import 匹配过宽会损坏合法皮肤。** 此前在任意位置匹配 `@import`，于是 `.a{content:"@import //x";color:red}` 会被从 `@` 切到该声明的分号，留下一个未闭合的字符串和一份坏掉的样式表——为了保护皮肤反而弄坏了一个本来没问题的皮肤。改成只在 at-rule 能出现的位置匹配：样式表开头，或 `;` / `}` 之后；并要求关键字后面不是字母数字，避免 `@importantthing` 被当成 `@import`。 **CSS 转义不是漏洞，但原因不显然，值得钉死。** 样式表可以写 `url("https:\/\/host/x.png")`，浏览器解转义后得到一个可用的远程 URL，而字面文本里没有 `://`，分类器给出的是 Embed 而不是 Drop。这是安全的，但安全性来自 Embed 能产出什么：要么把文件内联成 data: 负载，要么回退到 candidate-skins 虚拟主机下的 URL，两者都无法命名皮肤作者选定的 origin。转义过的 `..` 同理——它以字面转义文本的形式进入文件系统查找，不会穿越，也永远不会被写回文档。 也就是说，真正保护候选窗的不变量是「Embed 永不产出攻击者命名的 origin」，而不是「Drop 拦住了远程 URL 的每一种写法」。把这条写进头文件，并补一个用例钉住「转义写法永远不落到 Keep」——将来若有人改动 Embed 的回退逻辑让它原样输出 URL，这个洞会重新打开，用例会先失败。 本机以 -Wall -Wextra -Wpedantic -Werror + ASan/UBSan 编译运行 8/8 通过，portable 目标经 CMake + ctest 验证，clang-format 干净。 --- server/src/webview2/skin_css_policy.cpp | 27 ++++++++++++++++++- server/src/webview2/skin_css_policy.h | 11 ++++++++ server/tests/src/test_skin_css_policy.cpp | 32 +++++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) 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{}"));