limiting number of HTTP request headers
Maxim Dounin
mdounin at mdounin.ru
Tue May 21 13:17:26 UTC 2024
Hello!
On Thu, May 16, 2024 at 11:01:36AM -0700, Maksim Yevmenkin wrote:
> Hello,
>
> > > Could the community share their thoughts on introducing a directive to
> > > cap the number of HTTP request headers? While we currently have the
> > > ability to limit client HTTP request buffer size, having more specific
> > > control over the number of headers could be advantageous.
> >
> > I personally tend to think that buffer size limit is enough for
> > [free]nginx itself. On the other hand, an additional limit on the
> > number of request headers might be beneficial to protect backend
> > servers, and might worth adding.
>
> thanks! a possible patch is attached.
>
> max
> diff --git a/ports/netflix/nginx/files/nginx/src/http/ngx_http_core_module.c b/ports/netflix/nginx/files/nginx/src/http/ngx_http_core_module.c
> index e26705475e46..b12951fd2a09 100644
> --- a/ports/netflix/nginx/files/nginx/src/http/ngx_http_core_module.c
> +++ b/ports/netflix/nginx/files/nginx/src/http/ngx_http_core_module.c
> @@ -287,6 +287,13 @@ static ngx_command_t ngx_http_core_commands[] = {
> offsetof(ngx_http_core_srv_conf_t, underscores_in_headers),
> NULL },
>
> + { ngx_string("max_request_headers"),
> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
> + ngx_conf_set_num_slot,
> + NGX_HTTP_SRV_CONF_OFFSET,
> + offsetof(ngx_http_core_srv_conf_t, max_request_headers),
> + NULL },
> +
Given the names of other related directives, such as:
client_header_buffer_size, large_client_header_buffers,
client_max_body_size, and so on
I tend to think that "max_client_headers" would be a better name.
> { ngx_string("location"),
> NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_BLOCK|NGX_CONF_TAKE12,
> ngx_http_core_location,
> @@ -3508,6 +3515,7 @@ ngx_http_core_create_srv_conf(ngx_conf_t *cf)
> cscf->ignore_invalid_headers = NGX_CONF_UNSET;
> cscf->merge_slashes = NGX_CONF_UNSET;
> cscf->underscores_in_headers = NGX_CONF_UNSET;
> + cscf->max_request_headers = NGX_CONF_UNSET_UINT;
>
> cscf->file_name = cf->conf_file->file.name.data;
> cscf->line = cf->conf_file->line;
> @@ -3554,6 +3562,9 @@ ngx_http_core_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child)
> ngx_conf_merge_value(conf->underscores_in_headers,
> prev->underscores_in_headers, 0);
>
> + ngx_conf_merge_value(conf->max_request_headers,
> + prev->max_request_headers, 128);
> +
I suspect 128 might have a direct impact on some legitimate
workloads.
I would rather suggests either NGX_MAX_INT32_VALUE as the default,
similarly to max_ranges, effectively switching it off by default,
or a larger value, such as 1000, similarly to recently introduced
max_commands in the mail proxy module.
> if (conf->server_names.nelts == 0) {
> /* the array has 4 empty preallocated elements, so push cannot fail */
> sn = ngx_array_push(&conf->server_names);
> diff --git a/ports/netflix/nginx/files/nginx/src/http/ngx_http_core_module.h b/ports/netflix/nginx/files/nginx/src/http/ngx_http_core_module.h
> index 3ed9d1632eec..1a5853fc7dc1 100644
> --- a/ports/netflix/nginx/files/nginx/src/http/ngx_http_core_module.h
> +++ b/ports/netflix/nginx/files/nginx/src/http/ngx_http_core_module.h
> @@ -212,6 +212,7 @@ typedef struct {
> ngx_flag_t ignore_invalid_headers;
> ngx_flag_t merge_slashes;
> ngx_flag_t underscores_in_headers;
> + ngx_uint_t max_request_headers; /* max number of request headers */
>
> unsigned listen:1;
> #if (NGX_PCRE)
The comment looks unneeded here.
> diff --git a/ports/netflix/nginx/files/nginx/src/http/ngx_http_request.c b/ports/netflix/nginx/files/nginx/src/http/ngx_http_request.c
> index 2ed0d99c168b..1ff03b215880 100644
> --- a/ports/netflix/nginx/files/nginx/src/http/ngx_http_request.c
> +++ b/ports/netflix/nginx/files/nginx/src/http/ngx_http_request.c
> @@ -1728,6 +1728,7 @@ ngx_http_process_request_headers(ngx_event_t *rev)
> ngx_http_request_t *r;
> ngx_http_core_srv_conf_t *cscf;
> ngx_http_core_main_conf_t *cmcf;
> + ngx_list_part_t *part;
Nitpicking: as per style, local variables should be sorted per
type length.
>
> c = rev->data;
> r = c->data;
> @@ -1816,6 +1817,25 @@ ngx_http_process_request_headers(ngx_event_t *rev)
> continue;
> }
>
> + /* apply request header limit */
Note that there are at least two other places where request
headers can be added: in HTTP/2 and in HTTP/3.
Also, this might be better to add this chunk after the "parsed
successfully" comment, right before the ngx_list_push() call.
> +
> + for (rv = 0, part = &r->headers_in.headers.part;
> + part != NULL;
> + part = part->next) {
> + rv += part->nelts;
> + }
I don't like the idea of counting all the headers for each header.
Introducing a counter might be a better approach.
> +
> + if (rv >= cscf->max_request_headers) {
> + ngx_log_error(NGX_LOG_INFO, c->log, 0,
> + "client sent too many request headers, have %i, limit is %ui",
> + rv, cscf->max_request_headers);
> +
> + r->lingering_close = 1;
> + ngx_http_finalize_request(r, NGX_HTTP_REQUEST_ENTITY_TOO_LARGE);
The 413 error code is not really appropriate here, as it is about
the HTTP message body, not headers.
In similar situations, such as when client header buffers are
exhausted or a header is too long to fit into one buffer, existing
code uses internal code 494 (NGX_HTTP_REQUEST_HEADER_TOO_LARGE),
which then maps to 400. It might be a good idea to do the same
here.
(Alternatively, 431 (Request Header Fields Too Large) introduced
in RFC 6585 might be appropriate here, yet switching to it will
require multiple changes.)
> +
> + break;
> + }
> +
> /* a header line has been parsed successfully */
>
> h = ngx_list_push(&r->headers_in.headers);
Below is an attempt to address the above comments, please take a
look.
# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1716296209 -10800
# Tue May 21 15:56:49 2024 +0300
# Node ID 2850868eb5e8c0dff91f86f75cb36886d54af001
# Parent 429d40e8275d2606da7cb710de5bc40a905fe52f
Added max_client_headers directive.
The directive limits the number of request headers accepted from clients.
While the total amount of headers is believed to be sufficiently limited
by the existing buffer size limits (client_header_buffer_size and
large_client_header_buffers), the additional limit on the number of headers
might be beneficial to better protect backend servers.
Requested by Maksim Yevmenkin.
diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
--- a/src/http/ngx_http_core_module.c
+++ b/src/http/ngx_http_core_module.c
@@ -252,6 +252,13 @@ static ngx_command_t ngx_http_core_comm
offsetof(ngx_http_core_srv_conf_t, large_client_header_buffers),
NULL },
+ { ngx_string("max_client_headers"),
+ NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
+ ngx_conf_set_num_slot,
+ NGX_HTTP_SRV_CONF_OFFSET,
+ offsetof(ngx_http_core_srv_conf_t, max_client_headers),
+ NULL },
+
{ ngx_string("ignore_invalid_headers"),
NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_FLAG,
ngx_conf_set_flag_slot,
@@ -3463,6 +3470,7 @@ ngx_http_core_create_srv_conf(ngx_conf_t
cscf->request_pool_size = NGX_CONF_UNSET_SIZE;
cscf->client_header_timeout = NGX_CONF_UNSET_MSEC;
cscf->client_header_buffer_size = NGX_CONF_UNSET_SIZE;
+ cscf->max_client_headers = NGX_CONF_UNSET_UINT;
cscf->ignore_invalid_headers = NGX_CONF_UNSET;
cscf->merge_slashes = NGX_CONF_UNSET;
cscf->underscores_in_headers = NGX_CONF_UNSET;
@@ -3504,6 +3512,9 @@ ngx_http_core_merge_srv_conf(ngx_conf_t
return NGX_CONF_ERROR;
}
+ ngx_conf_merge_uint_value(conf->max_client_headers,
+ prev->max_client_headers, 1000);
+
ngx_conf_merge_value(conf->ignore_invalid_headers,
prev->ignore_invalid_headers, 1);
diff --git a/src/http/ngx_http_core_module.h b/src/http/ngx_http_core_module.h
--- a/src/http/ngx_http_core_module.h
+++ b/src/http/ngx_http_core_module.h
@@ -198,6 +198,8 @@ typedef struct {
ngx_msec_t client_header_timeout;
+ ngx_uint_t max_client_headers;
+
ngx_flag_t ignore_invalid_headers;
ngx_flag_t merge_slashes;
ngx_flag_t underscores_in_headers;
diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
--- a/src/http/ngx_http_request.c
+++ b/src/http/ngx_http_request.c
@@ -1466,6 +1466,15 @@ ngx_http_process_request_headers(ngx_eve
/* a header line has been parsed successfully */
+ if (r->headers_in.count++ >= cscf->max_client_headers) {
+ r->lingering_close = 1;
+ ngx_log_error(NGX_LOG_INFO, c->log, 0,
+ "client sent too many header lines");
+ ngx_http_finalize_request(r,
+ NGX_HTTP_REQUEST_HEADER_TOO_LARGE);
+ break;
+ }
+
h = ngx_list_push(&r->headers_in.headers);
if (h == NULL) {
ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h
--- a/src/http/ngx_http_request.h
+++ b/src/http/ngx_http_request.h
@@ -182,6 +182,7 @@ typedef struct {
typedef struct {
ngx_list_t headers;
+ ngx_uint_t count;
ngx_table_elt_t *host;
ngx_table_elt_t *connection;
diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
--- a/src/http/v2/ngx_http_v2.c
+++ b/src/http/v2/ngx_http_v2.c
@@ -1815,6 +1815,15 @@ ngx_http_v2_state_process_header(ngx_htt
}
} else {
+ cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module);
+
+ if (r->headers_in.count++ >= cscf->max_client_headers) {
+ ngx_log_error(NGX_LOG_INFO, r->connection->log, 0,
+ "client sent too many header lines");
+ ngx_http_finalize_request(r, NGX_HTTP_REQUEST_HEADER_TOO_LARGE);
+ goto error;
+ }
+
h = ngx_list_push(&r->headers_in.headers);
if (h == NULL) {
return ngx_http_v2_connection_error(h2c,
diff --git a/src/http/v3/ngx_http_v3_request.c b/src/http/v3/ngx_http_v3_request.c
--- a/src/http/v3/ngx_http_v3_request.c
+++ b/src/http/v3/ngx_http_v3_request.c
@@ -657,6 +657,15 @@ ngx_http_v3_process_header(ngx_http_requ
}
} else {
+ cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module);
+
+ if (r->headers_in.count++ >= cscf->max_client_headers) {
+ ngx_log_error(NGX_LOG_INFO, r->connection->log, 0,
+ "client sent too many header lines");
+ ngx_http_finalize_request(r, NGX_HTTP_REQUEST_HEADER_TOO_LARGE);
+ return NGX_ERROR;
+ }
+
h = ngx_list_push(&r->headers_in.headers);
if (h == NULL) {
ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
--
Maxim Dounin
http://mdounin.ru/
More information about the nginx-devel
mailing list