From: Eric Covener Date: Mon, 7 Jul 2025 11:56:48 +0000 (+0000) Subject: improve h2 header error handling X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=a6b8db04ec2436d6fe5ef7ac70211a02b454113b;p=thirdparty%2Fapache%2Fhttpd.git improve h2 header error handling git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1927038 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/modules/http2/h2_request.c b/modules/http2/h2_request.c index 2713947c37..6373e0a244 100644 --- a/modules/http2/h2_request.c +++ b/modules/http2/h2_request.c @@ -64,18 +64,20 @@ typedef struct { apr_table_t *headers; apr_pool_t *pool; apr_status_t status; + h2_hd_scratch *scratch; } h1_ctx; static int set_h1_header(void *ctx, const char *key, const char *value) { h1_ctx *x = ctx; int was_added; - h2_req_add_header(x->headers, x->pool, key, strlen(key), value, strlen(value), 0, &was_added); + h2_req_add_header(x->headers, x->pool, key, strlen(key), + value, strlen(value), x->scratch, &was_added); return 1; } apr_status_t h2_request_rcreate(h2_request **preq, apr_pool_t *pool, - request_rec *r) + request_rec *r, h2_hd_scratch *scratch) { h2_request *req; const char *scheme, *authority, *path; @@ -125,6 +127,7 @@ apr_status_t h2_request_rcreate(h2_request **preq, apr_pool_t *pool, x.pool = pool; x.headers = req->headers; x.status = APR_SUCCESS; + x.scratch = scratch; apr_table_do(set_h1_header, &x, r->headers_in, NULL); *preq = req; @@ -134,7 +137,8 @@ apr_status_t h2_request_rcreate(h2_request **preq, apr_pool_t *pool, apr_status_t h2_request_add_header(h2_request *req, apr_pool_t *pool, const char *name, size_t nlen, const char *value, size_t vlen, - size_t max_field_len, int *pwas_added) + struct h2_hd_scratch *scratch, + int *pwas_added) { apr_status_t status = APR_SUCCESS; @@ -185,7 +189,7 @@ apr_status_t h2_request_add_header(h2_request *req, apr_pool_t *pool, else { /* non-pseudo header, add to table */ status = h2_req_add_header(req->headers, pool, name, nlen, value, vlen, - max_field_len, pwas_added); + scratch, pwas_added); } return status; diff --git a/modules/http2/h2_request.h b/modules/http2/h2_request.h index 7e20b69724..ae6b6a2510 100644 --- a/modules/http2/h2_request.h +++ b/modules/http2/h2_request.h @@ -19,17 +19,21 @@ #include "h2.h" +struct h2_hd_scratch; + h2_request *h2_request_create(int id, apr_pool_t *pool, const char *method, const char *scheme, const char *authority, const char *path, apr_table_t *header); apr_status_t h2_request_rcreate(h2_request **preq, apr_pool_t *pool, - request_rec *r); + request_rec *r, + struct h2_hd_scratch *scratch); apr_status_t h2_request_add_header(h2_request *req, apr_pool_t *pool, const char *name, size_t nlen, const char *value, size_t vlen, - size_t max_field_len, int *pwas_added); + struct h2_hd_scratch *scratch, + int *pwas_added); apr_status_t h2_request_add_trailer(h2_request *req, apr_pool_t *pool, const char *name, size_t nlen, diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c index fc8b6119ae..a5f1872bc2 100644 --- a/modules/http2/h2_session.c +++ b/modules/http2/h2_session.c @@ -109,13 +109,29 @@ static void cleanup_unprocessed_streams(h2_session *session) h2_mplx_c1_streams_do(session->mplx, rst_unprocessed_stream, session); } +/* APR callback invoked if allocation fails. */ +static int abort_on_oom(int retcode) +{ + ap_abort_on_oom(); + return retcode; /* unreachable, hopefully. */ +} + static h2_stream *h2_session_open_stream(h2_session *session, int stream_id, int initiated_on) { h2_stream * stream; + apr_allocator_t *allocator; apr_pool_t *stream_pool; + apr_status_t rv; - apr_pool_create(&stream_pool, session->pool); + rv = apr_allocator_create(&allocator); + if (rv != APR_SUCCESS) + return NULL; + + apr_allocator_max_free_set(allocator, ap_max_mem_free); + apr_pool_create_ex(&stream_pool, session->pool, NULL, allocator); + apr_allocator_owner_set(allocator, stream_pool); + apr_pool_abort_set(abort_on_oom, stream_pool); apr_pool_tag(stream_pool, "h2_stream"); stream = h2_stream_create(stream_id, stream_pool, session, @@ -972,6 +988,14 @@ apr_status_t h2_session_create(h2_session **psession, conn_rec *c, request_rec * } h2_c1_io_init(&session->io, session); + /* setup request header scratch buffers */ + session->hd_scratch.max_len = session->s->limit_req_fieldsize? + session->s->limit_req_fieldsize : 8190; + session->hd_scratch.name = + apr_pcalloc(session->pool, session->hd_scratch.max_len + 1); + session->hd_scratch.value = + apr_pcalloc(session->pool, session->hd_scratch.max_len + 1); + session->padding_max = h2_config_sgeti(s, H2_CONF_PADDING_BITS); if (session->padding_max) { session->padding_max = (0x01 << session->padding_max) - 1; @@ -1032,7 +1056,7 @@ apr_status_t h2_session_create(h2_session **psession, conn_rec *c, request_rec * n = h2_config_sgeti(s, H2_CONF_PUSH_DIARY_SIZE); session->push_diary = h2_push_diary_create(session->pool, n); - + if (APLOGcdebug(c)) { ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, H2_SSSN_LOG(APLOGNO(03200), session, @@ -1699,9 +1723,10 @@ static void on_stream_state_enter(void *ctx, h2_stream *stream) break; case H2_SS_CLEANUP: nghttp2_session_set_stream_user_data(session->ngh2, stream->id, NULL); + update_child_status(session, SERVER_BUSY_WRITE, "done", stream); h2_mplx_c1_stream_cleanup(session->mplx, stream, &session->open_streams); + stream = NULL; ++session->streams_done; - update_child_status(session, SERVER_BUSY_WRITE, "done", stream); break; default: break; diff --git a/modules/http2/h2_session.h b/modules/http2/h2_session.h index 2c8f334cce..7932a9e2cc 100644 --- a/modules/http2/h2_session.h +++ b/modules/http2/h2_session.h @@ -29,6 +29,7 @@ */ #include "h2.h" +#include "h2_util.h" struct apr_thread_mutext_t; struct apr_thread_cond_t; @@ -118,6 +119,8 @@ typedef struct h2_session { struct h2_iqueue *out_c1_blocked; /* all streams with output blocked on c1 buffer full */ struct h2_iqueue *ready_to_process; /* all streams ready for processing */ + h2_hd_scratch hd_scratch; + } h2_session; const char *h2_session_state_str(h2_session_state state); diff --git a/modules/http2/h2_stream.c b/modules/http2/h2_stream.c index a50713cac0..f0e671ca5d 100644 --- a/modules/http2/h2_stream.c +++ b/modules/http2/h2_stream.c @@ -659,7 +659,8 @@ apr_status_t h2_stream_set_request_rec(h2_stream *stream, if (stream->rst_error) { return APR_ECONNRESET; } - status = h2_request_rcreate(&req, stream->pool, r); + status = h2_request_rcreate(&req, stream->pool, r, + &stream->session->hd_scratch); if (status == APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_DEBUG, status, r, H2_STRM_LOG(APLOGNO(03058), stream, @@ -691,13 +692,11 @@ static void set_error_response(h2_stream *stream, int http_status) static apr_status_t add_trailer(h2_stream *stream, const char *name, size_t nlen, const char *value, size_t vlen, - size_t max_field_len, int *pwas_added) + h2_hd_scratch *scratch) { conn_rec *c = stream->session->c1; - char *hname, *hvalue; const char *existing; - *pwas_added = 0; if (nlen == 0 || name[0] == ':') { ap_log_cerror(APLOG_MARK, APLOG_DEBUG, APR_EINVAL, c, H2_STRM_LOG(APLOGNO(03060), stream, @@ -710,20 +709,35 @@ static apr_status_t add_trailer(h2_stream *stream, if (!stream->trailers_in) { stream->trailers_in = apr_table_make(stream->pool, 5); } - hname = apr_pstrndup(stream->pool, name, nlen); - h2_util_camel_case_header(hname, nlen); - existing = apr_table_get(stream->trailers_in, hname); - if (max_field_len - && ((existing? strlen(existing)+2 : 0) + vlen + nlen + 2 > max_field_len)) { - /* "key: (oldval, )?nval" is too long */ + + if (((nlen + vlen + 2) > scratch->max_len)) return APR_EINVAL; + + /* We need 0-terminated strings to operate on apr_table */ + AP_DEBUG_ASSERT(nlen < scratch->max_len); + memcpy(scratch->name, name, nlen); + scratch->name[nlen] = 0; + AP_DEBUG_ASSERT(vlen < scratch->max_len); + memcpy(scratch->value, value, vlen); + scratch->value[vlen] = 0; + + existing = apr_table_get(stream->trailers_in, scratch->name); + if(existing) { + if (!vlen) /* not adding a 0-length value to existing */ + return APR_SUCCESS; + if ((strlen(existing) + 2 + vlen + nlen + 2 > scratch->max_len)) { + /* "name: existing, value" is too long */ + return APR_EINVAL; + } + apr_table_merge(stream->trailers_in, scratch->name, scratch->value); } - if (!existing) *pwas_added = 1; - hvalue = apr_pstrndup(stream->pool, value, vlen); - apr_table_mergen(stream->trailers_in, hname, hvalue); - ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c, - H2_STRM_MSG(stream, "added trailer '%s: %s'"), hname, hvalue); - + else { + h2_util_camel_case_header(scratch->name, nlen); + apr_table_set(stream->trailers_in, scratch->name, scratch->value); + } + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c, + H2_STRM_MSG(stream, "added trailer '%s: %s'"), + scratch->name, scratch->value); return APR_SUCCESS; } @@ -732,7 +746,7 @@ apr_status_t h2_stream_add_header(h2_stream *stream, const char *value, size_t vlen) { h2_session *session = stream->session; - int error = 0, was_added = 0; + int error = 0; apr_status_t status = APR_SUCCESS; H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); @@ -760,6 +774,7 @@ apr_status_t h2_stream_add_header(h2_stream *stream, ++stream->request_headers_added; } else if (H2_SS_IDLE == stream->state) { + int was_added; if (!stream->rtmp) { if (H2_STREAM_CLIENT_INITIATED(stream->id)) { ++stream->session->remote.emitted_count; @@ -771,7 +786,7 @@ apr_status_t h2_stream_add_header(h2_stream *stream, } status = h2_request_add_header(stream->rtmp, stream->pool, name, nlen, value, vlen, - session->s->limit_req_fieldsize, &was_added); + &session->hd_scratch, &was_added); ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, session->c1, H2_STRM_MSG(stream, "add_header: '%.*s: %.*s"), (int)nlen, name, (int)vlen, value); @@ -779,8 +794,8 @@ apr_status_t h2_stream_add_header(h2_stream *stream, } else if (H2_SS_OPEN == stream->state) { status = add_trailer(stream, name, nlen, value, vlen, - session->s->limit_req_fieldsize, &was_added); - if (was_added) ++stream->request_headers_added; + &session->hd_scratch); + if (!status) ++stream->request_headers_added; } else { status = APR_EINVAL; @@ -789,16 +804,17 @@ apr_status_t h2_stream_add_header(h2_stream *stream, if (APR_EINVAL == status) { /* header too long */ - if (!h2_stream_is_ready(stream)) { + if (!h2_stream_is_ready(stream) && !stream->request_headers_failed) { ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, session->c1, - H2_STRM_LOG(APLOGNO(10180), stream,"Request header exceeds " - "LimitRequestFieldSize: %.*s"), + H2_STRM_LOG(APLOGNO(10180), stream, + "Request header exceeds LimitRequestFieldSize(%d): %.*s"), + (int)session->hd_scratch.max_len, (int)H2MIN(nlen, 80), name); } error = HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE; goto cleanup; } - + if (session->s->limit_req_fields > 0 && stream->request_headers_added > session->s->limit_req_fields) { /* too many header lines */ @@ -810,12 +826,13 @@ apr_status_t h2_stream_add_header(h2_stream *stream, if (!h2_stream_is_ready(stream)) { ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, session->c1, H2_STRM_LOG(APLOGNO(10181), stream, "Number of request headers " - "exceeds LimitRequestFields")); + "exceeds LimitRequestFields(%d)"), + (int)session->s->limit_req_fields); } error = HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE; goto cleanup; } - + cleanup: if (error) { ++stream->request_headers_failed; diff --git a/modules/http2/h2_util.c b/modules/http2/h2_util.c index 8e53cebdf9..605c348ca1 100644 --- a/modules/http2/h2_util.c +++ b/modules/http2/h2_util.c @@ -1693,10 +1693,9 @@ int h2_ignore_resp_trailer(const char *name, size_t len) } static apr_status_t req_add_header(apr_table_t *headers, apr_pool_t *pool, - nghttp2_nv *nv, size_t max_field_len, + nghttp2_nv *nv, h2_hd_scratch *scratch, int *pwas_added) { - char *hname, *hvalue; const char *existing; *pwas_added = 0; @@ -1712,15 +1711,14 @@ static apr_status_t req_add_header(apr_table_t *headers, apr_pool_t *pool, /* Cookie header come separately in HTTP/2, but need * to be merged by "; " (instead of default ", ") */ - if (max_field_len - && strlen(existing) + nv->valuelen + nv->namelen + 4 - > max_field_len) { + if ((strlen(existing) + nv->valuelen + nv->namelen + 4) + > scratch->max_len) { /* "key: oldval, nval" is too long */ return APR_EINVAL; } - hvalue = apr_pstrndup(pool, (const char*)nv->value, nv->valuelen); apr_table_setn(headers, "Cookie", - apr_psprintf(pool, "%s; %s", existing, hvalue)); + apr_psprintf(pool, "%s; %.*s", existing, + (int)nv->valuelen, nv->value)); return APR_SUCCESS; } } @@ -1731,27 +1729,40 @@ static apr_status_t req_add_header(apr_table_t *headers, apr_pool_t *pool, } } - hname = apr_pstrndup(pool, (const char*)nv->name, nv->namelen); - h2_util_camel_case_header(hname, nv->namelen); - existing = apr_table_get(headers, hname); - if (max_field_len) { - if ((existing? strlen(existing)+2 : 0) + nv->valuelen + nv->namelen + 2 - > max_field_len) { - /* "key: (oldval, )?nval" is too long */ + if (((nv->namelen + nv->valuelen + 2) > scratch->max_len)) + return APR_EINVAL; + + /* We need 0-terminated strings to operate on apr_table */ + AP_DEBUG_ASSERT(nv->namelen < scratch->max_len); + memcpy(scratch->name, nv->name, nv->namelen); + scratch->name[nv->namelen] = 0; + AP_DEBUG_ASSERT(nv->valuelen < scratch->max_len); + memcpy(scratch->value, nv->value, nv->valuelen); + scratch->value[nv->valuelen] = 0; + + *pwas_added = 1; + existing = apr_table_get(headers, scratch->name); + if (existing) { + if (!nv->valuelen) /* not adding a 0-length value to existing */ + return APR_SUCCESS; + if ((strlen(existing) + 2 + nv->valuelen + nv->namelen + 2) + > scratch->max_len) { + /* "name: existing, value" is too long */ return APR_EINVAL; } + apr_table_merge(headers, scratch->name, scratch->value); + } + else { + h2_util_camel_case_header(scratch->name, nv->namelen); + apr_table_set(headers, scratch->name, scratch->value); } - if (!existing) *pwas_added = 1; - hvalue = apr_pstrndup(pool, (const char*)nv->value, nv->valuelen); - apr_table_mergen(headers, hname, hvalue); - return APR_SUCCESS; } apr_status_t h2_req_add_header(apr_table_t *headers, apr_pool_t *pool, const char *name, size_t nlen, const char *value, size_t vlen, - size_t max_field_len, int *pwas_added) + h2_hd_scratch *scratch, int *pwas_added) { nghttp2_nv nv; @@ -1759,7 +1770,7 @@ apr_status_t h2_req_add_header(apr_table_t *headers, apr_pool_t *pool, nv.namelen = nlen; nv.value = (uint8_t*)value; nv.valuelen = vlen; - return req_add_header(headers, pool, &nv, max_field_len, pwas_added); + return req_add_header(headers, pool, &nv, scratch, pwas_added); } /******************************************************************************* diff --git a/modules/http2/h2_util.h b/modules/http2/h2_util.h index dcec73eaf2..d1a809c52b 100644 --- a/modules/http2/h2_util.h +++ b/modules/http2/h2_util.h @@ -397,14 +397,21 @@ apr_status_t h2_req_create_ngheader(h2_ngheader **ph, apr_pool_t *p, const struct h2_request *req); #endif +typedef struct h2_hd_scratch { + size_t max_len; /* header field size name + ': ' + value */ + char *name; /* max_len+1 sized */ + char *value; /* max_len+1 sized */ + +} h2_hd_scratch; + /** * Add a HTTP/2 header and return the table key if it really was added * and not ignored. */ -apr_status_t h2_req_add_header(apr_table_t *headers, apr_pool_t *pool, +apr_status_t h2_req_add_header(apr_table_t *headers, apr_pool_t *pool, const char *name, size_t nlen, const char *value, size_t vlen, - size_t max_field_len, int *pwas_added); + h2_hd_scratch *scratch, int *pwas_added); /******************************************************************************* * apr brigade helpers diff --git a/test/modules/http2/test_200_header_invalid.py b/test/modules/http2/test_200_header_invalid.py index 6b73301c28..1687e3d981 100644 --- a/test/modules/http2/test_200_header_invalid.py +++ b/test/modules/http2/test_200_header_invalid.py @@ -133,7 +133,7 @@ class TestInvalidHeaders: assert 431 == r.response["status"] # test header field count, LimitRequestFields (default 100) - # see #201: several headers with same name are mered and count only once + # see #201: several headers with same name are merged and counted def test_h2_200_12(self, env): url = env.mkurl("https", "cgi", "/") opt = [] @@ -143,7 +143,7 @@ class TestInvalidHeaders: r = env.curl_get(url, options=opt) assert r.response["status"] == 200 r = env.curl_get(url, options=(opt + ["-H", "y: 2"])) - assert r.response["status"] == 200 + assert r.response["status"] == 431 # test header field count, LimitRequestFields (default 100) # different header names count each