Skip to content

Commit 22b27ea

Browse files
manolamamanolama
committed
Tighten up the regexes for Gnuplot URI params per multiple security reports.
The best way of avoiding RCEs is to disable Gnuplot, but this should help a little.
1 parent 9b62442 commit 22b27ea

File tree

2 files changed

+135
-100
lines changed

2 files changed

+135
-100
lines changed

‎src/tsd/GraphHandler.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,15 +72,15 @@ final class GraphHandler implements HttpRpc {
7272

7373
private static final String RANGE_COMPONENT = "\\\"?-?\\d*\\.?(\\d+)?([eE]-?\\d+)?\\\"?";
7474
private static Pattern RANGE_VALIDATOR = Pattern.compile(
75-
"\\["+RANGE_COMPONENT+":"+RANGE_COMPONENT+"]");
76-
private static Pattern LABEL_VALIDATOR = Pattern.compile("[a-zA-z0-9 \\-_]");
75+
"^\\["+RANGE_COMPONENT+":"+RANGE_COMPONENT+"]$");
76+
private static Pattern LABEL_VALIDATOR = Pattern.compile("^[a-zA-z0-9 \\-_]+$");
7777
private static Pattern KEY_VALIDATOR = Pattern.compile(
78-
"(out|left|top|center|right|horiz|box|bottom)?\\s?");
79-
private static Pattern STYLE_VALIDATOR = Pattern.compile("(linespoint|points|circles|dots)");
80-
private static Pattern COLOR_VALIDATOR = Pattern.compile("(x|X)[a-fA-F0-9]{6}");
81-
private static Pattern SMOOTH_VALIDATOR = Pattern.compile("unique|frequency|fnormal|cumulative|cnormal|bins|csplines|acsplines|mcsplines|bezier|sbezier|unwrap|zsort");
78+
"^out|left|top|center|right|horiz|box|bottom$");
79+
private static Pattern STYLE_VALIDATOR = Pattern.compile("^linespoint|points|circles|dots$");
80+
private static Pattern COLOR_VALIDATOR = Pattern.compile("^(x|X)[a-fA-F0-9]{6}$");
81+
private static Pattern SMOOTH_VALIDATOR = Pattern.compile("^unique|frequency|fnormal|cumulative|cnormal|bins|csplines|acsplines|mcsplines|bezier|sbezier|unwrap|zsort$");
8282
// NOTE: This one should be tightened for only time based formatters.
83-
private static Pattern FORMAT_VALIDATOR = Pattern.compile("(%[a-zA-Z])+[:\\/]?\\s?");
83+
private static Pattern FORMAT_VALIDATOR = Pattern.compile("^[%0-9.a-zA-Z \\-]+$");
8484
private static Pattern WXH_VALIDATOR = Pattern.compile("^\\d+x\\d+$");
8585
/** Number of times we had to do all the work up to running Gnuplot. */
8686
private static final AtomicInteger graphs_generated

‎test/tsd/TestGraphHandler.java‎

