When request headers are too long, some headers get removed

[Darkness4]

Hello,

I noticed an issue which removed original headers in the upstream request, caused by having too many headers. This is not caused by buffers configuration, but it is caused by these lines:

https://github.com/curityio/nginx_phantom_token_module/blob/cf500a61ef0bf91fcd4e3d440f8262a1f853ff69/phantom_token.c#L213 https://github.com/curityio/nginx_phantom_token_module/blob/cf500a61ef0bf91fcd4e3d440f8262a1f853ff69/phantom_token.c#L273 https://github.com/curityio/nginx_phantom_token_module/blob/cf500a61ef0bf91fcd4e3d440f8262a1f853ff69/phantom_token.c#L465

nelts is the "Number of ELemenTS". However, the issue is that part is considered as last, which is not always true.

But, that's not all, I believe there is also an algorithmic issue when adding, editing or removing headers.

nelts manipulation issue

For context, headers is a linked list of buffers (ngx_list_t), where each buffer (.part) is an array of struct header (ngx_list_part_t *).

Usually, to loop the list, you should do:

static ngx_table_elt_t *find_header_in(ngx_http_request_t *r, ngx_str_t key) {
    ngx_list_part_t *part;
    ngx_table_elt_t *h;
    ngx_uint_t i;

    part = &r->headers_in.headers.part;
    h = part->elts;

    for (i = 0;; i++) {
        if (i >= part->nelts) {
            if (part->next == NULL) {
                break;
            }

            // Walk next part
            part = part->next;
            h = part->elts;
            i = 0;
        }

        if (h[i].key.len != key.len ||
            ngx_strncasecmp(h[i].key.data, key.data, key.len) != 0) {
            continue; // Continue if not matched
        }

        // Found
        return &h[i];
    }
    return NULL;
}

As documented here: https://github.com/nginx/nginx/blob/master/src/core/ngx_list.h

For the first and third occurences (L213 and L465): We are adding a header. request->headers_in.headers.part correspond to the first element. When there aren't many headers, &request->headers_in.headers.part == request->headers_in.headers->last.

Upon appending a new value (like in line 199), ngx_list_push could result a simple append in an existing buffer, or it could result in new buffer.

In the first scenario, it is correct to update request->headers_in.headers.part since headers_in.headers.part is not a pointer but a copy.

In the second scenario, since the first buffer is full, we shouldn't update the nelts.

TL;DR: The fix is:

// If last == part, we need to update the number of elements
if (request->headers_in.headers.part.next == NULL) {
  request->headers_in.headers.part.nelts =
    request->headers_in.headers.last->nelts;
}

For the second occurence (L273): We are removing a header Content-Type. There are two issues here:

• The same nelts issue.
• It is possible that Content-Type is not the last element.

Solving the nelts issue is the same: check if part == last. For the second issue, we need to use a function to remove it.

Upsert and removing headers issues

As seen in the previous section, the header removal is not done. But there is also one more issue. Headers that could hold the same key are added. For example, L213 and L465 could add an existing Accept header instead of reusing the existing one.

I would recommend to use a function like in the nginx more headers module: https://github.com/openresty/headers-more-nginx-module/blob/84a65d68687c9de5166fd49ddbbd68c6962234eb/src/ngx_http_headers_more_headers_in.c#L220-L221

to upsert. It also includes a remove function which will solve the L273 issue.

I would gladly make a PR if you accept to use the helper functions from the more headers module.

(We did this internally)