Avoid panic in parse_legacy_color on whitespace-only input - #431
Avoid panic in parse_legacy_color on whitespace-only input#431nicoburns wants to merge 1 commit into
parse_legacy_color on whitespace-only input#431Conversation
|
It would be better to submit this upstream in Gecko, I think. As far as I know this is shared code. |
|
It's in a dir called "servo", so I think it's probably not shared. But I can probably submit upstream anyway. |
Ah, sorry. I missed that. It's not shared, so submitting it here makes sense. |
|
e665fab to
e03f460
Compare
|
I have added a test, although |
|
|
||
| // Step 3. | ||
| input = input.trim_matches(HTML_SPACE_CHARACTERS); | ||
| if input.is_empty() { |
There was a problem hiding this comment.
This seems a spec problem https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-a-legacy-colour-value
Please file an issue.
| #[cfg(test)] | ||
| mod test { | ||
| use crate::attr::parse_legacy_color; | ||
| #[test] | ||
| fn parsing_whitespace_only_color_does_not_panic() { | ||
| assert!(parse_legacy_color(" ").is_err()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This isn't checked, so please make sure there is WPT coverage
3 participants
Bug
parse_legacy_coloronly checked emptiness before stripping HTML whitespace, so a value like<body bgcolor=" ">reached step 9'sinput.as_bytes()[0]with an empty string and panicked with an index-out-of-bounds, a crash triggerable from untrusted HTML.Changes made
Two changes made in
style/servo/attr.rs:strip_prefixesfunction which is tolerant of empty input. Technically not necessarily given fix, but it's a nice readability improvement.This essentially fixes the bug twice, but I think a little defence in depth is nice here
AI declaration: this bug was found by an AI-security scan, and fix included here was proposed by the AI.
(it has been verified to be a genuine bug and sensible fix by me).