Lines changed: 128 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -67,102 +67,114 @@ public final class TestGraphHandler {
6767

6868
@Test
6969
public void setYRangeParams() throws Exception {
70-
Plot plot = mock(Plot.class);
71-
HttpQuery query = mock(HttpQuery.class);
72-
Map<String, List<String>> params = Maps.newHashMap();
73-
when(query.getQueryString()).thenReturn(params);
70+
assertPlotParam("yrange","[0:1]");
71+
assertPlotParam("yrange", "[:]");
72+
assertPlotParam("yrange", "[:0]");
73+
assertPlotParam("yrange", "[:42]");
74+
assertPlotParam("yrange", "[:-42]");
75+
assertPlotParam("yrange", "[:0.8]");
76+
assertPlotParam("yrange", "[:-0.8]");
77+
assertPlotParam("yrange", "[:42.4]");
78+
assertPlotParam("yrange", "[:-42.4]");
79+
assertPlotParam("yrange", "[:4e4]");
80+
assertPlotParam("yrange", "[:-4e4]");
81+
assertPlotParam("yrange", "[:4e-4]");
82+
assertPlotParam("yrange", "[:-4e-4]");
83+
assertPlotParam("yrange", "[:4.2e4]");
84+
assertPlotParam("yrange", "[:-4.2e4]");
85+
assertPlotParam("yrange", "[0:]");
86+
assertPlotParam("yrange", "[5:]");
87+
assertPlotParam("yrange", "[-5:]");
88+
assertPlotParam("yrange", "[0.5:]");
89+
assertPlotParam("yrange", "[-0.5:]");
90+
assertPlotParam("yrange", "[10.5:]");
91+
assertPlotParam("yrange", "[-10.5:]");
92+
assertPlotParam("yrange", "[10e5:]");
93+
assertPlotParam("yrange", "[-10e5:]");
94+
assertPlotParam("yrange", "[10e-5:]");
95+
assertPlotParam("yrange", "[-10e-5:]");
96+
assertPlotParam("yrange", "[10.1e-5:]");
97+
assertPlotParam("yrange", "[-10.1e-5:]");
98+
assertPlotParam("yrange", "[-10.1e-5:-10.1e-6]");
99+
assertInvalidPlotParam("yrange", "[33:system('touch /tmp/poc.txt')]");
100+
}
74101

75-
params.put("yrange", Lists.newArrayList("[0:1]"));
76-
GraphHandler.setPlotParams(query, plot);
77-
78-
params.put("yrange", Lists.newArrayList("[:]"));
79-
GraphHandler.setPlotParams(query, plot);
80-
81-
params.put("yrange", Lists.newArrayList("[:0]"));
82-
GraphHandler.setPlotParams(query, plot);
83-
84-
params.put("yrange", Lists.newArrayList("[:42]"));
85-
GraphHandler.setPlotParams(query, plot);
86-
87-
params.put("yrange", Lists.newArrayList("[:-42]"));
88-
GraphHandler.setPlotParams(query, plot);
89-
90-
params.put("yrange", Lists.newArrayList("[:0.8]"));
91-
GraphHandler.setPlotParams(query, plot);
92-
93-
params.put("yrange", Lists.newArrayList("[:-0.8]"));
94-
GraphHandler.setPlotParams(query, plot);
95-
96-
params.put("yrange", Lists.newArrayList("[:42.4]"));
97-
GraphHandler.setPlotParams(query, plot);
98-
99-
params.put("yrange", Lists.newArrayList("[:-42.4]"));
100-
GraphHandler.setPlotParams(query, plot);
101-
102-
params.put("yrange", Lists.newArrayList("[:4e4]"));
103-
GraphHandler.setPlotParams(query, plot);
104-
105-
params.put("yrange", Lists.newArrayList("[:-4e4]"));
106-
GraphHandler.setPlotParams(query, plot);
107-
108-
params.put("yrange", Lists.newArrayList("[:4e-4]"));
109-
GraphHandler.setPlotParams(query, plot);
110-
111-
params.put("yrange", Lists.newArrayList("[:-4e-4]"));
112-
GraphHandler.setPlotParams(query, plot);
113-
114-
params.put("yrange", Lists.newArrayList("[:4.2e4]"));
115-
GraphHandler.setPlotParams(query, plot);
116-
117-
params.put("yrange", Lists.newArrayList("[:-4.2e4]"));
118-
GraphHandler.setPlotParams(query, plot);
119-
120-
params.put("yrange", Lists.newArrayList("[0:]"));
121-
GraphHandler.setPlotParams(query, plot);
122-
123-
params.put("yrange", Lists.newArrayList("[5:]"));
124-
GraphHandler.setPlotParams(query, plot);
125-
126-
params.put("yrange", Lists.newArrayList("[-5:]"));
127-
GraphHandler.setPlotParams(query, plot);
128-
129-
params.put("yrange", Lists.newArrayList("[0.5:]"));
130-
GraphHandler.setPlotParams(query, plot);
131-
132-
params.put("yrange", Lists.newArrayList("[-0.5:]"));
133-
GraphHandler.setPlotParams(query, plot);
102+
@Test
103+
public void setKeyParams() throws Exception {
104+
assertPlotParam("key", "out");
105+
assertPlotParam("key", "left");
106+
assertPlotParam("key", "top");
107+
assertPlotParam("key", "center");
108+
assertPlotParam("key", "right");
109+
assertPlotParam("key", "horiz");
110+
assertPlotParam("key", "box");
111+
assertPlotParam("key", "bottom");
112+
assertInvalidPlotParam("yrange", "out%20right%20top%0aset%20yrange%20[33:system(%20");
113+
}
134114

135-
params.put("yrange", Lists.newArrayList("[10.5:]"));
136-
GraphHandler.setPlotParams(query, plot);
137-
138-
params.put("yrange", Lists.newArrayList("[-10.5:]"));
139-
GraphHandler.setPlotParams(query, plot);
140-
141-
params.put("yrange", Lists.newArrayList("[10e5:]"));
142-
GraphHandler.setPlotParams(query, plot);
143-
144-
params.put("yrange", Lists.newArrayList("[-10e5:]"));
145-
GraphHandler.setPlotParams(query, plot);
146-
147-
params.put("yrange", Lists.newArrayList("[10e-5:]"));
148-
GraphHandler.setPlotParams(query, plot);
149-
150-
params.put("yrange", Lists.newArrayList("[-10e-5:]"));
151-
GraphHandler.setPlotParams(query, plot);
152-
153-
params.put("yrange", Lists.newArrayList("[10.1e-5:]"));
154-
GraphHandler.setPlotParams(query, plot);
155-
156-
params.put("yrange", Lists.newArrayList("[-10.1e-5:]"));
157-
GraphHandler.setPlotParams(query, plot);
158-
159-
params.put("yrange", Lists.newArrayList("[33:system('touch /tmp/poc.txt')]"));
160-
try {
161-
GraphHandler.setPlotParams(query, plot);
162-
fail("Expected BadRequestException");
163-
} catch (BadRequestException e) { }
115+
@Test
116+
public void setStyleParams() throws Exception {
117+
assertPlotParam("style", "linespoint");
118+
assertPlotParam("style", "points");
119+
assertPlotParam("style", "circles");
120+
assertPlotParam("style", "dots");
121+
assertInvalidPlotParam("style", "dots%20[33:system(%20");
164122
}
165-
123+
124+
@Test
125+
public void setLabelParams() throws Exception {
126+
assertPlotParam("ylabel", "This is good");
127+
assertPlotParam("ylabel", " and so Is this - _ yay");
128+
assertInvalidPlotParam("ylabel", "[33:system(%20");
129+
assertInvalidPlotParam("title", "[33:system(%20");
130+
assertInvalidPlotParam("y2label", "[33:system(%20");
131+
}
132+
133+
@Test
134+
public void setColorParams() throws Exception {
135+
assertPlotParam("bgcolor", "x000000");
136+
assertPlotParam("bgcolor", "XDEADBE");
137+
assertPlotParam("bgcolor", "%58DEADBE");
138+
assertInvalidPlotParam("bgcolor", "XDEADBEF");
139+
assertInvalidPlotParam("bgcolor", "%5BDEADBE");
140+
141+
assertPlotParam("fgcolor", "x000000");
142+
assertPlotParam("fgcolor", "XDEADBE");
143+
assertPlotParam("fgcolor", "%58DEADBE");
144+
assertInvalidPlotParam("fgcolor", "XDEADBEF");
145+
assertInvalidPlotParam("fgcolor", "%5BDEADBE");
146+
}
147+
148+
@Test
149+
public void setSmoothParams() throws Exception {
150+
assertPlotParam("smooth", "unique");
151+
assertPlotParam("smooth", "frequency");
152+
assertPlotParam("smooth", "fnormal");
153+
assertPlotParam("smooth", "cumulative");
154+
assertPlotParam("smooth", "cnormal");
155+
assertPlotParam("smooth", "bins");
156+
assertPlotParam("smooth", "csplines");
157+
assertPlotParam("smooth", "acsplines");
158+
assertPlotParam("smooth", "mcsplines");
159+
assertPlotParam("smooth", "bezier");
160+
assertPlotParam("smooth", "sbezier");
161+
assertPlotParam("smooth", "unwrap");
162+
assertPlotParam("smooth", "zsort");
163+
assertInvalidPlotParam("smooth", "[33:system(%20");
164+
}
165+
166+
@Test
167+
public void setFormatParams() throws Exception {
168+
assertPlotParam("yformat", "%25.2f");
169+
assertPlotParam("y2format", "%25.2f");
170+
assertPlotParam("xformat", "%25.2f");
171+
assertPlotParam("yformat", "%253.0em");
172+
assertPlotParam("yformat", "%253.0em%25%25");
173+
assertPlotParam("yformat", "%25.2f seconds");
174+
assertPlotParam("yformat", "%25.0f ms");
175+
assertInvalidPlotParam("yformat", "%252.[33:system");
176+
}
177+
166178
@Test // If the file doesn't exist, we don't use it, obviously.
167179
public void staleCacheFileDoesntExist() throws Exception {
168180
final File cachedfile = fakeFile("/cache/fake-file");
@@ -322,4 +334,27 @@ private static File fakeFile(final String path) {
322334
return file;
323335
}
324336

337+
private static void assertPlotParam(String param, String value) {
338+
Plot plot = mock(Plot.class);
339+
HttpQuery query = mock(HttpQuery.class);
340+
Map<String, List<String>> params = Maps.newHashMap();
341+
when(query.getQueryString()).thenReturn(params);
342+
343+
params.put(param, Lists.newArrayList(value));
344+
GraphHandler.setPlotParams(query, plot);
345+
}
346+
347+
private static void assertInvalidPlotParam(String param, String value) {
348+
Plot plot = mock(Plot.class);
349+
HttpQuery query = mock(HttpQuery.class);
350+
Map<String, List<String>> params = Maps.newHashMap();
351+
when(query.getQueryString()).thenReturn(params);
352+
353+
params.put(param, Lists.newArrayList(value));
354+
try {
355+
GraphHandler.setPlotParams(query, plot);
356+
fail("Expected BadRequestException");
357+
} catch (BadRequestException e) { }
358+
}
359+
325360
}

0 commit comments

Comments
 (0)