KNOX-1817 - Fix XSS issues with Alias API #70

moresandeep · 2019-03-12T20:26:08Z

What changes were proposed in this pull request?

The Alias API was passing user input back in some cases as response without encoding, this was when an error was thrown or when a response message saying 'alias' for a 'topology' was created. This opens up the API for XSS attacks. The PR:

Adds encoding to the data that is going out as response.
Decodes the user inputs - since the api uses application/x-www-form-urlencoded

How was this patch tested?

The patch was tested manually e.g.

`curl -iku admin:admin-password -H "Content-Type: application/json" -d "value=mysecret" -X PUT 'https://localhost:8443/gateway/admin/api/v1/aliases/sandbox/somelongreallylongalias<>'
HTTP/1.1 201 Created
Date: Tue, 12 Mar 2019 19:54:00 GMT
Set-Cookie: KNOXSESSIONID=node0tb9bz05vhh6k1xpp0ti2p0vqh2.node0;Path=/gateway/admin;Secure;HttpOnly
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Set-Cookie: rememberMe=deleteMe; Path=/gateway/admin; Max-Age=0; Expires=Mon, 11-Mar-2019 19:54:00 GMT
Content-Type: application/json
Content-Length: 85
Server: Jetty(9.4.15.v20190215)

{ "created" : { "topology": "sandbox", "alias": "somelongreallylongalias<>" } }`

moresandeep · 2019-03-12T20:26:59Z

@risdenk and @lmccay would love to get your inputs on this patch.

risdenk

One change required for sure - new version property not in the right place and doesn't follow same naming convention.

Also I don't know if we need to url decode the parameters. jax-rs should decode them automatically?

If the path params are decoded automatically, not sure we need to add the url decoding and the added catch exceptions.

risdenk · 2019-03-12T20:34:51Z

gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/AliasResource.java

      return status(INTERNAL_SERVER_ERROR).
-          entity("Error adding alias " + alias + " for cluster " + topology
+          entity("Error adding alias " + Encode.forHtml(alias) + " for cluster " + Encode.forHtml(topology)
              + ", reason: " + e.toString()).


Will the e.toString() potentially print the value of the alias from line 111 on a decoding error?

Interesting, there is a chance it could, I will remove it.

I think if the URLDecoder stuff is removed then there shouldn't be a reason for the alias to be printed. Not sure though.

risdenk · 2019-03-12T20:35:08Z

gateway-service-admin/src/main/java/org/apache/knox/gateway/service/admin/AliasResource.java

+      /* check if the string is properly encoded and expect encoded values */
+      topology = URLDecoder.decode(topology, StandardCharsets.UTF_8.name());
+      alias = URLDecoder.decode(alias, StandardCharsets.UTF_8.name());
+      value = URLDecoder.decode(value, StandardCharsets.UTF_8.name());


Does this limit the values that can be used for an alias?

values ? I did not follow the comment. I will be taking out this piece of code because of your earlier comment.

Yea sorry wasn't clear. I meant does the url decoding mess up what alias values can be stored. ie: kevin%20password but guess it doesn't matter since this gets removed anyway :)

Ahh, I see, righto, I thought I was doing it wrong by not decoding it and including the 'application/x-www-form-urlencoded' but I was wrong.

risdenk · 2019-03-12T20:37:57Z

...ay-service-admin/src/main/java/org/apache/knox/gateway/service/admin/TopologiesResource.java

