CVE-2023-36812: Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection') (CWE-74)
value: OpenTSDB is a open source, distributed, scalable Time Series Database (TSDB). OpenTSDB is vulnerable to Remote Code Execution vulnerability by writing user-controlled input to Gnuplot configuration file and running Gnuplot with the generated configuration. This issue has been patched in  commit `07c4641471c` and further refined in commit `fa88d3e4b`. These patches are available in the `2.4.2` release. Users are advised to upgrade. User unable to upgrade may disable Gunuplot via the config option`tsd.core.enable_ui = true` and remove the shell files `mygnuplot.bat` and `mygnuplot.sh`.
CWE description: The product constructs all or part of a command, data structure, or record using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify how it is parsed or interpreted when it is sent to a downstream component.
CVSS v3 base score: 9.8
Published: 2023-06-30T23:15Z
Repository: https://github.com/OpenTSDB/opentsdb
Language: Java
Files changed: src/tsd/GraphHandler.java, test/tsd/TestGraphHandler.java
Commit date: 2023-04-11 09:10:29 -0700
Commit message: Improved fix for #2261.
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

Diff: diff --git a/src/tsd/GraphHandler.java b/src/tsd/GraphHandler.java
index bf321e8..2af125e 100644
--- a/src/tsd/GraphHandler.java
+++ b/src/tsd/GraphHandler.java
@@ -38,19 +38,21 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import com.fasterxml.jackson.core.JsonParseException;
 import com.fasterxml.jackson.databind.JsonMappingException;
 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;
 import net.opentsdb.stats.StatsCollector;
 import net.opentsdb.tools.GnuplotInstaller;
@@ -665,10 +667,11 @@ final class GraphHandler implements HttpRpc {
   /** Parses the {@code wxh} query parameter to set the graph dimension. */
   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());
       }
       final int wxhlength = wxh.length();
@@ -742,52 +745,59 @@ final class GraphHandler implements HttpRpc {
   static void setPlotParams(final HttpQuery query, final Plot plot) {
     final HashMap<String, String> params = new HashMap<String, String>();
     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());
       }
       params.put("format x", stringify(value));
@@ -797,45 +807,51 @@ final class GraphHandler implements HttpRpc {
     }
     if ((value = popParam(querystring, "y2log")) != null) {
       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());
       }
       params.put("style", value);
@@ -1069,6 +1085,28 @@ final class GraphHandler implements HttpRpc {
   static void logError(final HttpQuery query, final String msg,
                        final Throwable e) {
     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);
+      }
+    }
+  }
+
 }
diff --git a/test/tsd/TestGraphHandler.java b/test/tsd/TestGraphHandler.java
index d2fbca4..d2600f8 100644
--- a/test/tsd/TestGraphHandler.java
+++ b/test/tsd/TestGraphHandler.java
@@ -95,10 +95,11 @@ public final class TestGraphHandler {
     assertPlotParam("yrange", "[-10e-5:]");
     assertPlotParam("yrange", "[10.1e-5:]");
     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
   public void setKeyParams() throws Exception {
     assertPlotParam("key", "out");
@@ -107,44 +108,54 @@ public final class TestGraphHandler {
     assertPlotParam("key", "center");
     assertPlotParam("key", "right");
     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
   public void setStyleParams() throws Exception {
     assertPlotParam("style", "linespoint");
     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
   public void setColorParams() throws Exception {
     assertPlotParam("bgcolor", "x000000");
     assertPlotParam("bgcolor", "XDEADBE");
     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
   public void setSmoothParams() throws Exception {
     assertPlotParam("smooth", "unique");
@@ -158,11 +169,12 @@ public final class TestGraphHandler {
     assertPlotParam("smooth", "mcsplines");
     assertPlotParam("smooth", "bezier");
     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
   public void setFormatParams() throws Exception {
     assertPlotParam("yformat", "%25.2f");
@@ -170,11 +182,12 @@ public final class TestGraphHandler {
     assertPlotParam("xformat", "%25.2f");
     assertPlotParam("yformat", "%253.0em");
     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.
   public void staleCacheFileDoesntExist() throws Exception {
     final File cachedfile = fakeFile("/cache/fake-file");
@@ -342,10 +355,17 @@ public final class TestGraphHandler {
 
     params.put(param, Lists.newArrayList(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);
     Map<String, List<String>> params = Maps.newHashMap();
     when(query.getQueryString()).thenReturn(params);
@@ -355,6 +375,16 @@ public final class TestGraphHandler {
       GraphHandler.setPlotParams(query, plot);
       fail("Expected BadRequestException");
     } 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) { }
+  }
+
 }

Vulnerable code: import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

    assertInvalidPlotParam("yrange", "out%20right%20top%0aset%20yrange%20[33:system(%20");
    assertInvalidPlotParam("style", "dots%20[33:system(%20");
    assertInvalidPlotParam("ylabel", "[33:system(%20");
    assertInvalidPlotParam("title", "[33:system(%20");
    assertInvalidPlotParam("y2label", "[33:system(%20");
    assertInvalidPlotParam("smooth", "[33:system(%20");
    assertInvalidPlotParam("yformat", "%252.[33:system");
Fixed code: import net.opentsdb.core.*;
import net.opentsdb.core.Tags;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

      validateString("wxh", wxh);
      validateString("yrange", value, "[:]");
      validateString("y2range", value, "[:]");
      validateString("ylabel", value, " ");
      validateString("y2label", value, " ");
      validateString("yformat", value, "% ");
      validateString("y2format", value, "% ");
      validateString("xformat", value, "% ");
      validateString("key", value);
      validateString("title", value, " ");
      validateString("bgcolor", value);
      validateString("fgcolor", value);
      validateString("smooth", value);
      validateString("style", value);
  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);
      }
    }
  }

    assertInvalidPlotParam("y2range", "[42:%0a[33:system('touch /tmp/poc.txt')]");
    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");
    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\"");
    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");
    assertInvalidPlotParam("bgcolor", "xBDE%0AAD");
    assertInvalidPlotParam("fgcolor", "xBDE%0AAD");
    assertInvalidPlotParam("smooth", "bezier%20system(%20");
    assertInvalidPlotParam("smooth", "fnormal%0asystem(%20");
    assertInvalidPlotParam("yformat", "%252.system(%20");
    assertInvalidPlotParam("yformat", "%252.%0asystem(%20");
  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 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) { }
  }