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.

Sign up for GitHub

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

Jump to bottom

Swagger UI Description allows HTML Injection (XSS by Design) #830

Closed
DinisCruz opened this issue Jan 15, 2015 · 17 comments
Closed

Swagger UI Description allows HTML Injection (XSS by Design) #830

DinisCruz opened this issue Jan 15, 2015 · 17 comments
Labels
type: bug

Comments

@DinisCruz
Copy link

Hi, I was trying out the Swagger UI, and I noticed that the swagger.apiInfo.description value allows HTML, which could lead to an XSS injection (if that description string is maliciously manipulated)

Here is a unit test that replicates this issue (in Coffee-Script):

QA_NWR_API = require './TM-QA-NWR-API'
app        = require '../../app'


describe.only 'swagger', ->
  page = QA_NWR_API.create(before, after)
  url = 'http://localhost:8002/'
  server = null

  before (done)->
    server = app.listen(8002);
    url.append('docs').GET (html)->
        html.assert_Is_String()
        done()

  after ->
    server.close()
    url.GET (html)->
        assert_Is_Null(html)
        done()

  it 'Issue 830 - Swagger UI Description allows HTML Injection (XSS by Design)', (done)->
    dynamicText = 'jQuery Injection - '.add_5_Random_Letters()
    javascript = "$(function() { $('#target').html('#{dynamicText}')})"
    app.swagger.apiInfo.description = "Description element allows: <h1 id='target'>...</h1> <script>#{javascript}</script>"
    page.chrome.open url + 'docs', ()->
      page.html (html,$)->
        $('#target').html().assert_Is(dynamicText)
        done()

... as executed in WebStorm:

image

It looks like this is 'by design' since for example the swagger-node-express takes advantage of this 'feature' to add a link to its description: https://github.com/swagger-api/swagger-node-express/blob/master/sample-application/app.js#L83-L90

image

I don't think that this is a high risk security issue, but it would be good to either:

a) add a note to the documentation (referencing this issue) with an alert to be careful with the swagger.apiInfo.description value, or
b) use markdown for the description value (which will allow features like links, without the html scripting capability)
@fehguy fehguy added this to the v2.1.0-M1 milestone Jan 15, 2015
@DinisCruz
Copy link
Author

Here is another XSS Injection location

image

@DinisCruz
Copy link
Author

And a couple more on

image

which are rendered here:

image

@mohsen1 mohsen1 self-assigned this Jan 28, 2015
mohsen1 added a commit that referenced this issue Jan 28, 2015
@mohsen1
Strip script tags from HTML descriptions
ad96bfd 
Fixes #830
@mohsen1 mohsen1 mentioned this issue Jan 28, 2015
Closed
@fehguy
Copy link
Contributor

fehguy commented Jan 30, 2015

should be fixed in #862

@fehguy fehguy closed this as completed Jan 30, 2015
@DinisCruz
Copy link
Author

Hi, is there a test that confirms the XSS fix?

@mala
Copy link

mala commented Jul 14, 2015

@fehguy @mohsen1
this is too bad way to fix xss https://github.com/swagger-api/swagger-ui/pull/862/files#diff-7b22c617cbad75cdd8123d16345c8e80R2699

example: <img src=x onerror=...>
http://petstore.swagger.io/?url=http://api.ma.la/tmp/cors/swi/

I think that restriction of url param or attention about security is needed.

@mala
Copy link

mala commented Aug 22, 2015

ping @fehguy @mohsen1

@webron
Copy link
Contributor

webron commented Aug 22, 2015

@mala - feel free to submit a PR.

@mala
Copy link

mala commented Aug 22, 2015

I can write a patch, but I don't know all function of swagger-ui, and I'm not using swagger-ui.
Please reopen this issue for now.

@ianlewis
Copy link

ianlewis commented Sep 8, 2015

Please reopen this issue as it's a full on XSS vulnerability with the ability to exploit it just by having users click on URLs.

@webron
Copy link
Contributor

webron commented Sep 8, 2015

While we do want to solve security issues with the UI, just reopening this issue won't help. The issue was opened on a specific XSS case. Opening an issue on 'you have XX vulnerabilities everywhere' doesn't help us isolate the problems and solving them, even if there are many of them, unless it comes with a clear method of checking and testing the whole project for such vulnerabilities.

If you'd like to help us further, please open specific issues pointing us to the various locations, or a general one with more information on how to map them out. Suggestions on how to fix the issues are also welcome, and PRs are great too.

@gmanfunky gmanfunky mentioned this issue Sep 29, 2015
Closed
@gmanfunky
Copy link

The specific case this issue described was not fixed with the initially referenced fix. #862 added a 'sanitize' helper, but it was never put in use.

@DinisCruz issue also goes on to describe that this isn't just a <script injection issue.

To fix most XSS described in this issue, I suggest a combination of more liberal use of handlebar template escaping (here: #1633)

API-Data can use Markdown to insert hyperlinks in descriptions if desired.

@nikhiljindal nikhiljindal mentioned this issue Oct 20, 2015
Closed
6 tasks
@nikhiljindal
Copy link

As pointed our earlier, the specific XSS case that this issue was filed for doesnt seem to be fixed yet.
To repro: http://petstore.swagger.io/?url=http://api.ma.la/tmp/cors/swi/ (you should not be getting that dialog)

I will recommend opening this issue until it is fixed.

@webron webron reopened this Mar 17, 2016
@webron webron removed this from the v2.1-M1 milestone Mar 17, 2016
@nikhiljindal nikhiljindal mentioned this issue Mar 19, 2016
Merged
@nikhiljindal
Copy link

One way to fix the reported XSS issue (http://petstore.swagger.io/?url=http://api.ma.la/tmp/cors/swi/) is to just remove the URL query param. Is that query param required?

(I can send a PR if removing it is fine)

@webron
Copy link
Contributor

webron commented Mar 21, 2016

@nikhiljindal - it's required, yes. It's being used by quite a few users.

@DinisCruz
Copy link
Author

btw, to exploit this you don't need user interaction since the payload can be delivered on any page the user visits (in an hidden image)

Ok, after looking at it for a little bit, this is actually a much worse vulnerability than I originally thought of, as it stands it looks like it will affect any website that exposes the swagger interface.

For example : http://kubernetes.io/kubernetes/third_party/swagger-ui/?url=http://api.ma.la/tmp/cors/swi/

image

That javascript code will now execute under the domain affected (in the case above http://kubernetes.io/ )

I'm sure there is a good way to do a Google Dork search for swagger exposed interfaces

@fehguy
Copy link
Contributor

fehguy commented Jul 21, 2016

Fixed in 2.1.5

@fehguy fehguy closed this as completed Jul 21, 2016
@DinisCruz
Copy link
Author

great thanks

is there a test that confirms the fix?

can you point to the commit that fixed it?

@tbombach tbombach mentioned this issue Oct 25, 2016
Closed
10 tasks
@selimok selimok mentioned this issue Dec 24, 2016
Closed
@stoicflame stoicflame mentioned this issue Apr 19, 2017
Closed
@mikegodber mikegodber mentioned this issue Oct 19, 2017
Closed
@nekloth nekloth mentioned this issue Mar 7, 2018
Closed
@mend-for-github-com mend-for-github-com bot mentioned this issue Apr 30, 2024
Open
@mend-for-github-com mend-for-github-com bot mentioned this issue Aug 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Strip script tags from HTML descriptions swagger-api/swagger-ui
8 participants
@ianlewis @mala @webron @fehguy @mohsen1 @DinisCruz @gmanfunky @nikhiljindal