Page MenuHomeSoftware Heritage

swh-web: Fix XSS
ClosedPublic

Authored by kalpitk on Sat, May 4, 5:09 PM.

Details

Summary

Fix XSS vulnerabilities in swh-web

Related T1699

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kalpitk created this revision.Sat, May 4, 5:09 PM
vlorentz accepted this revision.Mon, May 6, 10:43 AM
This revision is now accepted and ready to land.Mon, May 6, 10:43 AM
vlorentz requested changes to this revision.Mon, May 6, 10:44 AM

Could you add one (or more) unit test for this?

This revision now requires changes to proceed.Mon, May 6, 10:44 AM
anlambert added a subscriber: anlambert.EditedMon, May 6, 10:57 AM

My main issue on that change is that the generated links will not be available anymore in the displayed error message, see output below after your change on that example (https://archive.softwareheritage.org/browse/revision/041f5ea1cf987a4068ef5f39ba0a09be85952064/?origin=https://github.com/git/got):

swh.web.common.exc.NotFoundExc: The Software Heritage archive has a revision with the hash you provided but the origin mentioned in your request appears broken: <a href="https://github.com/git/got">https://github.com/git/got</a>. Please check the URL and try again.

Nevertheless, you can still browse the revision without origin information: <a href="/browse/revision/041f5ea1cf987a4068ef5f39ba0a09be85952064/">/browse/revision/041f5ea1cf987a4068ef5f39ba0a09be85952064/</a>

This makes the navigation more painful as the user will have to copy / paste the link instead of simply clicking on it.

Considering the only vulnerability here is if someone submits an origin url through a save code now request containing <script> tags in it, I think it will be better to filter out that kind
of bogus data directly in the common/origin_save.py file.

@anlambert
Ok. Will need to rework this, to keep the links working.
But,
1- if we filter out directly while saving, it may cause some valid links being rejected.
2- if I use filterXSS, other html tags will still work.

@kalpitk , indeed this is not so easy as it seems to fix any possible XSS injections.

1- if we filter out directly while saving, it may cause some valid links being rejected.

Yes, this calls for clever filtering server-side on the submitted origin urls to save into the archive.

2- if I use filterXSS, other html tags will still work.

Maybe but at least the harmful tags will be removed so this is not really a big deal IMHO.

We should really add some end to end tests using Selenium in a near future to better detect
and prevent XSS vulnerabilities.

kalpitk updated this revision to Diff 4713.Tue, May 7, 10:43 AM
  • use mark_safe with escape
anlambert requested changes to this revision.Tue, May 7, 10:56 AM

Looks good. Please address my comment and I will accept the diff.

swh/web/common/exc.py
121

Please move the call to mark_safe back into the _generate_error_page function.

This revision now requires changes to proceed.Tue, May 7, 10:56 AM
kalpitk updated this revision to Diff 4717.Tue, May 7, 11:03 AM
  • move mark_safe to _generate_error_page

@anlambert We need to remember to use 'escape' unsafe things, whenever marking some html as safe.

I think, I'll add selenium end-to-end tests in another commit (as per Timeline in my GSoC proposal?)

kalpitk updated this revision to Diff 4718.Tue, May 7, 11:18 AM

rebase and squash commits

kalpitk updated this revision to Diff 4719.Tue, May 7, 11:51 AM
  • Fix XSS in origin/search

@anlambert We need to remember to use 'escape' unsafe things, whenever marking some html as safe.

Indeed, can you create a task on the forge to track this work ?

I think, I'll add selenium end-to-end tests in another commit (as per Timeline in my GSoC proposal?)

Of course, adding end-to-end tests should be handled in a separate diff and a task should also be created on the forge (maybe I already created one, need to check that).

anlambert requested changes to this revision.Tue, May 7, 1:22 PM
anlambert added inline comments.
swh/web/browse/utils.py
866

This url must also be escaped.

This revision now requires changes to proceed.Tue, May 7, 1:22 PM
vlorentz resigned from this revision.Tue, May 7, 1:23 PM
anlambert added a comment.EditedTue, May 7, 2:10 PM

@kalpitk , I found other XSS vulnerabilities related to error pages. In the file common/exc.py, request.META['PATH_INFO'] values must also be escaped (see https://forge.softwareheritage.org/source/swh-web/browse/master/swh/web/common/exc.py$71 as an example).

Also, every message set to the following exception types: NotFoundExc, BadInpuExc, ForbiddenExc in the browse code should be checked for url escaping (see https://forge.softwareheritage.org/source/swh-web/browse/master/swh/web/browse/views/utils/snapshot_context.py$80-120 for instance)

Can you add these extra url escaping to that diff ?

kalpitk updated this revision to Diff 4722.Tue, May 7, 2:33 PM
kalpitk updated this revision to Diff 4724.Tue, May 7, 2:41 PM
  • Add escaping for BadInputExc
kalpitk updated this revision to Diff 4727.Tue, May 7, 2:46 PM
  • Add escape for request.META

@anlambert I think, I have added escape wherever it was vulnerable.

checksums and ids and plain strings can't have xss vulnerability.

anlambert added inline comments.Tue, May 7, 2:53 PM
swh/web/browse/views/utils/snapshot_context.py
121

Only the url should be escaped here, not the whole message

kalpitk added inline comments.Tue, May 7, 3:04 PM
swh/web/browse/views/utils/snapshot_context.py
121

Since we don't have any hyperlinks, we can escape the whole message.

And shouldn't branch and branch_type also be escaped in that case?

anlambert requested changes to this revision.EditedTue, May 7, 3:07 PM

@kalpitk , found another issue in the web api HTML interface: http://localhost:5004/api/1/origin/git/url/%3Cscript%3Ealert(document.domain)%3C/script%3E/

You should fix that by hacking on api/apiresponse.py

swh/web/browse/views/utils/snapshot_context.py
121

You're right, I forgot these could be specified as url arguments too.
Forget my last comment.

This revision now requires changes to proceed.Tue, May 7, 3:07 PM
kalpitk updated this revision to Diff 4729.Tue, May 7, 3:47 PM
  • Fix XSS in web API interface
anlambert requested changes to this revision.Tue, May 7, 3:51 PM
swh/web/api/apiresponse.py
180

Here I would escape the error string only when the request media type is text/html as JSON error reponses will look strange otherwise.
You can use the following test to detect the media type:

if request.accepted_media_type == 'text/html':
    error_data['reason'] = escape(error_data['reason'])
This revision now requires changes to proceed.Tue, May 7, 3:51 PM
anlambert added inline comments.Tue, May 7, 3:59 PM
swh/web/api/apiresponse.py
180

This is why the tests are failing by the way: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/429/console

anlambert added inline comments.Tue, May 7, 4:15 PM
swh/web/browse/utils.py
494

Can you remove the escaping of attrs here. It breaks some link styles and as the value of this parameter is provided from string literals in the code, we can consider it safe.

@kalpitk , I found where the XSS vulnerability is located when a branch name contains some HTML code in it.

In the file templates/includes/top-navigation.html, you need to replace:

var snapshotContext = false;
var branch = false;
{% if snapshot_context %}
  snapshotContext = true;
  branch = {{ snapshot_context.branch|jsonify }};
{% endif %}
swh.browse.initSnapshotNavigation(snapshotContext, branch);

by

var snapshotContext = false;
var branch = false;
{% if snapshot_context %}
  snapshotContext = true;
  branch = "{{ snapshot_context.branch|escape }}";
{% endif %}
swh.browse.initSnapshotNavigation(snapshotContext, branch !== "None");
kalpitk updated this revision to Diff 4741.Tue, May 7, 5:47 PM
  • fix failing tests and make minor changes
anlambert requested changes to this revision.Tue, May 7, 6:06 PM

@kalpitk , I think we fixed most of the XSS vulnerabilities so I am ready to accept the diff in its current state.
Before that, I need you to do the following:

  • address my latest comments
  • perform an interactive rebase in order to:
    • remove the useless merge commit in the diff (please rebase instead of pulling the changes from master when working on a diff)
    • squash the remaining commits into a single one
    • set the commit message to something like
swh-web: Fix numerous XSS vulnerabilities

detail the issues fixed in the commit message body then terminate the commit message with that line

Closes T1699
  • rebase on origin/master before updating the diff
swh/web/api/apiresponse.py
180

You need to remove the escaping here

swh/web/tests/api/views/test_content.py
139 ↗(On Diff #4741)

You can revert this as no escaping should happen when calling the api programmatically.

This revision now requires changes to proceed.Tue, May 7, 6:06 PM
kalpitk updated this revision to Diff 4746.Tue, May 7, 7:49 PM

squash commits

anlambert accepted this revision.Tue, May 7, 7:56 PM

Thanks for the deep testing for XSS ! Let's land this. You should be able to push your changes to the master branch using git directly (use the Phabricator web interface otherwise).

This revision is now accepted and ready to land.Tue, May 7, 7:56 PM
This revision was automatically updated to reflect the committed changes.