Skip to content

Commit

Permalink
Improved fix for OpenTSDB#2261.
Browse files Browse the repository at this point in the history 
Regular expressions wouldn't catch the newlines or possibly other
control characters. Now we'll use the TAG validation code to make
sure the inputs are only plain ASCII printables first.
Fixes CVE-2018-12972, CVE-2020-35476
  • Loading branch information
@manolama
manolama committed Apr 11, 2023
1 parent 22b27ea commit 07c4641
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 10 deletions.
44 changes: 41 additions & 3 deletions src/tsd/GraphHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand All @@ -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());
Expand Down Expand Up @@ -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);
}
}
}

}
44 changes: 37 additions & 7 deletions test/tsd/TestGraphHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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);
Expand All @@ -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) { }
}

}

0 comments on commit 07c4641

Please sign in to comment.