CVE-2016-5641

[sdavis-r7]

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

    * ping @ the authors of the individual language modules
    * java @cbornet , @xhh ,@epaul
    * ruby @wing328 , @zlx 
    * javascript @xhh 
    * php @arnested 

[sdavis-r7]

ping @cbornet , @xhh, @ePaul (java), @wing328 (ruby), @xhh (javascript), @arnested (php)

[todb-r7]

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.

[sdavis-r7]

please note. this build failure looks to be because of the python swagger petstore client... python is not something this PR has touched

 [INFO] Python Swagger Petstore Client .................... FAILURE [1:01.897s]

jdk1.7 passsed, where as jdk1.8 did not (on a python client?) please advise.

Local jdk1.8 with mvn test ends in success.

[wing328]

@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.

[ePaul]

@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:

Until code generators are patched by their maintainers, users are advised to carefully inspect Swagger documents for language-specific escape sequences.

Either this, or review the generated code (if you know the language). Or use just trusted API definitions.

[ePaul]

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.)

[sdavis-r7]

@ePaul Thank you! The intent for to force it as single line, though I am very open to suggestions on modification here.

[wing328]

@sdavis-r7 I'll probably keep * and sanitize the tag to remove line break instead so as to conform to the PHP coding style.

[sdavis-r7]

@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.

[ePaul]

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 # in the Ruby docs.

[ePaul]

I reproduced the Java example, but I got problems with reproducing the Node JS one. @sdavis-r7 Did you use nodejs-server, or some other one?

[sdavis-r7]

@ePaul nodejs-client/javascript

[ePaul]

Verified and approved for language java and variants thereof (i.e. everything based on the changed AbstractJavaCodegen). 001c2c3 demonstrates the problem, 77fd629 the solution. (Maybe we should put that into some test case.)

👍

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 nodejs-client in current master – maybe @sdavis-r7 used an older version where this did exist, or some other product based on this which adds this?

I can't really judge on the fix for other languages.

[sdavis-r7]

@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.

[sdavis-r7]

@ePaul I was at first feeling I was on to the unsanitzed 'example' section of service.mustache

  {{#examples}}examples['{{contentType}}'] = {{{example}}};

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 .

[fehguy]

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.

[ePaul]

@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.

[fehguy]

@ePaul can you give an example?

[sdavis-r7]

@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)

/* 
{{description}} 
*/

lets the following through

/* 
*/ echo system(chr(0x63).chr(0x61).chr(0x74).chr(0x20).chr(0x2f).chr(0x65).chr(0x74).chr(0x63).chr(0x2f).chr(0x70).chr(0x61).chr(0x73).chr(0x73).chr(0x77).chr(0x64) ); /* 
*/

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.

[fehguy]

@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.

[sdavis-r7]

@fehguy excellent. Not sure it 'fits', though the OWASP ESAPI is potentially useful here for a type of central component model. ⛄

[rpgreen]

When can we expect this fix to be officially released?

[wing328]

@rpgreen we still need to go through each language one by one.

I just filed #3224 to provide better handle of potential code injection for PHP API client.

For your environment, is that allowed to use the latest master in production?