fix(dom): neutralize all comment terminators when serializing, not just "-->" (#402)
The previous fix escaped only "-->" in comment data, but the HTML tokenizer ends a comment on four sequences: "-->", "--!>", a leading ">", and a leading "->". document.createComment(...) accepts arbitrary strings, so a scripted comment like "><img src=x onerror=alert(1)>" still serialized to "<!--><img ...>-->": the leading ">" abrupt-closes the empty comment and the trailing <img> parses as live markup (mXSS). The "--!>" and "->" forms escape the same way. Every terminator form requires a ">", and entities are not decoded inside comments, so escape every ">" to ">": this neutralizes all four at once, keeps the payload inside a single comment, and supersedes the "-->"-only guard. Regression test builds a scripted comment for each terminator form, serializes, re-parses, and asserts no <img> escapes; the leading-">", leading-"->", and "--!>" cases failed before this change.
This commit is contained in:
1 parent
c915a11f59
commit
367601b1ca
1 file changed
+54
-8
@@ -125,14 +125,19 @@ impl DomTree {
|
|||||||
}
|
}
|
||||||
NodeData::Comment { contents } => {
|
NodeData::Comment { contents } => {
|
||||||
buf.push_str("<!--");
|
buf.push_str("<!--");
|
||||||
// The HTML parser can never produce a comment whose data contains
|
// The HTML parser can never produce a comment that closes early,
|
||||||
// "-->" (that ends the comment), but script can via
|
// but script can via document.createComment(...). The tokenizer
|
||||||
// document.createComment("a-->b"). Emitting it verbatim would close
|
// ends a comment on ANY of four sequences: "-->", "--!>", a
|
||||||
// the comment early and let the trailing text escape into markup.
|
// leading ">", or a leading "->" (comment-start / -start-dash
|
||||||
// Neutralize the terminator so the serialized output round-trips as
|
// abrupt-close). Every one requires a ">". Emitting it verbatim
|
||||||
// a single comment.
|
// would close the comment early and let the trailing text parse
|
||||||
if contents.contains("-->") {
|
// as live markup (mXSS). Entities are not decoded inside
|
||||||
buf.push_str(&contents.replace("-->", "-->"));
|
// comments, so escaping every ">" to ">" neutralizes all four
|
||||||
|
// forms at once and keeps the data as a single comment.
|
||||||
|
// (Supersedes the earlier "-->"-only guard, which left the
|
||||||
|
// leading-">", leading-"->", and "--!>" forms exploitable.)
|
||||||
|
if contents.contains('>') {
|
||||||
|
buf.push_str(&contents.replace('>', ">"));
|
||||||
} else {
|
} else {
|
||||||
buf.push_str(contents);
|
buf.push_str(contents);
|
||||||
}
|
}
|
||||||
@@ -233,4 +238,45 @@ mod tests {
|
|||||||
assert!(html.contains("<img"));
|
assert!(html.contains("<img"));
|
||||||
assert!(!html.contains("</img>"));
|
assert!(!html.contains("</img>"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn comment_serialization_neutralizes_all_terminator_forms() {
|
||||||
|
use crate::tree::NodeData;
|
||||||
|
|
||||||
|
// A comment ends on any of "-->", "--!>", a leading ">", or a leading
|
||||||
|
// "->". `document.createComment(...)` accepts arbitrary strings, so a
|
||||||
|
// scripted comment can carry each closing form followed by a real tag.
|
||||||
|
// Serializing must keep the payload inside a single comment; if it
|
||||||
|
// closes early, the trailing "<img>" becomes live markup (mXSS).
|
||||||
|
let payloads = [
|
||||||
|
"><img src=x onerror=alert(1)>", // leading ">" abrupt-closes empty comment
|
||||||
|
"-><img src=x>", // leading "->" abrupt-closes
|
||||||
|
"a--!><img src=x>", // internal "--!>" closes (incorrectly-closed)
|
||||||
|
"a--><img src=x>", // internal "-->" closes (the previously-fixed form)
|
||||||
|
];
|
||||||
|
|
||||||
|
for payload in payloads {
|
||||||
|
let tree = parse_html(r#"<div id="host"></div>"#);
|
||||||
|
let host = tree.get_element_by_id("host").unwrap();
|
||||||
|
let comment = tree.new_node(NodeData::Comment { contents: payload.to_string() });
|
||||||
|
tree.append_child(host, comment);
|
||||||
|
|
||||||
|
let serialized = tree.outer_html(host);
|
||||||
|
|
||||||
|
// Re-parsing the serialized markup must not surface an <img>: the
|
||||||
|
// payload has to stay inside the comment.
|
||||||
|
let reparsed = parse_html(&serialized);
|
||||||
|
assert!(
|
||||||
|
reparsed.query_selector("img").unwrap().is_none(),
|
||||||
|
"payload {payload:?} escaped the comment; serialized = {serialized}"
|
||||||
|
);
|
||||||
|
// And the serialized comment data must carry no raw ">".
|
||||||
|
let inner = &serialized[serialized.find("<!--").unwrap() + 4..];
|
||||||
|
let inner = &inner[..inner.find("-->").unwrap()];
|
||||||
|
assert!(
|
||||||
|
!inner.contains('>'),
|
||||||
|
"comment data still contains a raw '>': {serialized}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
Reference in new issue
Block a user