Skip to content

Commit 702bf15

Browse files
markt-asfmarkt-asf
committed
Stricter header value parsing
1 parent b191a0d commit 702bf15

16 files changed

+345
-72
lines changed

‎java/org/apache/coyote/http11/AbstractHttp11Protocol.java

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -83,27 +83,56 @@ public void setAllowHostHeaderMismatch(boolean allowHostHeaderMismatch) {
8383
}
8484

8585

86-
private boolean rejectIllegalHeaderName = false;
86+
private boolean rejectIllegalHeader = false;
8787
/**
88-
* If an HTTP request is received that contains an illegal header name (i.e.
89-
* the header name is not a token) will the request be rejected (with a 400
90-
* response) or will the illegal header be ignored.
88+
* If an HTTP request is received that contains an illegal header name or
89+
* value (e.g. the header name is not a token) will the request be rejected
90+
* (with a 400 response) or will the illegal header be ignored?
9191
*
9292
* @return {@code true} if the request will be rejected or {@code false} if
9393
* the header will be ignored
9494
*/
95-
public boolean getRejectIllegalHeaderName() { return rejectIllegalHeaderName; }
95+
public boolean getRejectIllegalHeader() { return rejectIllegalHeader; }
9696
/**
97-
* If an HTTP request is received that contains an illegal header name (i.e.
98-
* the header name is not a token) should the request be rejected (with a
99-
* 400 response) or should the illegal header be ignored.
97+
* If an HTTP request is received that contains an illegal header name or
98+
* value (e.g. the header name is not a token) should the request be
99+
* rejected (with a 400 response) or should the illegal header be ignored?
100+
*
101+
* @param rejectIllegalHeader {@code true} to reject requests with illegal
102+
* header names or values, {@code false} to
103+
* ignore the header
104+
*/
105+
public void setRejectIllegalHeader(boolean rejectIllegalHeader) {
106+
this.rejectIllegalHeader = rejectIllegalHeader;
107+
}
108+
/**
109+
* If an HTTP request is received that contains an illegal header name or
110+
* value (e.g. the header name is not a token) will the request be rejected
111+
* (with a 400 response) or will the illegal header be ignored?
112+
*
113+
* @return {@code true} if the request will be rejected or {@code false} if
114+
* the header will be ignored
115+
*
116+
* @deprecated Now an alias for {@link #getRejectIllegalHeader()}. Will be
117+
* removed in Tomcat 10 onwards.
118+
*/
119+
@Deprecated
120+
public boolean getRejectIllegalHeaderName() { return rejectIllegalHeader; }
121+
/**
122+
* If an HTTP request is received that contains an illegal header name or
123+
* value (e.g. the header name is not a token) should the request be
124+
* rejected (with a 400 response) or should the illegal header be ignored?
100125
*
101126
* @param rejectIllegalHeaderName {@code true} to reject requests with
102-
* illegal header names, {@code false} to
103-
* ignore the header
127+
* illegal header names or values,
128+
* {@code false} to ignore the header
129+
*
130+
* @deprecated Now an alias for {@link #setRejectIllegalHeader(boolean)}.
131+
* Will be removed in Tomcat 10 onwards.
104132
*/
133+
@Deprecated
105134
public void setRejectIllegalHeaderName(boolean rejectIllegalHeaderName) {
106-
this.rejectIllegalHeaderName = rejectIllegalHeaderName;
135+
this.rejectIllegalHeader = rejectIllegalHeaderName;
107136
}
108137

109138

