From ef4e190b54e86e8c85c968dd90163542534517b8 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Thu, 26 May 2022 23:50:29 -0700 Subject: [PATCH 1/5] Escape HTML special characters in the error message of no handler found for a REST request Signed-off-by: Tianli Feng --- .../org/opensearch/rest/RestController.java | 24 ++++++++++++++++++- .../opensearch/rest/RestControllerTests.java | 12 ++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index b576f8b83e5a0..9b458c8613ac6 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -488,7 +488,9 @@ private void handleBadRequest(String uri, RestRequest.Method method, RestChannel try (XContentBuilder builder = channel.newErrorBuilder()) { builder.startObject(); { - builder.field("error", "no handler found for uri [" + uri + "] and method [" + method + "]"); + // Escaping HTML special characters in the error message only aims to satisfy common security scanners. + // There is no vulnerability without escaping, because the response MIME type is application/json, no scripts will be run. + builder.field("error", "no handler found for uri [" + escapeHtml(uri) + "] and method [" + method + "]"); } builder.endObject(); channel.sendResponse(new BytesRestResponse(BAD_REQUEST, builder)); @@ -578,4 +580,24 @@ private static CircuitBreaker inFlightRequestsBreaker(CircuitBreakerService circ // We always obtain a fresh breaker to reflect changes to the breaker configuration. return circuitBreakerService.getBreaker(CircuitBreaker.IN_FLIGHT_REQUESTS); } + + /** + * Perform an HTML escape operation on a String input to prevent XSS vulnerability. + * @param text the String to be escaped. + * @return The escaped result String. + */ + private String escapeHtml(String text) { + StringBuilder out = new StringBuilder(); + for (int i = 0; i < text.length(); i++) { + char c = text.charAt(i); + if (c == '"' || c == '\'' || c == '<' || c == '>' || c == '&') { + out.append("&#"); + out.append((int) c); + out.append(';'); + } else { + out.append(c); + } + } + return out.toString(); + } } diff --git a/server/src/test/java/org/opensearch/rest/RestControllerTests.java b/server/src/test/java/org/opensearch/rest/RestControllerTests.java index 6004613c0ed17..34599914b3639 100644 --- a/server/src/test/java/org/opensearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/opensearch/rest/RestControllerTests.java @@ -553,6 +553,18 @@ public void testFaviconWithWrongHttpMethod() { assertThat(channel.getRestResponse().getHeaders().get("Allow"), hasItem(equalTo(RestRequest.Method.GET.toString()))); } + public void testHandleBadRequestWithHtmlSpecialCharsInUri() { + final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withPath( + "/" + ).build(); + final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.BAD_REQUEST); + restController.dispatchRequest(fakeRestRequest, channel, client.threadPool().getThreadContext()); + assertThat( + channel.getRestResponse().content().utf8ToString(), + containsString("/<script>alert('xss');alert("&#x6A&#x61&#x76&#x61");</script>") + ); + } + public void testDispatchUnsupportedHttpMethod() { final boolean hasContent = randomBoolean(); final RestRequest request = RestRequest.request(xContentRegistry(), new HttpRequest() { From 34f1b8f9db7b906cc4cf6668f9337c593f5465c0 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Wed, 1 Jun 2022 14:51:45 -0700 Subject: [PATCH 2/5] Filter out invalid URI in the error message of no handler found for a REST request Signed-off-by: Tianli Feng --- .../org/opensearch/rest/RestController.java | 32 ++++++------------- .../opensearch/rest/RestControllerTests.java | 5 +-- 2 files changed, 10 insertions(+), 27 deletions(-) diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index 9b458c8613ac6..bf6d3df204dc7 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -56,6 +56,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; +import java.net.URI; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -488,9 +489,14 @@ private void handleBadRequest(String uri, RestRequest.Method method, RestChannel try (XContentBuilder builder = channel.newErrorBuilder()) { builder.startObject(); { - // Escaping HTML special characters in the error message only aims to satisfy common security scanners. - // There is no vulnerability without escaping, because the response MIME type is application/json, no scripts will be run. - builder.field("error", "no handler found for uri [" + escapeHtml(uri) + "] and method [" + method + "]"); + try { + // Validate input URI to filter out HTML special characters in the error message, + // in case false-positive cross site scripting vulnerability is detected by common security scanners. + uri = new URI(uri).getPath(); + builder.field("error", "no handler found for uri [" + uri + "] and method [" + method + "]"); + } catch (Exception e) { + builder.field("error", "invalid uri has been requested"); + } } builder.endObject(); channel.sendResponse(new BytesRestResponse(BAD_REQUEST, builder)); @@ -580,24 +586,4 @@ private static CircuitBreaker inFlightRequestsBreaker(CircuitBreakerService circ // We always obtain a fresh breaker to reflect changes to the breaker configuration. return circuitBreakerService.getBreaker(CircuitBreaker.IN_FLIGHT_REQUESTS); } - - /** - * Perform an HTML escape operation on a String input to prevent XSS vulnerability. - * @param text the String to be escaped. - * @return The escaped result String. - */ - private String escapeHtml(String text) { - StringBuilder out = new StringBuilder(); - for (int i = 0; i < text.length(); i++) { - char c = text.charAt(i); - if (c == '"' || c == '\'' || c == '<' || c == '>' || c == '&') { - out.append("&#"); - out.append((int) c); - out.append(';'); - } else { - out.append(c); - } - } - return out.toString(); - } } diff --git a/server/src/test/java/org/opensearch/rest/RestControllerTests.java b/server/src/test/java/org/opensearch/rest/RestControllerTests.java index 34599914b3639..4bd72adb6bbc4 100644 --- a/server/src/test/java/org/opensearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/opensearch/rest/RestControllerTests.java @@ -559,10 +559,7 @@ public void testHandleBadRequestWithHtmlSpecialCharsInUri() { ).build(); final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.BAD_REQUEST); restController.dispatchRequest(fakeRestRequest, channel, client.threadPool().getThreadContext()); - assertThat( - channel.getRestResponse().content().utf8ToString(), - containsString("/<script>alert('xss');alert("&#x6A&#x61&#x76&#x61");</script>") - ); + assertThat(channel.getRestResponse().content().utf8ToString(), containsString("invalid uri has been requested")); } public void testDispatchUnsupportedHttpMethod() { From 863e272da551414215e3b16eb3f9c8d6e84924eb Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Wed, 1 Jun 2022 16:11:13 -0700 Subject: [PATCH 3/5] Filter out invalid HTTP method in the error message of no handler found for a REST request Signed-off-by: Tianli Feng --- server/src/main/java/org/opensearch/rest/RestController.java | 4 +++- .../test/java/org/opensearch/rest/RestControllerTests.java | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index bf6d3df204dc7..cc9a65dc5ce89 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -448,7 +448,9 @@ private void handleUnsupportedHttpMethod( msg.append("Incorrect HTTP method for uri [").append(uri); msg.append("] and method [").append(method).append("]"); } else { - msg.append(exception.getMessage()); + // Not using the error message directly from 'exception.getMessage()' to avoid unescaped HTML special characters, + // in case false-positive cross site scripting vulnerability is detected by common security scanners. + msg.append("Unexpected http method"); } if (validMethodSet.isEmpty() == false) { msg.append(", allowed: ").append(validMethodSet); diff --git a/server/src/test/java/org/opensearch/rest/RestControllerTests.java b/server/src/test/java/org/opensearch/rest/RestControllerTests.java index 4bd72adb6bbc4..ce649db307a61 100644 --- a/server/src/test/java/org/opensearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/opensearch/rest/RestControllerTests.java @@ -632,6 +632,7 @@ public Exception getInboundException() { assertTrue(channel.getSendResponseCalled()); assertThat(channel.getRestResponse().getHeaders().containsKey("Allow"), equalTo(true)); assertThat(channel.getRestResponse().getHeaders().get("Allow"), hasItem(equalTo(RestRequest.Method.GET.toString()))); + assertThat(channel.getRestResponse().content().utf8ToString(), containsString("Unexpected http method")); } private static final class TestHttpServerTransport extends AbstractLifecycleComponent implements HttpServerTransport { From c586b5bb345491188dedda372ed7176905f49e93 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Wed, 1 Jun 2022 16:36:23 -0700 Subject: [PATCH 4/5] Use uppercase HTTP in the error message Signed-off-by: Tianli Feng --- server/src/main/java/org/opensearch/rest/RestController.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index cc9a65dc5ce89..78bebcb9a0af1 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -450,7 +450,7 @@ private void handleUnsupportedHttpMethod( } else { // Not using the error message directly from 'exception.getMessage()' to avoid unescaped HTML special characters, // in case false-positive cross site scripting vulnerability is detected by common security scanners. - msg.append("Unexpected http method"); + msg.append("Unexpected HTTP method"); } if (validMethodSet.isEmpty() == false) { msg.append(", allowed: ").append(validMethodSet); From 3a7a48402d0ed511f835b5c217657e447b6cf546 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Wed, 1 Jun 2022 18:31:02 -0700 Subject: [PATCH 5/5] Use equalTo instead of containsString to test error message Signed-off-by: Tianli Feng --- .../test/java/org/opensearch/rest/RestControllerTests.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/rest/RestControllerTests.java b/server/src/test/java/org/opensearch/rest/RestControllerTests.java index ce649db307a61..bd4c7c9a4f824 100644 --- a/server/src/test/java/org/opensearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/opensearch/rest/RestControllerTests.java @@ -632,7 +632,10 @@ public Exception getInboundException() { assertTrue(channel.getSendResponseCalled()); assertThat(channel.getRestResponse().getHeaders().containsKey("Allow"), equalTo(true)); assertThat(channel.getRestResponse().getHeaders().get("Allow"), hasItem(equalTo(RestRequest.Method.GET.toString()))); - assertThat(channel.getRestResponse().content().utf8ToString(), containsString("Unexpected http method")); + assertThat( + channel.getRestResponse().content().utf8ToString(), + equalTo("{\"error\":\"Unexpected HTTP method, allowed: [GET]\",\"status\":405}") + ); } private static final class TestHttpServerTransport extends AbstractLifecycleComponent implements HttpServerTransport {