Remote Code Execution in OpenTSDB

@@ -40,15 +40,17 @@
 import com.google.common.base.Strings;
 import com.google.common.collect.Sets;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
+import net.opentsdb.core.*;
 import net.opentsdb.core.Const;
 import net.opentsdb.core.DataPoint;
 import net.opentsdb.core.DataPoints;
 import net.opentsdb.core.Query;
 import net.opentsdb.core.TSDB;
 import net.opentsdb.core.TSQuery;
+import net.opentsdb.core.Tags;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import net.opentsdb.graph.Plot;
 import net.opentsdb.meta.Annotation;
 import net.opentsdb.stats.Histogram;
@@ -667,6 +669,7 @@ static void setPlotDimensions(final HttpQuery query, final Plot plot) {
     String wxh = query.getQueryStringParam("wxh");
     if (wxh != null && !wxh.isEmpty()) {
       wxh = URLDecoder.decode(wxh.trim());
+      validateString("wxh", wxh);
       if (!WXH_VALIDATOR.matcher(wxh).find()) {
         throw new IllegalArgumentException("'wxh' was invalid. "
             + "Must satisfy the pattern " + WXH_VALIDATOR.toString());
@@ -744,48 +747,55 @@ static void setPlotParams(final HttpQuery query, final Plot plot) {
     final Map<String, List<String>> querystring = query.getQueryString();
     String value;
     if ((value = popParam(querystring, "yrange")) != null) {
+      validateString("yrange", value, "[:]");
       if (!RANGE_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'yrange' was invalid. "
             + "Must be in the format [min:max].");
       }
       params.put("yrange", value);
     }
     if ((value = popParam(querystring, "y2range")) != null) {
+      validateString("y2range", value, "[:]");
       if (!RANGE_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'y2range' was invalid. "
             + "Must be in the format [min:max].");
       }
       params.put("y2range", value);
     }
     if ((value = popParam(querystring, "ylabel")) != null) {
+      validateString("ylabel", value, " ");
       if (!LABEL_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'ylabel' was invalid. Must "
             + "satisfy the pattern " + LABEL_VALIDATOR.toString());
       }
       params.put("ylabel", stringify(value));
     }
     if ((value = popParam(querystring, "y2label")) != null) {
+      validateString("y2label", value, " ");
       if (!LABEL_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'y2label' was invalid. Must "
             + "satisfy the pattern " + LABEL_VALIDATOR.toString());
       }
       params.put("y2label", stringify(value));
     }
     if ((value = popParam(querystring, "yformat")) != null) {
+      validateString("yformat", value, "% ");
       if (!FORMAT_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'yformat' was invalid. Must "
             + "satisfy the pattern " + FORMAT_VALIDATOR.toString());
       }
       params.put("format y", stringify(value));
     }
     if ((value = popParam(querystring, "y2format")) != null) {
+      validateString("y2format", value, "% ");
       if (!FORMAT_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'y2format' was invalid. Must "
             + "satisfy the pattern " + FORMAT_VALIDATOR.toString());
       }
       params.put("format y2", stringify(value));
     }
     if ((value = popParam(querystring, "xformat")) != null) {
+      validateString("xformat", value, "% ");
       if (!FORMAT_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'xformat' was invalid. Must "
             + "satisfy the pattern " + FORMAT_VALIDATOR.toString());
@@ -799,41 +809,47 @@ static void setPlotParams(final HttpQuery query, final Plot plot) {
       params.put("logscale y2", "");
     }
     if ((value = popParam(querystring, "key")) != null) {
+      validateString("key", value);
       if (!KEY_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'key' was invalid. Must "
             + "satisfy the pattern " + KEY_VALIDATOR.toString());
       }
       params.put("key", value);
     }
     if ((value = popParam(querystring, "title")) != null) {
+      validateString("title", value, " ");
       if (!LABEL_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'title' was invalid. Must "
             + "satisfy the pattern " + LABEL_VALIDATOR.toString());
       }
       params.put("title", stringify(value));
     }
     if ((value = popParam(querystring, "bgcolor")) != null) {
+      validateString("bgcolor", value);
       if (!COLOR_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'bgcolor' was invalid. Must "
             + "be a hex value e.g. 'xFFFFFF'");
       }
       params.put("bgcolor", value);
     }
     if ((value = popParam(querystring, "fgcolor")) != null) {
+      validateString("fgcolor", value);
       if (!COLOR_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'fgcolor' was invalid. Must "
             + "be a hex value e.g. 'xFFFFFF'");
       }
       params.put("fgcolor", value);
     }
     if ((value = popParam(querystring, "smooth")) != null) {
+      validateString("smooth", value);
       if (!SMOOTH_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'smooth' was invalid. Must "
             + "satisfy the pattern " + SMOOTH_VALIDATOR.toString());
       }
       params.put("smooth", value);
     }
     if ((value = popParam(querystring, "style")) != null) {
+      validateString("style", value);
       if (!STYLE_VALIDATOR.matcher(value).find()) {
         throw new BadRequestException("'style' was invalid. Must "
             + "satisfy the pattern " + STYLE_VALIDATOR.toString());
@@ -1071,4 +1087,26 @@ static void logError(final HttpQuery query, final String msg,
     LOG.error(query.channel().toString() + ' ' + msg, e);
   }
 
+  static void validateString(final String what, final String s) {
+    validateString(what, s, "");
+  }
+
+  public static void validateString(final String what, final String s, String specials) {
+    if (s == null) {
+      throw new BadRequestException("Invalid " + what + ": null");
+    } else if ("".equals(s)) {
+      throw new BadRequestException("Invalid " + what + ": empty string");
+    }
+    final int n = s.length();
+    for (int i = 0; i < n; i++) {
+      final char c = s.charAt(i);
+      if (!(('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z')
+          || ('0' <= c && c <= '9') || c == '-' || c == '_' || c == '.'
+          || c == '/' || Character.isLetter(c) || specials.indexOf(c) != -1)) {
+        throw new BadRequestException("Invalid " + what
+            + " (\"" + s + "\"): illegal character: " + c);
+      }
+    }
+  }
+
 }

@@ -97,6 +97,7 @@ public void setYRangeParams() throws Exception {
     assertPlotParam("yrange", "[-10.1e-5:]");
     assertPlotParam("yrange", "[-10.1e-5:-10.1e-6]");
     assertInvalidPlotParam("yrange", "[33:system('touch /tmp/poc.txt')]");
+    assertInvalidPlotParam("y2range", "[42:%0a[33:system('touch /tmp/poc.txt')]");
   }
 
   @Test
@@ -109,7 +110,8 @@ public void setKeyParams() throws Exception {
     assertPlotParam("key", "horiz");
     assertPlotParam("key", "box");
     assertPlotParam("key", "bottom");
-    assertInvalidPlotParam("yrange", "out%20right%20top%0aset%20yrange%20[33:system(%20");
+    assertInvalidPlotParam("key", "out%20right%20top%0aset%20yrange%20[33:system(%20");
+    assertInvalidPlotParam("key", "%3Bsystem%20%22cat%20/home/ubuntuvm/secret.txt%20%3E/tmp/secret.txt%22%20%22");
   }
 
   @Test
@@ -118,16 +120,23 @@ public void setStyleParams() throws Exception {
     assertPlotParam("style", "points");
     assertPlotParam("style", "circles");
     assertPlotParam("style", "dots");
-    assertInvalidPlotParam("style", "dots%20[33:system(%20");
+    assertInvalidPlotParam("style", "dots%20%0a[33:system(%20");
+    assertInvalidPlotParam("style", "%3Bsystem%20%22cat%20/home/ubuntuvm/secret.txt%20%3E/tmp/secret.txt%22%20%22\"");
   }
 
   @Test
   public void setLabelParams() throws Exception {
     assertPlotParam("ylabel", "This is good");
     assertPlotParam("ylabel", " and so Is this - _ yay");
-    assertInvalidPlotParam("ylabel", "[33:system(%20");
-    assertInvalidPlotParam("title", "[33:system(%20");
-    assertInvalidPlotParam("y2label", "[33:system(%20");
+    assertInvalidPlotParam("ylabel", "system(%20no%0anewlines");
+    assertInvalidPlotParam("title", "system(%20no%0anewlines");
+    assertInvalidPlotParam("y2label", "system(%20no%0anewlines");
+  }
+
+  @Test
+  public void setWXH() throws Exception {
+    assertPlotDimension("wxh",  "720x640");
+    assertInvalidPlotDimension("wxh", "720%0ax640");
   }
 
   @Test
@@ -137,12 +146,14 @@ public void setColorParams() throws Exception {
     assertPlotParam("bgcolor", "%58DEADBE");
     assertInvalidPlotParam("bgcolor", "XDEADBEF");
     assertInvalidPlotParam("bgcolor", "%5BDEADBE");
+    assertInvalidPlotParam("bgcolor", "xBDE%0AAD");
 
     assertPlotParam("fgcolor", "x000000");
     assertPlotParam("fgcolor", "XDEADBE");
     assertPlotParam("fgcolor", "%58DEADBE");
     assertInvalidPlotParam("fgcolor", "XDEADBEF");
     assertInvalidPlotParam("fgcolor", "%5BDEADBE");
+    assertInvalidPlotParam("fgcolor", "xBDE%0AAD");
   }
 
   @Test
@@ -160,7 +171,8 @@ public void setSmoothParams() throws Exception {
     assertPlotParam("smooth", "sbezier");
     assertPlotParam("smooth", "unwrap");
     assertPlotParam("smooth", "zsort");
-    assertInvalidPlotParam("smooth", "[33:system(%20");
+    assertInvalidPlotParam("smooth", "bezier%20system(%20");
+    assertInvalidPlotParam("smooth", "fnormal%0asystem(%20");
   }
 
   @Test
@@ -172,7 +184,8 @@ public void setFormatParams() throws Exception {
     assertPlotParam("yformat", "%253.0em%25%25");
     assertPlotParam("yformat", "%25.2f seconds");
     assertPlotParam("yformat", "%25.0f ms");
-    assertInvalidPlotParam("yformat", "%252.[33:system");
+    assertInvalidPlotParam("yformat", "%252.system(%20");
+    assertInvalidPlotParam("yformat", "%252.%0asystem(%20");
   }
 
   @Test  // If the file doesn't exist, we don't use it, obviously.
@@ -344,6 +357,13 @@ private static void assertPlotParam(String param, String value) {
     GraphHandler.setPlotParams(query, plot);
   }
 
+  private static void assertPlotDimension(String param, String value) {
+    Plot plot = mock(Plot.class);
+    HttpQuery query = mock(HttpQuery.class);
+    when(query.getQueryStringParam(param)).thenReturn(value);
+    GraphHandler.setPlotParams(query, plot);
+  }
+
   private static void assertInvalidPlotParam(String param, String value) {
     Plot plot = mock(Plot.class);
     HttpQuery query = mock(HttpQuery.class);
@@ -357,4 +377,14 @@ private static void assertInvalidPlotParam(String param, String value) {
     } catch (BadRequestException e) { }
   }
 
+  private static void assertInvalidPlotDimension(String param, String value) {
+    Plot plot = mock(Plot.class);
+    HttpQuery query = mock(HttpQuery.class);
+    when(query.getQueryStringParam(param)).thenReturn(value);
+    try {
+      GraphHandler.setPlotDimensions(query, plot);
+      fail("Expected BadRequestException");
+    } catch (BadRequestException e) { }
+  }
+
 }

@@ -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;
@@ -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>"));
     }
   }
@@ -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>"));
     }
   }

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

@@ -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 'foo': 'metrics'"));
   }
 
   @Test
@@ -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 'foo': 'metrics'"));
   }
 
   @Test