Description of problem: The same string is currently being returned in the response when the value of 'event' parameter is tampered in the request to specify event of embed cartridge. This could be a potential for possible XSS attacks in the future. It should not be returned in the response or '<', '>' and the like should be sanitized unless there is any specific reason to do so. Version-Release number of selected component (if applicable): http://buildvm-devops.usersys.redhat.com/puddle/build/OpenShiftEnterprise/1.1.x/2013-01-07.2/ How reproducible: Always Steps to Reproduce: 1. Send a POST request to '/broker/rest/domains/<domain>/applications/<app>/cartridges/<cartridge>/events' to request an event of application(such as start/stop cartridge) with malformed 'event' parameter. # curl -k -X POST https://10.64.40.229/broker/rest/domains/domtest/applications/app3/cartridges/mysql-5.1/events --user "jechoi:987654" --data "event=<script>alert('xss')</script>" Actual results: <script>alert('xss')</script> is found without any sanitization in the response: {"data":null,"type":null,"version":"1.2","messages":[{"field":null,"severity":"error","text":"Invalid event '<script>alert('xss')</script>' for embedded cartridge mysql-5.1 within application 'app3'","exit_code":126}],"supported_api_versions":[1.0,1.1,1.2],"status":"bad_request"} Expected results: The <script>...</script> should not be returned Additional info:
I think I understand what you're saying but I don't think it's completely valid here (or in the other similar bugs). This isn't HTML output. It *is* sanitized, for the format that it's in - JSON or XML. If your concern is that this text could later be displayed directly by a webapp on the assumption that it's safe, it's true that would be a vulnerability, but it would be the app's vulnerability. I don't see that it's the API's role to sanitize every entry that could possibly be displayed by some app. The app should (a) sanitize its own user inputs and (b) sanitize the data it gets back from any external system before displaying it. Think about it and let me know if you disagree.
Running this by Clayton for comment. I would say it's out of scope if the message was functionally significant or the fix was difficult to handle, but I can see the principle that if it's easy not to take user output and spit it back as-is, it's best to do that. The message from the API does go right back to the user; it hopefully helps them identify what they did wrong. If they reversed the order or something it may not be obvious. Events aren't a good example, but let's say cartridges: $ rhc app create app '<script>alert("xss")</script>' Password: ******** Application Options =================== Gear Size: default Scaling: no Cartridge: <script>alert("xss")</script> [...] Invalid cartridge <script>alert("xss")</script>. Valid values are (python-2.6, diy-0.1, perl-5.10, php-5.3, ruby-1.9, jenkins-1.4, ruby-1.8) It's actually possible to do exactly the same thing via the developer console app (nothing checks the legitimacy of the cartridge type until it gets to the backend). Of course it's properly escaped. I think there's a need to return what the user said at least in the case of cartridges, to help out the confused user who does this: $ rhc app create php haxx0r [...] Invalid cartridge haxx0r However it might be reasonable to just trim out HTML-significant characters which will never be seen in valid carts or events, just as a precaution.
Ideally to deal with XSS we have two main and mutually complementary solutions: 1) sanitize input, e.g. look for "<" and ">" and strip it out/refuse the input, etc. 2) sanitize output, e.g. look for "<" and ">" and strip it out or convert it to the ampersand lt/gt ; In this case it sounds like there is never going to be a valid occurrence of "<" or ">" in the input so we can either strip it, or maybe refuse input (so people don't wonder why it silently strips it and complain/submit bugs).
I agree with Kurt - broker should reject parameter arguments that contain invalid characters via the standard process the broker has. I.e. add_application cartridges parameter should reject cartridges that don't validate to the cartridge name, sending a standard field message on the response for the field 'cartridges' with a message like 'Cartridges with < in their name are not valid' and returning. In general, messages are used by the CLI to display reasonable status to the client without having to have a lot of CLI logic to handle them. So we want the broker to treat an invalid parameter like any other generalized validation error. We do NOT want a special type of error, or a nonstandard error response. We should probably be using the rails sanitize call on all message output though just in case, so in the event that we miss rejecting the input, we still don't expose the output. We do not want to change the message though (because knowing what you typed is wrong is actually kind of valid).
This is considered security hardening and not a security vulnerability as there is no known exploitation vector at this time.
I don't like to pre-escape something that's not going right into HTML. This is more likely to go out to a text client, and then what do they do - unescape it? Or show "<" and such in their output? Yuck. So I think we agree, let's outright reject parameters that have suspicious characters rather than feed them back in error messages. There's no way anyone is ever going to make requests like this innocently, even by accident; it will always be a security probe, so "Invalid parameter" is as helpful as the message needs to be in that situation (re-evaluate if that turns out not to be true somewhere). Then we can continue to give actually helpful error messages to those who just make mistakes. If there's a way to safely across-the-board sanitize messages directed at the end-user (i.e. without functional significance), I can get behind that too. I don't see a need to have three separate bugs for essentially the same issue - anyone mind if I just mark the others as duplicates and we note any other API endpoints that need addressing in this bug?
I wouldn't mind but I created those 3 bugs separately as I supposed that they would affect a few different APIs rather than all APIs, because some APIs as mentioned in comment #3 are handling its input without the issue.
We'll fix, verify, and release them all on the same schedule, I just don't see a need to keep updating three different bugs. So, let it stand that this bug currently covers hardening the following: 1. /broker/rest/domains/<domain>/applications/<app>/cartridges/<cartridge>/events "event" parameter 2. /broker/rest/domains/<domain>/applications/<app>/events "event" parameter 3. /broker/rest/domains/<domain>/applications "cartridge" parameter If we find any more we can add them.
*** Bug 892897 has been marked as a duplicate of this bug. ***
*** Bug 892869 has been marked as a duplicate of this bug. ***
Can we get this into 1.2.1?
https://github.com/openshift/origin-server/pull/3886
Commit pushed to master at https://github.com/openshift/origin-server https://github.com/openshift/origin-server/commit/7dcc74ccf055ead51df045d2c821cade9908c07c Bug 892899
Tested this bug on devenv_3932, the following 3 restapi has been fixed, so change this bug to 'verified' 1) curl -k -H 'Accept: application/xml' --user user:test https://ec2-54-226-178-211.compute-1.amazonaws.com/broker/rest/domains/zqd/applications/zqphp/events -d event="<script>xss</script>" <?xml version="1.0" encoding="UTF-8"?> <response> <status>unprocessable_entity</status> <type nil="true"></type> <data> <datum nil="true"></datum> </data> <messages> <message> <severity>error</severity> <text>Event can only contain lowercase a-z and '-' characters</text> <exit-code>126</exit-code> <field>event</field> <index nil="true"></index> </message> </messages> <version>1.6</version> <api-version>1.6</api-version> <supported-api-versions> <supported-api-version>1.0</supported-api-version> <supported-api-version>1.1</supported-api-version> <supported-api-version>1.2</supported-api-version> <supported-api-version>1.3</supported-api-version> <supported-api-version>1.4</supported-api-version> <supported-api-version>1.5</supported-api-version> <supported-api-version>1.6</supported-api-version> </supported-api-versions> </response> 2) curl -k -H 'Accept: application/xml' --user user:test https://ec2-54-226-178-211.compute-1.amazonaws.com/broker/rest/domains/zqd/applications/zqphp/cartridges -d "name=<script>alert('xss')</script>" <?xml version="1.0" encoding="UTF-8"?> <response> <status>unprocessable_entity</status> <type nil="true"></type> <data> <datum nil="true"></datum> </data> <messages> <message> <severity>error</severity> <text>Invalid cartridge name. It can only contain alphanumeric characters, dashes(-) and dots(.)</text> <exit-code>109</exit-code> <field>name</field> <index nil="true"></index> </message> </messages> <version>1.6</version> <api-version>1.6</api-version> <supported-api-versions> <supported-api-version>1.0</supported-api-version> <supported-api-version>1.1</supported-api-version> <supported-api-version>1.2</supported-api-version> <supported-api-version>1.3</supported-api-version> <supported-api-version>1.4</supported-api-version> <supported-api-version>1.5</supported-api-version> <supported-api-version>1.6</supported-api-version> </supported-api-versions> </response> 3) curl -k -H 'Accept: application/xml' --user user:test https://ec2-54-226-178-211.compute-1.amazonaws.com/broker/rest/domains/zqd/applications/zqphp/cartridges/mysql-5.1/events -d "event=<script>alert('xss')</script>" <?xml version="1.0" encoding="UTF-8"?> <response> <status>unprocessable_entity</status> <type nil="true"></type> <data> <datum nil="true"></datum> </data> <messages> <message> <severity>error</severity> <text>Event can only contain characters and '-'</text> <exit-code>126</exit-code> <field>event</field> <index nil="true"></index> </message> </messages> <version>1.6</version> <api-version>1.6</api-version> <supported-api-versions> <supported-api-version>1.0</supported-api-version> <supported-api-version>1.1</supported-api-version> <supported-api-version>1.2</supported-api-version> <supported-api-version>1.3</supported-api-version> <supported-api-version>1.4</supported-api-version> <supported-api-version>1.5</supported-api-version> <supported-api-version>1.6</supported-api-version> </supported-api-versions> </response>