Skip to content

Commit

Permalink
Fix for #2269 and #2267 XSS vulnerability.
Browse files Browse the repository at this point in the history
Escaping the user supplied input when outputing the HTML for the old BadRequest
HTML handlers should help. Thanks to the reporters.
Fixes CVE-2018-13003.
  • Loading branch information
manolama committed Apr 11, 2023
1 parent 07c4641 commit fa88d3e
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 4 deletions.
13 changes: 11 additions & 2 deletions src/tsd/HttpQuery.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.HashSet;
import java.util.List;

import com.google.common.html.HtmlEscapers;
import net.opentsdb.core.Const;
import net.opentsdb.core.TSDB;
import net.opentsdb.graph.Plot;
Expand Down Expand Up @@ -373,14 +374,18 @@ public void internalError(final Exception cause) {
buf.append("\"}");
sendReply(HttpResponseStatus.INTERNAL_SERVER_ERROR, buf);
} else {
String response = "";
if (pretty_exc != null) {
response = HtmlEscapers.htmlEscaper().escape(pretty_exc);
}
sendReply(HttpResponseStatus.INTERNAL_SERVER_ERROR,
makePage("Internal Server Error", "Houston, we have a problem",
"<blockquote>"
+ "<h1>Internal Server Error</h1>"
+ "Oops, sorry but your request failed due to a"
+ " server error.<br/><br/>"
+ "Please try again in 30 seconds.<pre>"
+ pretty_exc
+ response
+ "</pre></blockquote>"));
}
}
Expand Down Expand Up @@ -420,14 +425,18 @@ public void badRequest(final BadRequestException exception) {
buf.append("\"}");
sendReply(HttpResponseStatus.BAD_REQUEST, buf);
} else {
String response = "";
if (exception.getMessage() != null) {
response = HtmlEscapers.htmlEscaper().escape(exception.getMessage());
}
sendReply(HttpResponseStatus.BAD_REQUEST,
makePage("Bad Request", "Looks like it's your fault this time",
"<blockquote>"
+ "<h1>Bad Request</h1>"
+ "Sorry but your request was rejected as being"
+ " invalid.<br/><br/>"
+ "The reason provided was:<blockquote>"
+ exception.getMessage()
+ response
+ "</blockquote></blockquote>"));
}
}
Expand Down
23 changes: 23 additions & 0 deletions test/tsd/TestHttpQuery.java
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,18 @@ public void internalErrorDeprecated() {
query.response().getContent().toString(Charset.forName("UTF-8"))
.substring(0, 15));
}

@Test
public void internalErrorDeprecatedHTMLEscaped() {
HttpQuery query = NettyMocks.getQuery(tsdb, "");
query.internalError(new Exception("<script>alert(document.cookie)</script>"));

assertEquals(HttpResponseStatus.INTERNAL_SERVER_ERROR,
query.response().getStatus());
assertTrue(query.response().getContent().toString(Charset.forName("UTF-8")).contains(
"&lt;script&gt;alert(document.cookie)&lt;/script&gt;"
));
}

@Test
public void internalErrorDeprecatedJSON() {
Expand Down Expand Up @@ -849,6 +861,17 @@ public void badRequestDeprecated() {
query.response().getContent().toString(Charset.forName("UTF-8"))
.substring(0, 15));
}

@Test
public void badRequestDeprecatedHTMLEscaped() {
HttpQuery query = NettyMocks.getQuery(tsdb, "/");
query.badRequest(new BadRequestException("<script>alert(document.cookie)</script>"));

assertEquals(HttpResponseStatus.BAD_REQUEST, query.response().getStatus());
assertTrue(query.response().getContent().toString(Charset.forName("UTF-8")).contains(
"The reason provided was:<blockquote>&lt;script&gt;alert(document.cookie)&lt;/script&gt;"
));
}

@Test
public void badRequestDeprecatedJSON() {
Expand Down
4 changes: 2 additions & 2 deletions test/tsd/TestQueryRpc.java
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ public void postQueryNoMetricBadRequest() throws Exception {
assertEquals(HttpResponseStatus.BAD_REQUEST, query.response().getStatus());
final String json =
query.response().getContent().toString(Charset.forName("UTF-8"));
assertTrue(json.contains("No such name for 'foo': 'metrics'"));
assertTrue(json.contains("No such name for &#39;foo&#39;: &#39;metrics&#39;"));
}

@Test
Expand Down Expand Up @@ -579,7 +579,7 @@ public void executeNSU() throws Exception {
assertEquals(HttpResponseStatus.BAD_REQUEST, query.response().getStatus());
final String json =
query.response().getContent().toString(Charset.forName("UTF-8"));
assertTrue(json.contains("No such name for 'foo': 'metrics'"));
assertTrue(json.contains("No such name for &#39;foo&#39;: &#39;metrics&#39;"));
}

@Test
Expand Down

0 comments on commit fa88d3e

Please sign in to comment.