-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rustdoc: Prevent JS injection from localStorage #120250
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fmease (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @jsha |
@@ -24,7 +24,7 @@ function getSettingValue(settingName) { | |||
return def; | |||
} | |||
} | |||
return current; | |||
return current.replace(/[^A-Za-z0-9_-]/g,""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment explaining why this is needed and how this is fixed please for posterity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So looks good to me as is. Just one worry: if someone uses a non-latin letter (or any unicode character). For example a greek person adds a theme with a greek name for their personal use. It's not going to use then, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. Perhaps it's better to exclude a set of illegal characters instead? Something like /[\s\(\)\\\/\*\"\'\`<>:;=&]/
would have the same effect, but I'd just want to be sure we're not forgetting a character we definitely don't want to get through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from <>.;=:()[]{}&|
, is there still a way to inject anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably here:
rust/src/librustdoc/html/static/js/storage.js
Lines 123 to 132 in 0e42435
if (document.readyState === "loading") { | |
document.write(`<link rel="stylesheet" id="themeStyle" href="${newHref}">`); | |
window.currentTheme = document.getElementById("themeStyle"); | |
} else { | |
window.currentTheme = document.createElement("link"); | |
window.currentTheme.rel = "stylesheet"; | |
window.currentTheme.id = "themeStyle"; | |
window.currentTheme.href = newHref; | |
document.documentElement.appendChild(window.currentTheme); | |
} |
It really does need to use document.write, because we want it to be implicitly potentially render blocking. The explicit blocking
attribute only works in Blink.
A few things come to mind that should probably be done here:
- It should require the theme to be one that actually exists. That's basically the only way to prevent nasty clickjacking attacks.
- It should URL escape the theme name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ew, document.write
. There's the blocking="render"
attribute that can be set on stylesheets to solve just that. But it's fairly new and not yet supported by firefox. Maybe doing a sync XHR in the script to fetch the stylesheet (so it's guaranteed to be in cache) could be used to block rendering.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#blocking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe doing a sync XHR in the script to fetch the stylesheet (so it's guaranteed to be in cache) could be used to block rendering.
The stylesheet won't be same-origin as the document if:
- You're on a
file://
URL. - The
--static-root-path
points at a different http host (CORS would fix that, but that would make deployment needlessly complicated).
Also, I'm just trying to block rendering. I don't actually want to block the entire event loop.
But it's fairly new and not yet supported by firefox.
That means rustdoc can't use it yet. Once it's available in Firefox ESR, it'll make that a lot simpler!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A FIXME
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed a quick commit that attempts to ensure the theme is among the actual defined themes, using the same logic used in buildSettingsPage
. It also URI encodes the theme name when inserting into the CSS <link>
.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
While this is a bug worth fixing, I don't think this is a vulnerability in any way (since you didn't follow the Rust security policy, you probably agree), but the comment does call it one. rustdoc docs can contain arbitrary JavaScript, so you always have to trust the docs you deploy on your origin. |
I think this is also similar to rust-lang/docs.rs#167 |
No, it's totally different. This is about injecting HTML from local storage, the other one is just about "docs can contain custom HTML" |
I do agree, "vulnerability" is probably too strong. I'll revise the comment. |
This comment has been minimized.
This comment has been minimized.
Squash this? |
r? notriddle |
If you need help squashing it, the docs on how to do it can be found here: https://rustc-dev-guide.rust-lang.org/git.html#squash-your-commits |
e4c0401
to
32a0afe
Compare
Sorry for the delay, I was AFK for a few days. This is now squashed. |
Sweet! @bors r+ rollup |
…llaumeGomez Rollup of 18 pull requests Successful merges: - rust-lang#119123 (Add triagebot mentions entry for simd intrinsics) - rust-lang#119991 (Reject infinitely-sized reads from io::Repeat) - rust-lang#120172 (bootstrap: add more unit tests) - rust-lang#120250 (rustdoc: Prevent JS injection from localStorage) - rust-lang#120376 (Update codegen test for LLVM 18) - rust-lang#120387 (interpret/memory: fix safety comment for large array memset optimization) - rust-lang#120400 (Bound errors span label cleanup) - rust-lang#120402 (Make the coroutine def id of an async closure the child of the closure def id) - rust-lang#120403 (Add instructions of how to use pre-vendored 'rustc-src') - rust-lang#120424 (raw pointer metadata API: data address -> data pointer) - rust-lang#120425 (Remove unnecessary unit returns in query declarations) - rust-lang#120439 (Move UI issue tests to subdirectories) - rust-lang#120443 (Fixes footnote handling in rustdoc) - rust-lang#120452 (std: Update documentation of seek_write on Windows) - rust-lang#120460 (Be more careful about interpreting a label/lifetime as a mistyped char literal.) - rust-lang#120464 (Add matthewjasper to some review groups) - rust-lang#120467 (Update books) - rust-lang#120488 (Diagnostic lifetimes cleanups) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#120250 - chadnorvell:rustdoc-xss, r=notriddle rustdoc: Prevent JS injection from localStorage It turns out that you can execute arbitrary JavaScript on the rustdocs settings page. Here's how: 1. Open `settings.html` on a rustdocs site. 2. Set "preferred light theme" to "dark" to initialize the corresponding localStorage value. 3. Plant a payload by executing this in your browser's dev console: ``Object.keys(localStorage).forEach(key=>localStorage.setItem(key,`javascript:alert()//*/javascript:javascript:"/*'/*\`/*--></noscript></title></textarea></style></template></noembed></script><html " onmouseover=/*<svg/*/onload=alert()onload=alert()//><svg onload=alert()><svg onload=alert()>*/</style><script>alert()</script><style>`));`` 4. Refresh the page -- you should see an alert. This could be particularly dangerous if rustdocs are deployed on a domain hosting some other application. Malicious code could circumvent `same-origin` policies and do mischievous things with user data. This change ensures that only defined themes can actually be selected (arbitrary strings from localStorage will not be written to the document), and for good measure sanitizes the theme name.
Pkgsrc changes: * Adapt checksums and patches. Upstream chnages: Version 1.77.0 (2024-03-21) ========================== - [Reveal opaque types within the defining body for exhaustiveness checking.] (rust-lang/rust#116821) - [Stabilize C-string literals.] (rust-lang/rust#117472) - [Stabilize THIR unsafeck.] (rust-lang/rust#117673) - [Add lint `static_mut_refs` to warn on references to mutable statics.] (rust-lang/rust#117556) - [Support async recursive calls (as long as they have indirection).] (rust-lang/rust#117703) - [Undeprecate lint `unstable_features` and make use of it in the compiler.] (rust-lang/rust#118639) - [Make inductive cycles in coherence ambiguous always.] (rust-lang/rust#118649) - [Get rid of type-driven traversal in const-eval interning] (rust-lang/rust#119044), only as a [future compatiblity lint] (rust-lang/rust#122204) for now. - [Deny braced macro invocations in let-else.] (rust-lang/rust#119062) Compiler -------- - [Include lint `soft_unstable` in future breakage reports.] (rust-lang/rust#116274) - [Make `i128` and `u128` 16-byte aligned on x86-based targets.] (rust-lang/rust#116672) - [Use `--verbose` in diagnostic output.] (rust-lang/rust#119129) - [Improve spacing between printed tokens.] (rust-lang/rust#120227) - [Merge the `unused_tuple_struct_fields` lint into `dead_code`.] (rust-lang/rust#118297) - [Error on incorrect implied bounds in well-formedness check] (rust-lang/rust#118553), with a temporary exception for Bevy. - [Fix coverage instrumentation/reports for non-ASCII source code.] (rust-lang/rust#119033) - [Fix `fn`/`const` items implied bounds and well-formedness check.] (rust-lang/rust#120019) - [Promote `riscv32{im|imafc}-unknown-none-elf` targets to tier 2.] (rust-lang/rust#118704) - Add several new tier 3 targets: - [`aarch64-unknown-illumos`] (rust-lang/rust#112936) - [`hexagon-unknown-none-elf`] (rust-lang/rust#117601) - [`riscv32imafc-esp-espidf`] (rust-lang/rust#119738) - [`riscv32im-risc0-zkvm-elf`] (rust-lang/rust#117958) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Implement `From<&[T; N]>` for `Cow<[T]>`.] (rust-lang/rust#113489) - [Remove special-case handling of `vec.split_off (0)`.](rust-lang/rust#119917) Stabilized APIs --------------- - [`array::each_ref`] (https://doc.rust-lang.org/stable/std/primitive.array.html#method.each_ref) - [`array::each_mut`] (https://doc.rust-lang.org/stable/std/primitive.array.html#method.each_mut) - [`core::net`] (https://doc.rust-lang.org/stable/core/net/index.html) - [`f32::round_ties_even`] (https://doc.rust-lang.org/stable/std/primitive.f32.html#method.round_ties_even) - [`f64::round_ties_even`] (https://doc.rust-lang.org/stable/std/primitive.f64.html#method.round_ties_even) - [`mem::offset_of!`] (https://doc.rust-lang.org/stable/std/mem/macro.offset_of.html) - [`slice::first_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.first_chunk) - [`slice::first_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.first_chunk_mut) - [`slice::split_first_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first_chunk) - [`slice::split_first_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first_chunk_mut) - [`slice::last_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.last_chunk) - [`slice::last_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.last_chunk_mut) - [`slice::split_last_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_last_chunk) - [`slice::split_last_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_last_chunk_mut) - [`slice::chunk_by`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunk_by) - [`slice::chunk_by_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunk_by_mut) - [`Bound::map`] (https://doc.rust-lang.org/stable/std/ops/enum.Bound.html#method.map) - [`File::create_new`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.create_new) - [`Mutex::clear_poison`] (https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#method.clear_poison) - [`RwLock::clear_poison`] (https://doc.rust-lang.org/stable/std/sync/struct.RwLock.html#method.clear_poison) Cargo ----- - [Extend the build directive syntax with `cargo::`.] (rust-lang/cargo#12201) - [Stabilize metadata `id` format as `PackageIDSpec`.] (rust-lang/cargo#12914) - [Pull out as `cargo-util-schemas` as a crate.] (rust-lang/cargo#13178) - [Strip all debuginfo when debuginfo is not requested.] (rust-lang/cargo#13257) - [Inherit jobserver from env for all kinds of runners.] (rust-lang/cargo#12776) - [Deprecate rustc plugin support in cargo.] (rust-lang/cargo#13248) Rustdoc ----- - [Allows links in markdown headings.] (rust-lang/rust#117662) - [Search for tuples and unit by type with `()`.] (rust-lang/rust#118194) - [Clean up the source sidebar's hide button.] (rust-lang/rust#119066) - [Prevent JS injection from `localStorage`.] (rust-lang/rust#120250) Misc ---- - [Recommend version-sorting for all sorting in style guide.] (rust-lang/rust#115046) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Add more weirdness to `weird-exprs.rs`.] (rust-lang/rust#119028)
It turns out that you can execute arbitrary JavaScript on the rustdocs settings page. Here's how:
settings.html
on a rustdocs site.Object.keys(localStorage).forEach(key=>localStorage.setItem(key,`javascript:alert()//*/javascript:javascript:"/*'/*\`/*--></noscript></title></textarea></style></template></noembed></script><html " onmouseover=/*<svg/*/onload=alert()onload=alert()//><svg onload=alert()><svg onload=alert()>*/</style><script>alert()</script><style>`));
This could be particularly dangerous if rustdocs are deployed on a domain hosting some other application. Malicious code could circumvent
same-origin
policies and do mischievous things with user data.This change ensures that only defined themes can actually be selected (arbitrary strings from localStorage will not be written to the document), and for good measure sanitizes the theme name.