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

Fix several Cross-Site Scripting Vulnerabilities #1175

Merged
merged 5 commits into from
Jan 22, 2019
Merged

Fix several Cross-Site Scripting Vulnerabilities #1175

merged 5 commits into from
Jan 22, 2019

Conversation

tch1bo
Copy link

@tch1bo tch1bo commented Jan 21, 2019

Hello,

I noticed 5 Cross-Site Scripting (XSS) Vulnerabilities in the pontoon project.

For each of the vulnerabilities please find a proposed fix in the corresponding commit. The commit messages include a Proof of Concept exploit to trigger the vulnerability.

Fortunately, I couldn't bypass the CSP, so these vulnerabilities might not be easily exploitable. Some of them are additionally protected by CSRF-cookies, which makes exploiting them in a real-world scenario even harder. Nevertheless, each of them is still a major security risk, which someone more skilled than me might successfully exploit, so i would suggest fixing them. Please consider my patches, which should mitigate the vulnerabilities and not harm legit usages of the application.

Also, please note that i am not a regular contributor to your project, so i am not very familiar with it. I found the bug while testing DeepCode’s AI Code Review. The tool can help you automate the process of finding such (and many other types of) bugs. You can sign-up your repo (free for Open Source) to receive notifications whenever new bugs are detected. You can give it a try here.

Any feedback is more than welcome at chibo@deepcode.ai.

Cheers, Victor.

Victor Chibotaru added 4 commits January 21, 2019 15:57
As a Proof of Concept consider a POST request to
https://pontoon.mozilla.org/save-custom-homepage/ with
the "custom_homepage" parameter set to "<script>evil_code</script>"
As a Proof of Concept consider an AJAX POST request to
https://pontoon.mozilla.org/get-entities/ with params set to:
project=foo, locale=foo, paths=<script>alert('xss')</script>.
This page is exempt from CSRF protection (the function is decorated
with @csrf_exempt), which makes the vulnerability exploitable.
As a Proof of Concept consider an AJAX POST request to
https://pontoon.mozilla.org/batch-edit-translations/ with
"action" parameter set to "<script>alert('xss')</script>".
@mathjazz
Copy link
Collaborator

Thanks for the patch, @tch1bo!

@adngdb Could you have a look, please?

@mathjazz mathjazz requested a review from adngdb January 21, 2019 15:48
As a Proof of Concept consider a POST request to
https://pontoon.mozilla.org/en/ajax/permissions/ with the
"managers" parameter set to "<script>alert(1)</script>".
@adngdb
Copy link
Collaborator

adngdb commented Jan 22, 2019

Hi @tch1bo, thank you for your contribution!

As far as I understand, however, these changes are useless. That is because in all but one case, we are treating data generated by django from our Form classes. That data should not contain any user input, only things that were written in code, and thus do not risk exposing us to an XSS attack.

The last one is about escaping data that comes from an external service, through their API. I understand that it is possible they might get compromised and then in turn it might be possible that we get compromised too. But I'm not even positive that is possible, since that data is exposed in a HTTP Response that is, as far as I know in our case, not rendered in our DOM. Hence, there should be no risk there either.

@mathjazz, you know more about this code than I do. Can you confirm that we do not render those HttpResponseBadRequest anywhere?

Hope that makes sense, if not, please do let me. :)

@adngdb
Copy link
Collaborator

adngdb commented Jan 22, 2019

And now I'm seeing your commit messages and I feel stupid... 🤦‍♂️

Copy link
Collaborator

@adngdb adngdb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I understand this better now, and I was able to reproduce some (but not all) of them on prod and locally. I agree that the risk here is minimum thanks to our CSP rules, but I also agree that it's better to be on the safe side of things. Thanks @tch1bo!

@adngdb adngdb merged commit 4a6006e into mozilla:master Jan 22, 2019
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

Successfully merging this pull request may close these issues.

3 participants