fix(skin): @import 剥离只匹配真正的 at-rule;补转义绕过的不变量说明与用例
自查时发现的两处。
**@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 干净。
This commit is contained in:
1 parent
40862b13ee
commit
205cafa4f5
3 files changed
+69
-1
No files matched your search
@@ -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;
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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{}"));
|
||||
|
||||
Reference in new issue
Block a user