@@ -360,7 +361,8 @@ public Response uploadProviderConfiguration(@PathParam("name") String name, @Con
        }
      } catch (URISyntaxException e) {
        log.invalidResourceURI(e.getInput(), e.getReason(), e);
-        response = status(Response.Status.BAD_REQUEST).entity("{ \"error\" : \"Failed to deploy provider configuration " + name + "\" }").build();
+        response = status(Response.Status.BAD_REQUEST).entity("{ \"error\" : \"Failed to deploy provider configuration " + Encode
+            .forHtml(name) + "\" }").build();


Seems like a weird break? Put Encode.forHtml(... on the same line?

yeah, I fixed one weird break and introduced another :(

risdenk · 2019-03-12T20:38:16Z

pom.xml

@@ -230,6 +230,7 @@
        <xml-matchers.version>0.10</xml-matchers.version>
        <zip4j.version>1.3.2</zip4j.version>
        <zookeeper.version>3.4.13</zookeeper.version>
+        <owasp-java-encoder>1.2.2</owasp-java-encoder>


Should be in alphabetical order, this is out of place

Property should also end in .version so something like owasp-java-encoder.version

I did not realize we were doing props in alphabetical order, previously we used to add new properties at the bottom, I'll change it to make it in order.

Missed the .version, forgot to clean up, I'll fix this.

Yea alphabetical after I cleaned up all the poms. Makes it easier to find later :)

rlevas · 2019-03-12T20:41:49Z

Since the content type is JSON, I think it would be an issue if the returned JSON document contained characters encoded for HTML. This may be confusing to a client using the JSON document since HTML-encoded characters may be re-encoded if being displayed in an HTML document.

<script>...</script> means something in an HTML document, but has no meaning (other than the literal string) in a JSON document. Therefore I would expect a consumer of a JSON document to properly encode the data for the target viewer. If the target was an HTML document, I would expect that the JSON to HTML translation code would perform the encoding of the string as needed.

moresandeep · 2019-03-12T21:53:53Z

One change required for sure - new version property not in the right place and doesn't follow same naming convention.

Oh right, this needs to be fixed.

Also I don't know if we need to url decode the parameters. jax-rs should decode them automatically?

https://docs.jboss.org/resteasy/docs/1.0.2.GA/userguide/html/_Encoded_and_encoding.html

https://www.concretepage.com/webservices/resteasy-3/rest-web-service-jax-rs-encoded-annotation-example-with-resteasy-3

If the path params are decoded automatically, not sure we need to add the url decoding and the added catch exceptions.

You are right, jax-rs does automatically decode params so we don't this.

moresandeep · 2019-03-12T22:20:56Z

was

Thanks @rlevas for using the <script> example. I was in two minds about using the HTML encoder, the argument I had in my mind was, something like Angular can pull out the value from JSON and use it. I get your point that HTML encoding is the responsibility of the app that does the conversion and <script> have no meaning as far as JSON is concerned. I am inclined towards taking the HTML encoding out for the JSON responses now.

moresandeep · 2019-03-13T19:34:02Z

After more discussion with @rlevas offline with reference to his previous comments it appears that this would not be an issue since the API will always return a JSON response.

Thanks @rlevas and @risdenk for the inputs !

KNOX-1817 - Fix XSS issues with Alias API

7a08d59

risdenk requested review from risdenk and lmccay March 12, 2019 20:29

risdenk requested changes Mar 12, 2019

View reviewed changes

moresandeep closed this Mar 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KNOX-1817 - Fix XSS issues with Alias API #70

KNOX-1817 - Fix XSS issues with Alias API #70

moresandeep commented Mar 12, 2019

moresandeep commented Mar 12, 2019

risdenk left a comment

risdenk Mar 12, 2019

moresandeep Mar 12, 2019

risdenk Mar 12, 2019

risdenk Mar 12, 2019

moresandeep Mar 12, 2019

risdenk Mar 12, 2019

moresandeep Mar 13, 2019

risdenk Mar 12, 2019

moresandeep Mar 12, 2019

risdenk Mar 12, 2019

risdenk Mar 12, 2019

moresandeep Mar 12, 2019

risdenk Mar 12, 2019

rlevas commented Mar 12, 2019 •

edited

moresandeep commented Mar 12, 2019

moresandeep commented Mar 12, 2019 •

edited

moresandeep commented Mar 13, 2019

KNOX-1817 - Fix XSS issues with Alias API #70

KNOX-1817 - Fix XSS issues with Alias API #70

Conversation

moresandeep commented Mar 12, 2019

What changes were proposed in this pull request?

How was this patch tested?

moresandeep commented Mar 12, 2019

risdenk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlevas commented Mar 12, 2019 • edited

moresandeep commented Mar 12, 2019

moresandeep commented Mar 12, 2019 • edited

moresandeep commented Mar 13, 2019

rlevas commented Mar 12, 2019 •

edited

moresandeep commented Mar 12, 2019 •

edited