search

lsegal / yard

YARD is a Ruby Documentation tool. The Y stands for "Yay!"
http://yardoc.org
MIT License
1.94k stars 397 forks source link

Arbitrary javascript can be inserted into doc comments #775

Closed jvennix-r7 closed 8 years ago

jvennix-r7 commented 10 years ago

It is unclear if this is a supported feature or just a byproduct of markdown generation. Either way, if I cd into someone else's project and run yard, then view the docs, I would not expect arbitrary javascript to be running in my browser. The impact is worsened by the fact that most people view their docs at a file:// URL, so on some browsers/plugins the javascript can read/steal local files. At a minimum it would be nice if yard supported a --sanitize flag that stripped <script> tags and on*= event handlers.

Reproduce:

Add <script>alert(1)</script> to a method documentation string, then view the docs for that class.

lsegal commented 10 years ago

I wouldn't call this arbitrary, since you've explicitly checked out a project that also runs arbitrary code (running YARD can run custom plugins and scripts without --safe). Note also that you can't just get access to random files, even in a file:// scheme. JavaScript does not have APIs to read the filesystem. CORS enabled browsers (any modern browser) disables XHR even in file://.

That said, this is a feature of markup libraries and depends on the markup type you use. If you're using RDoc formatting, for instance, I don't think <script> tags run, but it is different for markdown, which explicitly allows JS in their markup. We could potentially add a sanitize option.

jvennix-r7 commented 10 years ago

Sorry, I didn't mean arbitrary as in its origin, but that any javascript is allowed (it is not filtered). I didn't realize yard detects and runs plugins without --safe, that is good to know. A --strip-js option or some equivalent to --safe would be useful.

Note also that you can't just get access to random files, even in a file:// scheme. JavaScript does not have APIs to read the filesystem. CORS enabled browsers (any modern browser) disables XHR even in file://.

Unfortunately this is not true of some browsers. The worst offender is Safari, which gives free access to read any file (/etc/shadow/, system logs, etc), UXSS into any domain, load webarchives to install persisted javascript keyloggers, etc. Most other browsers allow XHR to read files in same/nested directories (which is not a problem here). A year ago I noticed a method to steal files from file:// URL in Chrome + Flash, using subdomains names to pass the data back. I submitted a bug, which was wontfix'd. This comment does a good job explaining why this is a wontfix and the dangers of local file content in Chrome with plugins. Generally the file:/// URL scheme tends to be problematic.

This would also be a useful feature for automated doc generation/hosting setups.

lsegal commented 10 years ago

We could use the same --safe to enable sanitization on HTML content. If you want to take a stab at this and open a PR, that would move things along much faster.

jvennix-r7 commented 10 years ago

@lsegal sure I'll give it a look later this week. If anything I can contribute some failing specs. Thanks for the quick response!

jvennix-r7 commented 10 years ago

Looked into this, it seems the gem to use is sanitize, but I am wary of adding nokogiri to the project just to support --safe, given that yard itself is pretty lightweight. Another option I think might work is to enable CSP with a meta tag in <head>:

<meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-eval'; style-src 'self' 'unsafe-inline'">

Which kills any inline scripts/handlers in modern CSP-capable browsers (well, most of them. Unfortunately firefox does not support the meta tag syntax yet). I might be able to wire up a "click to play" sort of thing as well that reloads with inline scripts enabled. This fix would be more forward-looking, but much more lightweight, than a capable js filter.

lsegal commented 10 years ago

I was referring more to passing a safe mode option to the relevant markup libraries that actually handle HTML. Specifically markdown ones.

I like the idea of using CSP though that would also apply to any of YARD's own script tags, so that's a problem.

jvennix-r7 commented 10 years ago

Ah okay, passing flags sounds much easier! And shoot, I didn't realize that yard uses inline <script>...</script> tags, unless these can be reworked into static js files CSP is out.

lsegal commented 8 years ago

I would accept a --safe-html as a PR. Marking this as closed (see CONTRIBUTING.md for why this was closed).