From: William A. Rowe Jr Date: Tue, 9 Jun 2015 20:12:31 +0000 (+0000) Subject: Limit accepted chunk-size to 2^63-1 and be strict about chunk-ext X-Git-Tag: 2.5.0-alpha~3093 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a6027e56924bb6227c1fdbf6f91e7e2438338be6;p=apache 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 --- diff --git a/CHANGES b/CHANGES index 63e7d0fc11..5361e50fbe 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,9 @@ -*- coding: utf-8 -*- Changes with Apache 2.5.0 + *) core: Limit accepted chunk-size to 2^63-1 and be strict about chunk-ext + authorized characters. [Yann Ylavic] + *) mod_ssl: When SSLVerify is disabled (NONE), don't force a renegotiation if the SSLVerifyDepth applied with the default/handshaken vhost differs from the one applicable with the finally selected vhost. [Yann Ylavic] diff --git a/docs/log-message-tags/next-number b/docs/log-message-tags/next-number index c914c6a5d9..6ba94a4313 100644 --- a/docs/log-message-tags/next-number +++ b/docs/log-message-tags/next-number @@ -1 +1 @@ -2901 +2902 diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c index 7ebb619f55..3ad3dced4c 100644 --- a/modules/http/http_filters.c +++ b/modules/http/http_filters.c @@ -57,15 +57,13 @@ APLOG_USE_MODULE(http); -#define INVALID_CHAR -2 - typedef struct http_filter_ctx { apr_off_t remaining; apr_off_t limit; apr_off_t limit_used; apr_int32_t chunk_used; - apr_int16_t chunkbits; + apr_int32_t chunkbits; enum { BODY_NONE, /* streamed data */ @@ -73,8 +71,10 @@ typedef struct http_filter_ctx BODY_CHUNK, /* chunk expected */ BODY_CHUNK_PART, /* chunk digits */ BODY_CHUNK_EXT, /* chunk extension */ + BODY_CHUNK_LF, /* got CR, expect LF after digits/extension */ BODY_CHUNK_DATA, /* data constrained by chunked encoding */ - BODY_CHUNK_END, /* chunk terminating CRLF */ + BODY_CHUNK_END, /* chunked data terminating CRLF */ + BODY_CHUNK_END_LF, /* got CR, expect LF after data */ BODY_CHUNK_TRAILER /* trailers */ } state; unsigned int eos_sent :1; @@ -89,7 +89,7 @@ typedef struct http_filter_ctx * In general, any negative number can be considered an overflow error. */ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer, - apr_size_t len, int linelimit) + apr_size_t len, int linelimit) { apr_size_t i = 0; @@ -99,10 +99,20 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer, ap_xlate_proto_from_ascii(&c, 1); /* handle CRLF after the chunk */ - if (ctx->state == BODY_CHUNK_END) { + if (ctx->state == BODY_CHUNK_END + || ctx->state == BODY_CHUNK_END_LF) { if (c == LF) { ctx->state = BODY_CHUNK; } + else if (c == CR && ctx->state == BODY_CHUNK_END) { + ctx->state = BODY_CHUNK_END_LF; + } + else { + /* + * LF expected. + */ + return APR_EINVAL; + } i++; continue; } @@ -111,28 +121,20 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer, if (ctx->state == BODY_CHUNK) { if (!apr_isxdigit(c)) { /* - * Detect invalid character at beginning. This also works for empty - * chunk size lines. + * Detect invalid character at beginning. This also works for + * empty chunk size lines. */ - return APR_EGENERAL; + return APR_EINVAL; } else { ctx->state = BODY_CHUNK_PART; } ctx->remaining = 0; - ctx->chunkbits = sizeof(long) * 8; + ctx->chunkbits = sizeof(apr_off_t) * 8; ctx->chunk_used = 0; } - /* handle a chunk part, or a chunk extension */ - /* - * In theory, we are supposed to expect CRLF only, but our - * test suite sends LF only. Tolerate a missing CR. - */ - if (c == ';' || c == CR) { - ctx->state = BODY_CHUNK_EXT; - } - else if (c == LF) { + if (c == LF) { if (ctx->remaining) { ctx->state = BODY_CHUNK_DATA; } @@ -140,8 +142,28 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer, ctx->state = BODY_CHUNK_TRAILER; } } - else if (ctx->state != BODY_CHUNK_EXT) { - int xvalue = 0; + else if (ctx->state == BODY_CHUNK_LF) { + /* + * LF expected. + */ + return APR_EINVAL; + } + else if (c == CR) { + ctx->state = BODY_CHUNK_LF; + } + else if (c == ';') { + ctx->state = BODY_CHUNK_EXT; + } + else if (ctx->state == BODY_CHUNK_EXT) { + /* + * Control chars (but tabs) are invalid. + */ + if (c != '\t' && apr_iscntrl(c)) { + return APR_EINVAL; + } + } + else if (ctx->state == BODY_CHUNK_PART) { + int xvalue; /* ignore leading zeros */ if (!ctx->remaining && c == '0') { @@ -149,6 +171,12 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer, continue; } + ctx->chunkbits -= 4; + if (ctx->chunkbits < 0) { + /* overflow */ + return APR_ENOSPC; + } + if (c >= '0' && c <= '9') { xvalue = c - '0'; } @@ -160,16 +188,18 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer, } else { /* bogus character */ - return APR_EGENERAL; + return APR_EINVAL; } ctx->remaining = (ctx->remaining << 4) | xvalue; - ctx->chunkbits -= 4; - if (ctx->chunkbits <= 0 || ctx->remaining < 0) { + if (ctx->remaining < 0) { /* overflow */ return APR_ENOSPC; } - + } + else { + /* Should not happen */ + return APR_EGENERAL; } i++; @@ -232,7 +262,8 @@ static apr_status_t read_chunked_trailers(http_ctx_t *ctx, ap_filter_t *f, * are successfully parsed. */ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, - ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes) + ap_input_mode_t mode, apr_read_type_e block, + apr_off_t readbytes) { core_server_config *conf; apr_bucket *e; @@ -282,8 +313,8 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, * reading the connection until it is closed by the server." */ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(02555) - "Unknown Transfer-Encoding: %s;" - " using read-until-close", tenc); + "Unknown Transfer-Encoding: %s; " + "using read-until-close", tenc); tenc = NULL; } else { @@ -308,22 +339,20 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, || endstr == lenp || *endstr || ctx->remaining < 0) { ctx->remaining = 0; - ap_log_rerror( - APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01587) - "Invalid Content-Length"); + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01587) + "Invalid Content-Length"); - return APR_ENOSPC; + return APR_EINVAL; } /* If we have a limit in effect and we know the C-L ahead of * time, stop it here if it is invalid. */ if (ctx->limit && ctx->limit < ctx->remaining) { - ap_log_rerror( - APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01588) - "Requested content-length of %" APR_OFF_T_FMT - " is larger than the configured limit" - " of %" APR_OFF_T_FMT, ctx->remaining, ctx->limit); + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01588) + "Requested content-length of %" APR_OFF_T_FMT + " is larger than the configured limit" + " of %" APR_OFF_T_FMT, ctx->remaining, ctx->limit); return APR_ENOSPC; } } @@ -378,6 +407,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, APR_BRIGADE_INSERT_TAIL(bb, e); rv = ap_pass_brigade(f->c->output_filters, bb); + apr_brigade_cleanup(bb); if (rv != APR_SUCCESS) { return AP_FILTER_ERROR; } @@ -401,7 +431,9 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, case BODY_CHUNK: case BODY_CHUNK_PART: case BODY_CHUNK_EXT: - case BODY_CHUNK_END: { + case BODY_CHUNK_LF: + case BODY_CHUNK_END: + case BODY_CHUNK_END_LF: { rv = ap_get_brigade(f->next, b, AP_MODE_GETLINE, block, 0); @@ -433,8 +465,9 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, f->r->server->limit_req_fieldsize); } if (rv != APR_SUCCESS) { - ap_log_rerror( - APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590) "Error reading chunk %s ", (APR_ENOSPC == rv) ? "(overflow)" : ""); + ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590) + "Error reading chunk %s ", + (APR_ENOSPC == rv) ? "(overflow)" : ""); return rv; } } @@ -446,9 +479,8 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, if (ctx->state == BODY_CHUNK_TRAILER) { /* Treat UNSET as DISABLE - trailers aren't merged by default */ - int merge_trailers = - conf->merge_trailers == AP_MERGE_TRAILERS_ENABLE; - return read_chunked_trailers(ctx, f, b, merge_trailers); + return read_chunked_trailers(ctx, f, b, + conf->merge_trailers == AP_MERGE_TRAILERS_ENABLE); } break; @@ -522,9 +554,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, * really count. This seems to be up for interpretation. */ ctx->limit_used += totalread; if (ctx->limit < ctx->limit_used) { - ap_log_rerror( - APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01591) "Read content-length of %" APR_OFF_T_FMT " is larger than the configured limit" - " of %" APR_OFF_T_FMT, ctx->limit_used, ctx->limit); + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01591) + "Read content-length of %" APR_OFF_T_FMT + " is larger than the configured limit" + " of %" APR_OFF_T_FMT, ctx->limit_used, ctx->limit); return APR_ENOSPC; } } @@ -549,7 +582,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, break; } default: { - break; + /* Should not happen */ + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, f->r, APLOGNO(02901) + "Unexpected body state (%i)", (int)ctx->state); + return APR_EGENERAL; } }