Skip to content

Commit 78667b8

Browse files
authored
Refactor header warning pattern to avoid stackoverflow (elastic#96409)
an alternative combined with * in a regex is causing a stackoverflow when parsing very long inputs (stack size have to be linearly correlated to the warning length). This commit refactors the warning value aka qdText as per rfc7230 into iterative approach closes elastic#95972
1 parent 9f731b0 commit 78667b8

File tree

3 files changed

+93
-9
lines changed

3 files changed

+93
-9
lines changed

server/src/main/java/org/elasticsearch/common/logging/HeaderWarning.java

Lines changed: 51 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
import java.util.concurrent.CopyOnWriteArraySet;
2424
import java.util.regex.Matcher;
2525
import java.util.regex.Pattern;
26+
import java.util.stream.IntStream;
27+
import java.util.stream.Stream;
2628

2729
/**
2830
* This is a simplistic logger that adds warning messages to HTTP headers.
@@ -39,7 +41,8 @@ public class HeaderWarning {
3941
"Elasticsearch-" + // warn agent
4042
"\\d+\\.\\d+\\.\\d+(?:-(?:alpha|beta|rc)\\d+)?(?:-SNAPSHOT)?-" + // warn agent
4143
"(?:[a-f0-9]{7}(?:[a-f0-9]{33})?|unknown) " + // warn agent
42-
"\"((?:\t| |!|[\\x23-\\x5B]|[\\x5D-\\x7E]|[\\x80-\\xFF]|\\\\|\\\\\")*)\"( " + // quoted warning value, captured
44+
// quoted warning value, captured. Do not add more greedy qualifiers later to avoid excessive backtracking
45+
"\"(?<quotedStringValue>.*)\"( " +
4346
// quoted RFC 1123 date format
4447
"\"" + // opening quote
4548
"(?:Mon|Tue|Wed|Thu|Fri|Sat|Sun), " + // weekday
@@ -48,7 +51,31 @@ public class HeaderWarning {
4851
"\\d{4} " + // 4-digit year
4952
"\\d{2}:\\d{2}:\\d{2} " + // (two-digit hour):(two-digit minute):(two-digit second)
5053
"GMT" + // GMT
51-
"\")?"); // closing quote (optional, since an older version can still send a warn-date)
54+
"\")?",// closing quote (optional, since an older version can still send a warn-date)
55+
Pattern.DOTALL
56+
); // in order to parse new line inside the qdText
57+
58+
/**
59+
* quoted-string is defined in https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6
60+
* quoted-string = DQUOTE *( qdtext / quoted-pair ) DQUOTE
61+
* qdtext = HTAB / SP /%x21 / %x23-5B / %x5D-7E / obs-text
62+
* obs-text = %x80-FF
63+
* <p>
64+
* this was previously used in WARNING_HEADER_PATTERN, but can cause stackoverflow
65+
* "\"((?:\t| |!|[\\x23-\\x5B]|[\\x5D-\\x7E]|[\\x80-\\xFF]|\\\\|\\\\\")*)\"( " + // quoted warning value, captured
66+
* the individual chars from qdText can be validated using the set of chars
67+
* the \\\\|\\\\\" (escaped '\' 0x20 and '"' 0x5c) which is used for quoted-pair has to be validated as strings
68+
*/
69+
private static BitSet qdTextChars = Stream.of(
70+
IntStream.of(0x09),// HTAB
71+
IntStream.of(0x20), // SPACE
72+
IntStream.of(0x21), // !
73+
// excluding 0x22 '"\"' which has to be escaped
74+
IntStream.rangeClosed(0x23, 0x5B),// ascii #-[
75+
// excluding 0x5c '\' which has to be escaped
76+
IntStream.rangeClosed(0x5D, 0x7E),// ascii ]-~
77+
IntStream.rangeClosed(0x80, 0xFF)// obs-text -bear in mind it contains 0x85 new line. Which requires DOT_ALL flag
78+
).flatMapToInt(i -> i).collect(BitSet::new, BitSet::set, BitSet::or);
5279
public static final Pattern WARNING_XCONTENT_LOCATION_PATTERN = Pattern.compile("^\\[.*?]\\[-?\\d+:-?\\d+] ");
5380

5481
/*
@@ -182,7 +209,26 @@ private static boolean assertWarningValue(final String s, final String warningVa
182209
final Matcher matcher = WARNING_HEADER_PATTERN.matcher(s);
183210
final boolean matches = matcher.matches();
184211
assert matches;
185-
return matcher.group(1).equals(warningValue);
212+
String quotedStringValue = matcher.group("quotedStringValue");
213+
assert matchesQuotedString(quotedStringValue);
214+
return quotedStringValue.equals(warningValue);
215+
}
216+
217+
// this is meant to be in testing only
218+
public static boolean warningHeaderPatternMatches(final String s) {
219+
final Matcher matcher = WARNING_HEADER_PATTERN.matcher(s);
220+
final boolean matches = matcher.matches();
221+
if (matches) {
222+
String quotedStringValue = matcher.group("quotedStringValue");
223+
return matchesQuotedString(quotedStringValue);
224+
}
225+
return false;
226+
}
227+
228+
private static boolean matchesQuotedString(String qdtext) {
229+
qdtext = qdtext.replaceAll("\\\\\"", "");
230+
qdtext = qdtext.replaceAll("\\\\", "");
231+
return qdtext.chars().allMatch(c -> qdTextChars.get(c));
186232
}
187233

188234
/**
@@ -330,9 +376,8 @@ static void addWarning(Set<ThreadContext> threadContexts, String message, Object
330376
if (iterator.hasNext()) {
331377
final String formattedMessage = LoggerMessageFormat.format(message, params);
332378
final String warningHeaderValue = formatWarning(formattedMessage);
333-
// TODO #95972: Temporarily commented out to avoid StackOverflowError, see https://github.com/elastic/elasticsearch/issues/95972
334-
// assert WARNING_HEADER_PATTERN.matcher(warningHeaderValue).matches();
335-
// assert extractWarningValueFromWarningHeader(warningHeaderValue, false).equals(escapeAndEncode(formattedMessage));
379+
assert warningHeaderPatternMatches(warningHeaderValue);
380+
assert extractWarningValueFromWarningHeader(warningHeaderValue, false).equals(escapeAndEncode(formattedMessage));
336381
while (iterator.hasNext()) {
337382
try {
338383
final ThreadContext next = iterator.next();

server/src/test/java/org/elasticsearch/common/logging/HeaderWarningTests.java

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,4 +335,45 @@ public void testAddComplexWarning() {
335335
assertThat(responses.get(0), containsString("\"legacy template [global] has index patterns"));
336336
assertThat(responses.get(0), containsString(Integer.toString(299)));
337337
}
338+
339+
public void testHeaderWarningValidation() {
340+
ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
341+
final Set<ThreadContext> threadContexts = Collections.singleton(threadContext);
342+
343+
HeaderWarning.addWarning(threadContexts, allAllowedChars());
344+
345+
// LoggerMessageFormat.format makes sure all not allowed chars are escaped
346+
HeaderWarning.addWarning(threadContexts, "\"");
347+
HeaderWarning.addWarning(threadContexts, "\\");
348+
HeaderWarning.addWarning(threadContexts, allNotAllowedChars());
349+
}
350+
351+
private String allNotAllowedChars() {
352+
StringBuilder chars = new StringBuilder();
353+
for (char c = 0; c < 256; c++) {
354+
if (c < '\t' || ('\t' < c && c < 0x20) || c == 0x7f) {
355+
chars.append(c);
356+
}
357+
}
358+
return chars.toString();
359+
}
360+
361+
private static String allAllowedChars() {
362+
StringBuilder allPossibleChars = new StringBuilder();
363+
allPossibleChars.append('\t');
364+
allPossibleChars.append(' ');
365+
allPossibleChars.append('!');
366+
for (int i = 0x23; i <= 0x5b; i++) {
367+
allPossibleChars.append((char) i);
368+
}
369+
for (int i = 0x5d; i <= 0x7e; i++) {
370+
allPossibleChars.append((char) i);
371+
}
372+
for (int i = 0x80; i <= 0xff; i++) {
373+
allPossibleChars.append((char) i);
374+
}
375+
allPossibleChars.append("\\");
376+
allPossibleChars.append("\\\"");
377+
return allPossibleChars.toString();
378+
}
338379
}

test/yaml-rest-runner/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838
import java.util.Objects;
3939
import java.util.Set;
4040
import java.util.TreeMap;
41-
import java.util.regex.Matcher;
4241
import java.util.regex.Pattern;
4342

4443
import static java.util.Collections.emptyList;
@@ -438,8 +437,7 @@ void checkWarningHeaders(final List<String> warningHeaders, String testPath) {
438437
.collect(toCollection(LinkedHashSet::new));
439438
final Set<Pattern> expectedRegex = new LinkedHashSet<>(expectedWarningHeadersRegex);
440439
for (final String header : warningHeaders) {
441-
final Matcher matcher = HeaderWarning.WARNING_HEADER_PATTERN.matcher(header);
442-
final boolean matches = matcher.matches();
440+
final boolean matches = HeaderWarning.warningHeaderPatternMatches(header);
443441
if (matches) {
444442
final String message = HeaderWarning.extractWarningValueFromWarningHeader(header, true);
445443
if (allowed.contains(message)) {

0 commit comments

Comments
 (0)