Skip to content
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

Cross-Site-Scripting vulnerability in online reader! #1609

Closed
bkimminich opened this issue Nov 25, 2016 · 6 comments
Closed

Cross-Site-Scripting vulnerability in online reader! #1609

bkimminich opened this issue Nov 25, 2016 · 6 comments

Comments

@bkimminich
Copy link

I accidentally discovered that the online reader of GitBook is vulnerable to Cross-Site-Scripting (XSS). If HTML or Javascript code is not put in backticks to mark it as a code block (e.g. https://www.owasp.org/index.php/Cross-site_Scripting_(XSS)) it will actually be executed.

This is a severe security issue and should be addressed immediately. I added a harmless Javascript alert script to a page in my book for you to verify the issue: https://bkimminich.gitbooks.io/pwning-owasp-juice-shop/content/part2/xss.html

Disclaimer: I have reported this issue some weeks before via email and Slack but neither received a response nor was the issue fixed silently.

eoftedal pushed a commit to RetireJS/retire.js that referenced this issue Nov 25, 2016
@lbeltrame
Copy link

To be clear, does this affect every result produced by gitbook build? If so, it's a good time to evaluate alternatives...

@bkimminich
Copy link
Author

bkimminich commented Nov 28, 2016

At least every HTML output created from Markdown sources can include malicious content for XSS. Cannot confirm the same for AsciiDoc.

The best way to fix this is some strict sanitization of the generated HTML allowing (by whitelisting) only the minimum of tags and attributes that make sense for ebooks. Trying to forbid certain tags (by blacklisting) is practically doomed because there are a myriad of ways to disguise XSS attacks.

Here are some good libraries offering HTML Sanitization:

@AaronO
Copy link

AaronO commented Nov 28, 2016

@lbeltrame @bkimminich We often get reports of XSS "vulnerabilities", it's an attack that's easy to understand and orchestrate, so people report it without checking what measures we have in place to mitigate them.

At it's core, since gitbook by design allows authors to include JavaScript plugins, and custom HTML, users can effectively "XSS themselves". This is true for any hosting service that allows authors to use raw HTML/JS.

To recap:

Who can "create" an XSS ?

  • Authors: yes
  • Readers: no

Conclusion: You must be the owner of a piece of content to be able to inject an XSS attack.

How do we contain/sandbox XSS attacks ?

  • All content is hosted on separate user domains (e.g: {user}.gitbooks.io)
  • We use CSRF tokens to prevent CSRF attacks (enabled through XSS or otherwise)
  • Anytime user content is displayed on gitbook.com we sanitize the html
  • All cookies are HTTP only and signed, thus can not be read by in browser javascript
  • Custom javascript (good or bad) inside a book can only do very little, but allows the content owner to customize their book's behavior and add custom widgets

Conclusion: We use common and best-practice strategies to really isolate user-generated JS, whilst still enabling customization and plugins. Malicious JavaScript thus can't do much more than it would on any other kind of static sites

Conclusion

Forcefully sanitizing user's Markdown/Asciidoc/HTML would limit customization features that lots of users depend on and need. It would only provide a false sense of security, since users would be vulnerable to the core javascript that gitbook ships.

So rather than "hoping" there will never be malicious javascript, we implement best practice security measures so that malicious javascript can't really do anything that malicious (beyond what's possible on a std static website).

@bkimminich @lbeltrame Does that make sense ? I'm happy to answer any more questions you guys have !

@bkimminich
Copy link
Author

Thanks for you feedack, @AaronO! The scenario I would still worry about is a malicious author who hides an attack against another site that has no proper CSRF protection enabled. Don't see how sanitization would be able to distinguish these from included plugins.

But that you prevent session highjacking etc. on your end, that's good to hear!

@AaronO
Copy link

AaronO commented Nov 28, 2016

@bkimminich Isn't the vulnerability really with the other site ? What prevents someone from setting up their own website (GitHub pages, or whatever) to execute the same attack ?

We don't encourage users to use custom HTML, we push users to use standard markdown (which itself is XSS safe) and GitBook plugins (with the templating syntax).

The next major editor release will have WYSIWYG by default (still load & save markdown of course), which has first class support for markdown and gitbook plugins, if it detects HTML, it will fallback the user to a "raw mode" (similar to a code editor).

Overall we do not encourage people to write custom HTML, but since we're a hosting service that allows users to customize their content end to end, we can't prevent them from doing it.

What's important is that XSS can only do very little at the end of the day, because we've taken the measures to correctly isolate things.

For me it's akin to sandboxing on iOS or OS X, you can't prevent malicious or dumb code from doing an infinite loop without limiting real useful apps. The better approach is not to "ban loops" in code, but to sandbox apps and limit the resources they have access to and thus damage they can cause.

@AaronO
Copy link

AaronO commented Nov 28, 2016

@bkimminich Closing this issue as per your comment on the reveal.js thread.

@lbeltrame @bkimminich I greatly appreciate your feedback and concern for security. In this case of XSSs, I think we've taken the appropriate measures to limit any serious damage that could be done.

If you guys have any questions or further feedback, you can email me directly at aaron@gitbook.com, thanks !

@AaronO AaronO closed this as completed Nov 28, 2016
eoftedal pushed a commit to RetireJS/retire.js that referenced this issue Nov 28, 2016
* Add gitbook XSS vulnerability

see GitbookIO/gitbook#1609. Also reported to @nodesecurity.

* Revert #146 after clarification with @GitbookIO team
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants