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
CVE-2016-5641 #3201
CVE-2016-5641 #3201
Conversation
This is a proposed security fix, by the way. We attempted to notify SmartBear and apiteam@swagger.io, as did CERT (after we notified CERT), but to no avail. Please consider a security@swagger.io alias to make reporting vulnerabilities a smoother process for next time. |
please note. this build failure looks to be because of the python swagger petstore client... python is not something this PR has touched
jdk1.7 passsed, where as jdk1.8 did not (on a python client?) please advise. Local jdk1.8 with |
@sdavis-r7 I've restarted the job and now the CI tests passed. Thanks for the PR. We'll review and let you know if we've any questions. |
@sdavis-r7 thanks for this finding. I guess I never had the idea to run code generated from an untrusted API definition. (Though I'm mainly using OpenAPI/Swagger in company-internal APIs, which are extensively reviewed.) From the document:
Either this, or review the generated code (if you know the language). Or use just trusted API definitions. |
*/ | ||
{{#allParams}} // * @param {{dataType}} ${{paramName}} {{description}} {{#required}}(required){{/required}}{{^required}}(optional{{#defaultValue}}, default to {{{.}}}{{/defaultValue}}){{/required}} | ||
{{/allParams}} | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not knowing PHP that well, this looks like not the correct syntax for a documentation comment. (Might be acceptable for fixing the vulnerability, but should be changed later so something better, e.g. using a similar approach as for the other languages.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ePaul Thank you! The intent for to force it as single line, though I am very open to suggestions on modification here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdavis-r7 I'll probably keep *
and sanitize the tag to remove line break instead so as to conform to the PHP coding style.
@ePaul Trusted API can mean many things if you consider the channel between. I think that need be considered for what threat value it may have, and to your point about ensuring one can trust the provider & channel. |
I did generate the petstore samples once with current master and once after merging this PR's branch. The changes can be seen in 2dad256 – mainly comment changes for PHP, and an added |
I reproduced the Java example, but I got problems with reproducing the Node JS one. @sdavis-r7 Did you use |
@ePaul nodejs-client/javascript |
Verified and approved for language 👍 The example exploit just seems to apply to client side code (e.g. default Java client), but I guess similar vulnerabilities are there for some server implementations. I didn't find any language named I can't really judge on the fix for other languages. |
@ePaul this client: http://generator.swagger.io/api/gen/clients/javascript To be clear, The nodejs/javascript was client side (codegen). I feel the server side (codgen) should definitely be scrutinized. I will give the nodejs-server a once or twice over now. |
@ePaul I was at first feeling I was on to the unsanitzed 'example' section of service.mustache
though it appears to ' & " are santized in this. disclaimer: This is my attempt, and I may be missing something, but I don't see any current issues with nodejs-server . |
Folks, thanks for the discussion and discovery of the vulnerability. I believe proper escaping of all triple-mustache variables should be done in the language-specific template pre-processor, and that all uses of triple mustaches should be run through a specific escape logic, since escaping techniques vary between languages. This should have been done a while ago just to clean up the logic, but the vulnerability gives us a good reason to do so. Suggest we make the escape code abstract in the interface so it must be chosen in a language-specific implementation. In addition, an inventory of the triple mustaches should be done. |
@fehguy I'm not sure if concentrating on the triple mustaches is enough. While the double-mustaches are HTML-escaped, I think for some languages dangerous code can still get through. |
@ePaul can you give an example? |
@fehguy @ePaul {{}} is not enough for languages where things can be done 'beneath the radar' of html escaping. For example: (via model.mustache in the php resources)
lets the following through
Perhaps there need be deeper sanitization effforts before the template variables at a core location? A tricky task, but the usually recommended approach in AppSec/OWASP land. |
@sdavis-r7 got it, thanks. I'll think through some solutions for this and post back, probably in separate tickets. I think the fixes proposed in this PR address some of the issues but a more comprehensive fix is needed. |
@fehguy excellent. Not sure it 'fits', though the OWASP ESAPI is potentially useful here for a type of central component model. ⛄ |
When can we expect this fix to be officially released? |
please reference https://community.rapid7.com/community/infosec/blog/2016/06/23/r7-2016-06-remote-code-execution-via-swagger-parameter-injection-cve-2016-5641