‎java/org/apache/coyote/http11/AbstractInputBuffer.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,11 @@ public abstract class AbstractInputBuffer<S> implements InputBuffer{
108108
protected int lastActiveFilter;
109109

110110

111+
/**
112+
* Do HTTP headers with illegal names and/or values cause the request to be
113+
* rejected? Note that the field name is not quite right but cannot be
114+
* easily changed without breaking binary compatibility.
115+
*/
111116
protected boolean rejectIllegalHeaderName;
112117

113118

‎java/org/apache/coyote/http11/Http11AprProcessor.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ protected Log getLog() {
6161
// ----------------------------------------------------------- Constructors
6262

6363

64-
public Http11AprProcessor(int headerBufferSize, boolean rejectIllegalHeaderName,
64+
public Http11AprProcessor(int headerBufferSize, boolean rejectIllegalHeader,
6565
AprEndpoint endpoint, int maxTrailerSize, Set<String> allowedTrailerHeaders,
6666
int maxExtensionSize, int maxSwallowSize, String relaxedPathChars,
6767
String relaxedQueryChars) {
@@ -70,7 +70,7 @@ public Http11AprProcessor(int headerBufferSize, boolean rejectIllegalHeaderName,
7070

7171
httpParser = new HttpParser(relaxedPathChars, relaxedQueryChars);
7272

73-
inputBuffer = new InternalAprInputBuffer(request, headerBufferSize, rejectIllegalHeaderName,
73+
inputBuffer = new InternalAprInputBuffer(request, headerBufferSize, rejectIllegalHeader,
7474
httpParser);
7575
request.setInputBuffer(inputBuffer);
7676

‎java/org/apache/coyote/http11/Http11AprProtocol.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ protected void longPoll(SocketWrapper<Long> socket,
299299
@Override
300300
protected Http11AprProcessor createProcessor() {
301301
Http11AprProcessor processor = new Http11AprProcessor(
302-
proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeaderName(),
302+
proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeader(),
303303
(AprEndpoint)proto.endpoint, proto.getMaxTrailerSize(),
304304
proto.getAllowedTrailerHeadersAsSet(), proto.getMaxExtensionSize(),
305305
proto.getMaxSwallowSize(), proto.getRelaxedPathChars(),

‎java/org/apache/coyote/http11/Http11NioProcessor.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ protected Log getLog() {
6666
// ----------------------------------------------------------- Constructors
6767

6868

69-
public Http11NioProcessor(int maxHttpHeaderSize, boolean rejectIllegalHeaderName,
69+
public Http11NioProcessor(int maxHttpHeaderSize, boolean rejectIllegalHeader,
7070
NioEndpoint endpoint, int maxTrailerSize, Set<String> allowedTrailerHeaders,
7171
int maxExtensionSize, int maxSwallowSize, String relaxedPathChars,
7272
String relaxedQueryChars) {
@@ -76,7 +76,7 @@ public Http11NioProcessor(int maxHttpHeaderSize, boolean rejectIllegalHeaderName
7676
httpParser = new HttpParser(relaxedPathChars, relaxedQueryChars);
7777

7878
inputBuffer = new InternalNioInputBuffer(request, maxHttpHeaderSize,
79-
rejectIllegalHeaderName, httpParser);
79+
rejectIllegalHeader, httpParser);
8080
request.setInputBuffer(inputBuffer);
8181

8282
outputBuffer = new InternalNioOutputBuffer(response, maxHttpHeaderSize);

‎java/org/apache/coyote/http11/Http11NioProtocol.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ protected void longPoll(SocketWrapper<NioChannel> socket,
264264
@Override
265265
public Http11NioProcessor createProcessor() {
266266
Http11NioProcessor processor = new Http11NioProcessor(
267-
proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeaderName(),
267+
proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeader(),
268268
(NioEndpoint)proto.endpoint, proto.getMaxTrailerSize(),
269269
proto.getAllowedTrailerHeadersAsSet(), proto.getMaxExtensionSize(),
270270
proto.getMaxSwallowSize(), proto.getRelaxedPathChars(),

‎java/org/apache/coyote/http11/Http11Processor.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ protected Log getLog() {
5151
// ------------------------------------------------------------ Constructor
5252

5353

54-
public Http11Processor(int headerBufferSize, boolean rejectIllegalHeaderName,
54+
public Http11Processor(int headerBufferSize, boolean rejectIllegalHeader,
5555
JIoEndpoint endpoint, int maxTrailerSize, Set<String> allowedTrailerHeaders,
5656
int maxExtensionSize, int maxSwallowSize, String relaxedPathChars,
5757
String relaxedQueryChars) {
@@ -60,7 +60,7 @@ public Http11Processor(int headerBufferSize, boolean rejectIllegalHeaderName,
6060

6161
httpParser = new HttpParser(relaxedPathChars, relaxedQueryChars);
6262

63-
inputBuffer = new InternalInputBuffer(request, headerBufferSize, rejectIllegalHeaderName,
63+
inputBuffer = new InternalInputBuffer(request, headerBufferSize, rejectIllegalHeader,
6464
httpParser);
6565
request.setInputBuffer(inputBuffer);
6666

‎java/org/apache/coyote/http11/Http11Protocol.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ protected void longPoll(SocketWrapper<Socket> socket,
163163
@Override
164164
protected Http11Processor createProcessor() {
165165
Http11Processor processor = new Http11Processor(
166-
proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeaderName(),
166+
proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeader(),
167167
(JIoEndpoint)proto.endpoint, proto.getMaxTrailerSize(),
168168
proto.getAllowedTrailerHeadersAsSet(), proto.getMaxExtensionSize(),
169169
proto.getMaxSwallowSize(), proto.getRelaxedPathChars(),

‎java/org/apache/coyote/http11/InternalAprInputBuffer.java

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> {
5252
* Alternate constructor.
5353
*/
5454
public InternalAprInputBuffer(Request request, int headerBufferSize,
55-
boolean rejectIllegalHeaderName, HttpParser httpParser) {
55+
boolean rejectIllegalHeader, HttpParser httpParser) {
5656

5757
this.request = request;
5858
headers = request.getMimeHeaders();
@@ -64,7 +64,7 @@ public InternalAprInputBuffer(Request request, int headerBufferSize,
6464
bbuf = ByteBuffer.allocateDirect((headerBufferSize / 1500 + 1) * 1500);
6565
}
6666

67-
this.rejectIllegalHeaderName = rejectIllegalHeaderName;
67+
this.rejectIllegalHeaderName = rejectIllegalHeader;
6868
this.httpParser = httpParser;
6969

7070
inputStreamInputBuffer = new SocketInputBuffer();
@@ -351,6 +351,8 @@ private boolean parseHeader()
351351
//
352352

353353
byte chr = 0;
354+
byte prevChr = 0;
355+
354356
while (true) {
355357

356358
// Read new bytes if needed
@@ -359,19 +361,23 @@ private boolean parseHeader()
359361
throw new EOFException(sm.getString("iib.eof.error"));
360362
}
361363

364+
prevChr = chr;
362365
chr = buf[pos];
363366

364-
if (chr == Constants.CR) {
365-
// Skip
366-
} else if (chr == Constants.LF) {
367-
pos++;
367+
if (chr == Constants.CR && prevChr != Constants.CR) {
368+
// Possible start of CRLF - process the next byte.
369+
} else if (prevChr == Constants.CR && chr == Constants.LF) {
370+
pos++;
368371
return false;
369372
} else {
373+
if (prevChr == Constants.CR) {
374+
// Must have read two bytes (first was CR, second was not LF)
375+
pos--;
376+
}
370377
break;
371378
}
372379

373380
pos++;
374-
375381
}
376382

377383
// Mark the current buffer position
@@ -456,10 +462,24 @@ private boolean parseHeader()
456462
throw new EOFException(sm.getString("iib.eof.error"));
457463
}
458464

459-
if (buf[pos] == Constants.CR) {
460-
// Skip
461-
} else if (buf[pos] == Constants.LF) {
465+
prevChr = chr;
466+
chr = buf[pos];
467+
if (chr == Constants.CR) {
468+
// Possible start of CRLF - process the next byte.
469+
} else if (prevChr == Constants.CR && chr == Constants.LF) {
462470
eol = true;
471+
} else if (prevChr == Constants.CR) {
472+
// Invalid value
473+
// Delete the header (it will be the most recent one)
474+
headers.removeHeader(headers.size() - 1);
475+
skipLine(lineStart, start);
476+
return true;
477+
} else if (chr != Constants.HT && HttpParser.isControl(chr)) {
478+
// Invalid value
479+
// Delete the header (it will be the most recent one)
480+
headers.removeHeader(headers.size() - 1);
481+
skipLine(lineStart, start);
482+
return true;
463483
} else if (buf[pos] == Constants.SP) {
464484
buf[realPos] = buf[pos];
465485
realPos++;
@@ -512,6 +532,9 @@ private void skipLine(int lineStart, int start) throws IOException {
512532
lastRealByte = pos - 1;
513533
}
514534

535+
byte chr = 0;
536+
byte prevChr = 0;
537+
515538
while (!eol) {
516539

517540
// Read new bytes if needed
@@ -520,9 +543,12 @@ private void skipLine(int lineStart, int start) throws IOException {
520543
throw new EOFException(sm.getString("iib.eof.error"));
521544
}
522545

523-
if (buf[pos] == Constants.CR) {
546+
prevChr = chr;
547+
chr = buf[pos];
548+
549+
if (chr == Constants.CR) {
524550
// Skip
525-
} else if (buf[pos] == Constants.LF) {
551+
} else if (prevChr == Constants.CR && chr == Constants.LF) {
526552
eol = true;
527553
} else {
528554
lastRealByte = pos;

‎java/org/apache/coyote/http11/InternalInputBuffer.java

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,14 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> {
5353
* Default constructor.
5454
*/
5555
public InternalInputBuffer(Request request, int headerBufferSize,
56-
boolean rejectIllegalHeaderName, HttpParser httpParser) {
56+
boolean rejectIllegalHeader, HttpParser httpParser) {
5757

5858
this.request = request;
5959
headers = request.getMimeHeaders();
6060

6161
buf = new byte[headerBufferSize];
6262

63-
this.rejectIllegalHeaderName = rejectIllegalHeaderName;
63+
this.rejectIllegalHeaderName = rejectIllegalHeader;
6464
this.httpParser = httpParser;
6565

6666
inputStreamInputBuffer = new InputStreamInputBuffer();
@@ -303,6 +303,8 @@ private boolean parseHeader()
303303
//
304304

305305
byte chr = 0;
306+
byte prevChr = 0;
307+
306308
while (true) {
307309

308310
// Read new bytes if needed
@@ -311,19 +313,23 @@ private boolean parseHeader()
311313
throw new EOFException(sm.getString("iib.eof.error"));
312314
}
313315

316+
prevChr = chr;
314317
chr = buf[pos];
315318

316-
if (chr == Constants.CR) {
317-
// Skip
318-
} else if (chr == Constants.LF) {
319+
if (chr == Constants.CR && prevChr != Constants.CR) {
320+
// Possible start of CRLF - process the next byte.
321+
} else if (prevChr == Constants.CR && chr == Constants.LF) {
319322
pos++;
320323
return false;
321324
} else {
325+
if (prevChr == Constants.CR) {
326+
// Must have read two bytes (first was CR, second was not LF)
327+
pos--;
328+
}
322329
break;
323330
}
324331

325332
pos++;
326-
327333
}
328334

329335
// Mark the current buffer position
@@ -409,15 +415,29 @@ private boolean parseHeader()
409415
throw new EOFException(sm.getString("iib.eof.error"));
410416
}
411417

412-
if (buf[pos] == Constants.CR) {
413-
// Skip
414-
} else if (buf[pos] == Constants.LF) {
418+
prevChr = chr;
419+
chr = buf[pos];
420+
if (chr == Constants.CR) {
421+
// Possible start of CRLF - process the next byte.
422+
} else if (prevChr == Constants.CR && chr == Constants.LF) {
415423
eol = true;
416-
} else if (buf[pos] == Constants.SP) {
417-
buf[realPos] = buf[pos];
424+
} else if (prevChr == Constants.CR) {
425+
// Invalid value
426+
// Delete the header (it will be the most recent one)
427+
headers.removeHeader(headers.size() - 1);
428+
skipLine(lineStart, start);
429+
return true;
430+
} else if (chr != Constants.HT && HttpParser.isControl(chr)) {
431+
// Invalid value
432+
// Delete the header (it will be the most recent one)
433+
headers.removeHeader(headers.size() - 1);
434+
skipLine(lineStart, start);
435+
return true;
436+
} else if (chr == Constants.SP) {
437+
buf[realPos] = chr;
418438
realPos++;
419439
} else {
420-
buf[realPos] = buf[pos];
440+
buf[realPos] = chr;
421441
realPos++;
422442
lastSignificantChar = realPos;
423443
}
@@ -483,6 +503,9 @@ private void skipLine(int lineStart, int start) throws IOException {
483503
lastRealByte = pos - 1;
484504
}
485505

506+
byte chr = 0;
507+
byte prevChr = 0;
508+
486509
while (!eol) {
487510

488511
// Read new bytes if needed
@@ -491,9 +514,12 @@ private void skipLine(int lineStart, int start) throws IOException {
491514
throw new EOFException(sm.getString("iib.eof.error"));
492515
}
493516

494-
if (buf[pos] == Constants.CR) {
517+
prevChr = chr;
518+
chr = buf[pos];
519+
520+
if (chr == Constants.CR) {
495521
// Skip
496-
} else if (buf[pos] == Constants.LF) {
522+
} else if (prevChr == Constants.CR && chr == Constants.LF) {
497523
eol = true;
498524
} else {
499525
lastRealByte = pos;

0 commit comments

Comments
 (0)