Skip to content

Commit a6027e5

Browse files
committed
Limit accepted chunk-size to 2^63-1 and be strict about chunk-ext
authorized characters. Submitted by: Yann Ylavic git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1684513 13f79535-47bb-0310-9956-ffa450edef68
1 parent 3f38589 commit a6027e5

File tree

3 files changed

+87
-48
lines changed

3 files changed

+87
-48
lines changed

Diff for: CHANGES

+3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
-*- coding: utf-8 -*-
22
Changes with Apache 2.5.0
33

4+
*) core: Limit accepted chunk-size to 2^63-1 and be strict about chunk-ext
5+
authorized characters. [Yann Ylavic]
6+
47
*) mod_ssl: When SSLVerify is disabled (NONE), don't force a renegotiation if
58
the SSLVerifyDepth applied with the default/handshaken vhost differs from
69
the one applicable with the finally selected vhost. [Yann Ylavic]

Diff for: docs/log-message-tags/next-number

+1-1
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
2901
1+
2902

Diff for: modules/http/http_filters.c

+83-47
Original file line numberDiff line numberDiff line change
@@ -57,24 +57,24 @@
5757

5858
APLOG_USE_MODULE(http);
5959

60-
#define INVALID_CHAR -2
61-
6260
typedef struct http_filter_ctx
6361
{
6462
apr_off_t remaining;
6563
apr_off_t limit;
6664
apr_off_t limit_used;
6765
apr_int32_t chunk_used;
68-
apr_int16_t chunkbits;
66+
apr_int32_t chunkbits;
6967
enum
7068
{
7169
BODY_NONE, /* streamed data */
7270
BODY_LENGTH, /* data constrained by content length */
7371
BODY_CHUNK, /* chunk expected */
7472
BODY_CHUNK_PART, /* chunk digits */
7573
BODY_CHUNK_EXT, /* chunk extension */
74+
BODY_CHUNK_LF, /* got CR, expect LF after digits/extension */
7675
BODY_CHUNK_DATA, /* data constrained by chunked encoding */
77-
BODY_CHUNK_END, /* chunk terminating CRLF */
76+
BODY_CHUNK_END, /* chunked data terminating CRLF */
77+
BODY_CHUNK_END_LF, /* got CR, expect LF after data */
7878
BODY_CHUNK_TRAILER /* trailers */
7979
} state;
8080
unsigned int eos_sent :1;
@@ -89,7 +89,7 @@ typedef struct http_filter_ctx
8989
* In general, any negative number can be considered an overflow error.
9090
*/
9191
static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
92-
apr_size_t len, int linelimit)
92+
apr_size_t len, int linelimit)
9393
{
9494
apr_size_t i = 0;
9595

@@ -99,10 +99,20 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
9999
ap_xlate_proto_from_ascii(&c, 1);
100100

101101
/* handle CRLF after the chunk */
102-
if (ctx->state == BODY_CHUNK_END) {
102+
if (ctx->state == BODY_CHUNK_END
103+
|| ctx->state == BODY_CHUNK_END_LF) {
103104
if (c == LF) {
104105
ctx->state = BODY_CHUNK;
105106
}
107+
else if (c == CR && ctx->state == BODY_CHUNK_END) {
108+
ctx->state = BODY_CHUNK_END_LF;
109+
}
110+
else {
111+
/*
112+
* LF expected.
113+
*/
114+
return APR_EINVAL;
115+
}
106116
i++;
107117
continue;
108118
}
@@ -111,44 +121,62 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
111121
if (ctx->state == BODY_CHUNK) {
112122
if (!apr_isxdigit(c)) {
113123
/*
114-
* Detect invalid character at beginning. This also works for empty
115-
* chunk size lines.
124+
* Detect invalid character at beginning. This also works for
125+
* empty chunk size lines.
116126
*/
117-
return APR_EGENERAL;
127+
return APR_EINVAL;
118128
}
119129
else {
120130
ctx->state = BODY_CHUNK_PART;
121131
}
122132
ctx->remaining = 0;
123-
ctx->chunkbits = sizeof(long) * 8;
133+
ctx->chunkbits = sizeof(apr_off_t) * 8;
124134
ctx->chunk_used = 0;
125135
}
126136

127-
/* handle a chunk part, or a chunk extension */
128-
/*
129-
* In theory, we are supposed to expect CRLF only, but our
130-
* test suite sends LF only. Tolerate a missing CR.
131-
*/
132-
if (c == ';' || c == CR) {
133-
ctx->state = BODY_CHUNK_EXT;
134-
}
135-
else if (c == LF) {
137+
if (c == LF) {
136138
if (ctx->remaining) {
137139
ctx->state = BODY_CHUNK_DATA;
138140
}
139141
else {
140142
ctx->state = BODY_CHUNK_TRAILER;
141143
}
142144
}
143-
else if (ctx->state != BODY_CHUNK_EXT) {
144-
int xvalue = 0;
145+
else if (ctx->state == BODY_CHUNK_LF) {
146+
/*
147+
* LF expected.
148+
*/
149+
return APR_EINVAL;
150+
}
151+
else if (c == CR) {
152+
ctx->state = BODY_CHUNK_LF;
153+
}
154+
else if (c == ';') {
155+
ctx->state = BODY_CHUNK_EXT;
156+
}
157+
else if (ctx->state == BODY_CHUNK_EXT) {
158+
/*
159+
* Control chars (but tabs) are invalid.
160+
*/
161+
if (c != '\t' && apr_iscntrl(c)) {
162+
return APR_EINVAL;
163+
}
164+
}
165+
else if (ctx->state == BODY_CHUNK_PART) {
166+
int xvalue;
145167

146168
/* ignore leading zeros */
147169
if (!ctx->remaining && c == '0') {
148170
i++;
149171
continue;
150172
}
151173

174+
ctx->chunkbits -= 4;
175+
if (ctx->chunkbits < 0) {
176+
/* overflow */
177+
return APR_ENOSPC;
178+
}
179+
152180
if (c >= '0' && c <= '9') {
153181
xvalue = c - '0';
154182
}
@@ -160,16 +188,18 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
160188
}
161189
else {
162190
/* bogus character */
163-
return APR_EGENERAL;
191+
return APR_EINVAL;
164192
}
165193

166194
ctx->remaining = (ctx->remaining << 4) | xvalue;
167-
ctx->chunkbits -= 4;
168-
if (ctx->chunkbits <= 0 || ctx->remaining < 0) {
195+
if (ctx->remaining < 0) {
169196
/* overflow */
170197
return APR_ENOSPC;
171198
}
172-
199+
}
200+
else {
201+
/* Should not happen */
202+
return APR_EGENERAL;
173203
}
174204

175205
i++;
@@ -232,7 +262,8 @@ static apr_status_t read_chunked_trailers(http_ctx_t *ctx, ap_filter_t *f,
232262
* are successfully parsed.
233263
*/
234264
apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
235-
ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes)
265+
ap_input_mode_t mode, apr_read_type_e block,
266+
apr_off_t readbytes)
236267
{
237268
core_server_config *conf;
238269
apr_bucket *e;
@@ -282,8 +313,8 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
282313
* reading the connection until it is closed by the server."
283314
*/
284315
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(02555)
285-
"Unknown Transfer-Encoding: %s;"
286-
" using read-until-close", tenc);
316+
"Unknown Transfer-Encoding: %s; "
317+
"using read-until-close", tenc);
287318
tenc = NULL;
288319
}
289320
else {
@@ -308,22 +339,20 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
308339
|| endstr == lenp || *endstr || ctx->remaining < 0) {
309340

310341
ctx->remaining = 0;
311-
ap_log_rerror(
312-
APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01587)
313-
"Invalid Content-Length");
342+
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01587)
343+
"Invalid Content-Length");
314344

