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

Stored XSS in comments section https://publiclab.org/questions #3549

Closed
vsk4 opened this issue Oct 1, 2018 · 15 comments
Closed

Stored XSS in comments section https://publiclab.org/questions #3549

vsk4 opened this issue Oct 1, 2018 · 15 comments
Labels
bug the issue is regarding one of our programs which faces problems when a certain task is executed fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet help wanted requires help by anyone willing to contribute Ruby security

Comments

@vsk4
Copy link
Member

vsk4 commented Oct 1, 2018

Please describe the problem (or idea)

The comment section is vulnerable to stored - XSS

What happened just before the problem occurred? Or what problem could this idea solve?

I tried to comment in here and pasted simple XSS payload and uploaded the comment, after refreshing the page it got executed.
While taking the input for the comment, filtering of input does not happen so that XSS script is executed.

What did you expect to see that you didn't?

Filtering of script and no pop up when a payload(testing script) is applied.
See the deadly effects of Stored XSS here

Please show us where to look

click on this link to see the Stored -XSS
Screenshots :-
screen shot 2018-10-01 at 8 40 36 am
screen shot 2018-10-01 at 8 40 15 am

https://publiclab.org/questions

What's your PublicLab.org username?

herovsk4

This can help us diagnose the issue:

please have a look at this link for more information on XSS.
Filter the input containing Script tags like <,>-,% and all other payload inputs.

Browser, version, and operating system

Safari, chrome and firefox

Many bugs are related to these -- please help us track it down and reproduce what you're seeing!


Thank you!

Your help makes Public Lab better! We deeply appreciate your helping refine and improve this site.

To learn how to write really great issues, which increases the chances they'll be resolved, see:

https://publiclab.org/wiki/developers#Contributing+for+non-coders

@welcome
Copy link

welcome bot commented Oct 1, 2018

Thanks for opening your first issue here! Please follow the issue template to help us help you 👍🎉😄
If you have screenshots to share demonstrating the issue, that's really helpful! 📸

@jywarren jywarren added bug the issue is regarding one of our programs which faces problems when a certain task is executed Ruby security labels Oct 1, 2018
@jywarren
Copy link
Member

jywarren commented Oct 1, 2018

Hi! Thank you! I'm going to move your comment to our staging server so it doesn't impact real people posting on PublicLab.org:

https://publiclab.org/questions/apparao/09-29-2018/with-which-android-tablets-is-the-mobius-action-cam-compatible#c20756

It's now at: https://stable.publiclab.org/notes/bkleist/04-04-2017/uwec-air-quality-monitoring#c17379

The offending string was:

<img src=x onerror=prompt(133)>

I'm going to make a test which demonstrates this, so it can be fixed in the template file; I wonder if it has to do with the hidden raw display, here:

<% comment_body = render_comment_body(comment) %>
<p id="comment-body-<%= comment.cid %>"><%= raw filtered_comment_body(comment_body) %></p>

@jywarren
Copy link
Member

jywarren commented Oct 1, 2018

I think we should be filtering some parameters here:

def render_comment_body(comment)
raw RDiscount.new(
title_suggestion(comment),
:autolink
).to_html
end

here, would we filter out attributes like onerror and such? What's the best way to sanitize?

@jywarren
Copy link
Member

jywarren commented Oct 1, 2018

OK! I've demonstrated the vulnerability in a unit test here: #3553

I believe we can use the sanitize() method and whitelist a lot of attributes:

https://guides.rubyonrails.org/security.html#html-javascript-injection-countermeasures

Be sure we whitelist everything we really need, like class, href, src, and so forth!

@vsk4 did you want to give this one a try? If so, try building on the branch in the PR above -- that way if you succeed, you should be able to get that test to pass.

But first, you could experiment in a Rails console locally, by using variations of the sanitize() method to try to get it to reject the onerror attribute. Make sense?

Thanks again!

@jywarren jywarren added help wanted requires help by anyone willing to contribute fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet labels Oct 4, 2018
@jywarren
Copy link
Member

jywarren commented Oct 4, 2018

Great, so sanitize() can now go in comment.rb, here

def render_body

@vsk4
Copy link
Member Author

vsk4 commented Oct 4, 2018

@jywarren can you be clear where should I insert sanitize()?

@jywarren
Copy link
Member

jywarren commented Oct 4, 2018 via email

@jywarren
Copy link
Member

jywarren commented Oct 5, 2018

So I tried this with a badly formatted comment but it removed the markdown formatting too. I think we may need to somehow go through the HTML and ensure it's properly nested... :-/

@jywarren
Copy link
Member

jywarren commented Oct 5, 2018

And note that if we do use sanitize in the model, we'll have to call it like:

ApplicationController.helpers.sanitize() since it's not normally available in the models.

@jywarren
Copy link
Member

jywarren commented Oct 5, 2018

And note that if we do use sanitize in the model, we'll have to call it like:

ApplicationController.helpers.sanitize() since it's not normally available in the models.

Aha! The answer to closing unclosed tags is:

Nokogiri::HTML::DocumentFragment.parse(html).to_html (https://stackoverflow.com/questions/9933333/sanitize-html-and-close-incomplete-tags)

This is for a distinct issue and we should add it too, in comment.rb. I've created a test for this which is incomplete but could help us diagnose: #3630

@jywarren
Copy link
Member

Hello @vsk4, I see this is listed as a first-timers-only issue in your GCI mentor application. If you can read over some of these docs, you'll see that such issues follow a format to welcome someone else into the project, so if you're interested in being a mentor, please give this a try! We're happy to help and we're grateful for your involvement. You can read a bit more about this here:

A first-timers-only issue like these is simple, self-contained, and has specific step-by-step formatting, in order to be a great entry point for a new contributor. If you're familiar enough with this code, please consider formatting a first-timers-only issue, and then ping @publiclab/reviewers to get it labelled.

If you've already done this in another issue, that's fine, just either update your application form or respond to this comment with a link to your FTO issue and I'll update my data. Thanks a lot!

@vsk4
Copy link
Member Author

vsk4 commented Oct 21, 2018

Thank you @jywarren as of now I am not interested to be as a mentor for GCI. Thank you for helping me out. I would love to contribute to the Public Lab� community mostly in security issues 😃

@jywarren
Copy link
Member

Thank you, we'd LOVE to have your help! 🎉

@jywarren
Copy link
Member

jywarren commented Oct 22, 2018 via email

@RuthNjeri
Copy link
Contributor

Fixed in #7282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug the issue is regarding one of our programs which faces problems when a certain task is executed fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet help wanted requires help by anyone willing to contribute Ruby security
Projects
None yet
Development

No branches or pull requests

3 participants