315-
return APR_ENOSPC;
345+
return APR_EINVAL;
316346
}
317347

318348
/* If we have a limit in effect and we know the C-L ahead of
319349
* time, stop it here if it is invalid.
320350
*/
321351
if (ctx->limit && ctx->limit < ctx->remaining) {
322-
ap_log_rerror(
323-
APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01588)
324-
"Requested content-length of %" APR_OFF_T_FMT
325-
" is larger than the configured limit"
326-
" of %" APR_OFF_T_FMT, ctx->remaining, ctx->limit);
352+
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01588)
353+
"Requested content-length of %" APR_OFF_T_FMT
354+
" is larger than the configured limit"
355+
" of %" APR_OFF_T_FMT, ctx->remaining, ctx->limit);
327356
return APR_ENOSPC;
328357
}
329358
}
@@ -378,6 +407,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
378407
APR_BRIGADE_INSERT_TAIL(bb, e);
379408

380409
rv = ap_pass_brigade(f->c->output_filters, bb);
410+
apr_brigade_cleanup(bb);
381411
if (rv != APR_SUCCESS) {
382412
return AP_FILTER_ERROR;
383413
}
@@ -401,7 +431,9 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
401431
case BODY_CHUNK:
402432
case BODY_CHUNK_PART:
403433
case BODY_CHUNK_EXT:
404-
case BODY_CHUNK_END: {
434+
case BODY_CHUNK_LF:
435+
case BODY_CHUNK_END:
436+
case BODY_CHUNK_END_LF: {
405437

406438
rv = ap_get_brigade(f->next, b, AP_MODE_GETLINE, block, 0);
407439

@@ -433,8 +465,9 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
433465
f->r->server->limit_req_fieldsize);
434466
}
435467
if (rv != APR_SUCCESS) {
436-
ap_log_rerror(
437-
APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590) "Error reading chunk %s ", (APR_ENOSPC == rv) ? "(overflow)" : "");
468+
ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590)
469+
"Error reading chunk %s ",
470+
(APR_ENOSPC == rv) ? "(overflow)" : "");
438471
return rv;
439472
}
440473
}
@@ -446,9 +479,8 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
446479

447480
if (ctx->state == BODY_CHUNK_TRAILER) {
448481
/* Treat UNSET as DISABLE - trailers aren't merged by default */
449-
int merge_trailers =
450-
conf->merge_trailers == AP_MERGE_TRAILERS_ENABLE;
451-
return read_chunked_trailers(ctx, f, b, merge_trailers);
482+
return read_chunked_trailers(ctx, f, b,
483+
conf->merge_trailers == AP_MERGE_TRAILERS_ENABLE);
452484
}
453485

454486
break;
@@ -522,9 +554,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
522554
* really count. This seems to be up for interpretation. */
523555
ctx->limit_used += totalread;
524556
if (ctx->limit < ctx->limit_used) {
525-
ap_log_rerror(
526-
APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01591) "Read content-length of %" APR_OFF_T_FMT " is larger than the configured limit"
527-
" of %" APR_OFF_T_FMT, ctx->limit_used, ctx->limit);
557+
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01591)
558+
"Read content-length of %" APR_OFF_T_FMT
559+
" is larger than the configured limit"
560+
" of %" APR_OFF_T_FMT, ctx->limit_used, ctx->limit);
528561
return APR_ENOSPC;
529562
}
530563
}
@@ -549,7 +582,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
549582
break;
550583
}
551584
default: {
552-
break;
585+
/* Should not happen */
586+
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, f->r, APLOGNO(02901)
587+
"Unexpected body state (%i)", (int)ctx->state);
588+
return APR_EGENERAL;
553589
}
554590
}
555591

0 commit comments

Comments
 (0)