HTTP: fix segfault reading a request header with no cache slot

Reading a request header via r.headersIn[name] could crash the worker
with SIGSEGV.  The header in question was "Proxy-Connection", but any
header that nginx registers without a dedicated ngx_table_elt_t * slot
was affected.

The getter mapped an arbitrary header name through ngx_hash_find() and
treated (char *) &r->headers_in + hh->offset as a pointer to a cache
slot.  This only holds for headers whose parser stores into such a
slot.  "Proxy-Connection" is registered in ngx_http_headers_in[] with
offset 0 and a handler that stores nothing, so the getter aliased the
ngx_list_t at the start of ngx_http_headers_in_t and followed its elts
pointer as a bogus ngx_table_elt_t.

The fix is to walk the parsed header list by name, as it is always safe.
The single-value and semicolon-join flags moved into the per-name header
table, and the now-unused slot pointer argument and its branch were
dropped from the generic getter.

The slot lookup that is removed was only an optimization for the names
in ngx_http_headers_in[]; for any other header it missed the hash and
walked the list anyway, so dropping it is not a performance regression.

This closes #1071 issue on Github.

diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c
index 67e2124712d93c9ced146fc06c651aad9113027c..bba6a44ca79202238eced906730577b222de3f35 100644 (file)
--- a/nginx/ngx_http_js_module.c
+++ b/nginx/ngx_http_js_module.c
@@ -389,8 +389,8 @@ static njs_int_t ngx_http_js_header_out_special(njs_vm_t *vm,
     ngx_http_request_t *r, njs_str_t *v, njs_value_t *setval,
     njs_value_t *retval, ngx_table_elt_t **hh);
 static njs_int_t ngx_http_js_header_generic(njs_vm_t *vm,
-    ngx_http_request_t *r, ngx_list_t *headers, ngx_table_elt_t **ph,
-    unsigned flags, njs_str_t *name, njs_value_t *retval);
+    ngx_http_request_t *r, ngx_list_t *headers, unsigned flags,
+    njs_str_t *name, njs_value_t *retval);
 #endif
 static njs_int_t ngx_http_js_content_encoding(njs_vm_t *vm,
     ngx_http_request_t *r, unsigned flags, njs_str_t *name,
@@ -4393,20 +4393,24 @@ static njs_int_t
 ngx_http_js_ext_header_in(njs_vm_t *vm, njs_object_prop_t *prop, uint32_t atom_id,
     njs_value_t *value, njs_value_t *unused, njs_value_t *retval)
 {
-    unsigned             flags;
-    njs_int_t            rc;
-    njs_str_t            name, *h;
-    ngx_http_request_t  *r;
+    unsigned               flags;
+    njs_int_t              rc;
+    njs_str_t              name;
+    ngx_http_request_t    *r;
+    ngx_http_js_header_t  *h;
 
-    static njs_str_t single_headers_in[] = {
-        njs_str("Content-Type"),
-        njs_str("ETag"),
-        njs_str("From"),
-        njs_str("Max-Forwards"),
-        njs_str("Referer"),
-        njs_str("Proxy-Authorization"),
-        njs_str("User-Agent"),
-        njs_str(""),
+    static ngx_http_js_header_t  headers_in[] = {
+#define header(name, fl)  { njs_str(name), fl, 0 }
+        header("Content-Type",        NJS_HEADER_SINGLE),
+        header("ETag",                NJS_HEADER_SINGLE),
+        header("From",                NJS_HEADER_SINGLE),
+        header("Max-Forwards",        NJS_HEADER_SINGLE),
+        header("Referer",             NJS_HEADER_SINGLE),
+        header("Proxy-Authorization", NJS_HEADER_SINGLE),
+        header("User-Agent",          NJS_HEADER_SINGLE),
+        header("Cookie",              NJS_HEADER_SEMICOLON),
+        header("",                    0),
+#undef header
     };
 
     r = njs_vm_external(vm, ngx_http_js_request_proto_id, value);
@@ -4429,11 +4433,11 @@ ngx_http_js_ext_header_in(njs_vm_t *vm, njs_object_prop_t *prop, uint32_t atom_i
 
     flags = 0;
 
-    for (h = single_headers_in; h->length > 0; h++) {
-        if (h->length == name.length
-            && ngx_strncasecmp(h->start, name.start, name.length) == 0)
+    for (h = headers_in; h->name.len > 0; h++) {
+        if (h->name.len == name.length
+            && ngx_strncasecmp(h->name.data, name.start, name.length) == 0)
         {
-            flags |= NJS_HEADER_SINGLE;
+            flags = h->flags;
             break;
         }
     }
@@ -5142,48 +5146,11 @@ static njs_int_t
 ngx_http_js_header_in(njs_vm_t *vm, ngx_http_request_t *r, unsigned flags,
     njs_str_t *name, njs_value_t *retval)
 {
-    u_char                      *lowcase_key;
-    ngx_uint_t                   hash;
-    ngx_table_elt_t            **ph;
-    ngx_http_header_t           *hh;
-    ngx_http_core_main_conf_t   *cmcf;
-    u_char                       storage[128];
-
     if (retval == NULL) {
         return NJS_OK;
     }
 
-    /* look up hashed headers */
-
-    if (name->length < sizeof(storage)) {
-        lowcase_key = storage;
-
-    } else {
-        lowcase_key = ngx_pnalloc(r->pool, name->length);
-        if (lowcase_key == NULL) {
-            njs_vm_memory_error(vm);
-            return NJS_ERROR;
-        }
-    }
-
-    hash = ngx_hash_strlow(lowcase_key, name->start, name->length);
-
-    cmcf = ngx_http_get_module_main_conf(r, ngx_http_core_module);
-
-    hh = ngx_hash_find(&cmcf->headers_in_hash, hash, lowcase_key,
-                       name->length);
-
-    ph = NULL;
-
-    if (hh) {
-        if (hh->offset == offsetof(ngx_http_headers_in_t, cookie)) {
-            flags |= NJS_HEADER_SEMICOLON;
-        }
-
-        ph = (ngx_table_elt_t **) ((char *) &r->headers_in + hh->offset);
-    }
-
-    return ngx_http_js_header_generic(vm, r, &r->headers_in.headers, ph, flags,
+    return ngx_http_js_header_generic(vm, r, &r->headers_in.headers, flags,
                                       name, retval);
 }
 
@@ -5203,9 +5170,8 @@ ngx_http_js_header_out(njs_vm_t *vm, ngx_http_request_t *r, unsigned flags,
     njs_opaque_value_t   lvalue;
 
     if (retval != NULL && setval == NULL) {
-        return ngx_http_js_header_generic(vm, r, &r->headers_out.headers, NULL,
-                                          flags, name, retval);
-
+        return ngx_http_js_header_generic(vm, r, &r->headers_out.headers, flags,
+                                          name, retval);
     }
 
     part = &r->headers_out.headers.part;
@@ -5430,8 +5396,7 @@ done:
 
 static njs_int_t
 ngx_http_js_header_generic(njs_vm_t *vm, ngx_http_request_t *r,
-    ngx_list_t *headers, ngx_table_elt_t **ph, unsigned flags, njs_str_t *name,
-    njs_value_t *retval)
+    ngx_list_t *headers, unsigned flags, njs_str_t *name, njs_value_t *retval)
 {
     u_char            sep;
     njs_chb_t         chain;
@@ -5439,43 +5404,38 @@ ngx_http_js_header_generic(njs_vm_t *vm, ngx_http_request_t *r,
     ngx_uint_t        i;
     njs_value_t      *value;
     ngx_list_part_t  *part;
-    ngx_table_elt_t  *header, *h;
-
-    if (ph == NULL) {
-        /* iterate over all headers */
-
-        ph = &header;
-        part = &headers->part;
-        h = part->elts;
+    ngx_table_elt_t  *header, *h, **ph;
 
-        for (i = 0; /* void */ ; i++) {
+    ph = &header;
+    part = &headers->part;
+    h = part->elts;
 
-            if (i >= part->nelts) {
-                if (part->next == NULL) {
-                    break;
-                }
+    for (i = 0; /* void */ ; i++) {
 
-                part = part->next;
-                h = part->elts;
-                i = 0;
+        if (i >= part->nelts) {
+            if (part->next == NULL) {
+                break;
             }
 
-            if (h[i].hash == 0
-                || name->length != h[i].key.len
-                || ngx_strncasecmp(name->start, h[i].key.data, name->length)
-                   != 0)
-            {
-                continue;
-            }
+            part = part->next;
+            h = part->elts;
+            i = 0;
+        }
 
-            *ph = &h[i];
-            ph = &h[i].next;
+        if (h[i].hash == 0
+            || name->length != h[i].key.len
+            || ngx_strncasecmp(name->start, h[i].key.data, name->length) != 0)
+        {
+            continue;
         }
 
-        *ph = NULL;
-        ph = &header;
+        *ph = &h[i];
+        ph = &h[i].next;
     }
 
+    *ph = NULL;
+    ph = &header;
+
     if (*ph == NULL) {
         njs_value_undefined_set(retval);
         return NJS_DECLINED;
@@ -8375,51 +8335,46 @@ ngx_http_qjs_headers_in_own_property_names(JSContext *cx,
 
 static njs_int_t
 ngx_http_qjs_header_generic(JSContext *cx, ngx_http_request_t *r,
-    ngx_list_t *headers, ngx_table_elt_t **ph, ngx_str_t *name,
-    JSPropertyDescriptor *pdesc, unsigned flags)
+    ngx_list_t *headers, ngx_str_t *name, JSPropertyDescriptor *pdesc,
+    unsigned flags)
 {
     u_char            sep;
     JSValue           val;
     njs_chb_t         chain;
     ngx_uint_t        i;
     ngx_list_part_t  *part;
-    ngx_table_elt_t  *header, *h;
-
-    if (ph == NULL) {
-        /* iterate over all headers */
-
-        ph = &header;
-        part = &headers->part;
-        h = part->elts;
+    ngx_table_elt_t  *header, *h, **ph;
 
-        for (i = 0; /* void */ ; i++) {
+    ph = &header;
+    part = &headers->part;
+    h = part->elts;
 
-            if (i >= part->nelts) {
-                if (part->next == NULL) {
-                    break;
-                }
+    for (i = 0; /* void */ ; i++) {
 
-                part = part->next;
-                h = part->elts;
-                i = 0;
+        if (i >= part->nelts) {
+            if (part->next == NULL) {
+                break;
             }
 
-            if (h[i].hash == 0
-                || name->len != h[i].key.len
-                || ngx_strncasecmp(name->data, h[i].key.data, name->len)
-                   != 0)
-            {
-                continue;
-            }
+            part = part->next;
+            h = part->elts;
+            i = 0;
+        }
 
-            *ph = &h[i];
-            ph = &h[i].next;
+        if (h[i].hash == 0
+            || name->len != h[i].key.len
+            || ngx_strncasecmp(name->data, h[i].key.data, name->len) != 0)
+        {
+            continue;
         }
 
-        *ph = NULL;
-        ph = &header;
+        *ph = &h[i];
+        ph = &h[i].next;
     }
 
+    *ph = NULL;
+    ph = &header;
+
     if (*ph == NULL) {
         return 0;
     }
@@ -8500,44 +8455,7 @@ static int
 ngx_http_qjs_header_in(JSContext *cx, ngx_http_request_t *r, unsigned flags,
     ngx_str_t *name, JSPropertyDescriptor *pdesc)
 {
-    u_char                      *lowcase_key;
-    ngx_uint_t                   hash;
-    ngx_table_elt_t            **ph;
-    ngx_http_header_t           *hh;
-    ngx_http_core_main_conf_t   *cmcf;
-    u_char                       storage[128];
-
-    /* look up hashed headers */
-
-    if (name->len < sizeof(storage)) {
-        lowcase_key = storage;
-
-    } else {
-        lowcase_key = ngx_pnalloc(r->pool, name->len);
-        if (lowcase_key == NULL) {
-            (void) JS_ThrowOutOfMemory(cx);
-            return -1;
-        }
-    }
-
-    hash = ngx_hash_strlow(lowcase_key, name->data, name->len);
-
-    cmcf = ngx_http_get_module_main_conf(r, ngx_http_core_module);
-
-    hh = ngx_hash_find(&cmcf->headers_in_hash, hash, lowcase_key,
-                       name->len);
-
-    ph = NULL;
-
-    if (hh) {
-        if (hh->offset == offsetof(ngx_http_headers_in_t, cookie)) {
-            flags |= NJS_HEADER_SEMICOLON;
-        }
-
-        ph = (ngx_table_elt_t **) ((char *) &r->headers_in + hh->offset);
-    }
-
-    return ngx_http_qjs_header_generic(cx, r, &r->headers_in.headers, ph, name,
+    return ngx_http_qjs_header_generic(cx, r, &r->headers_in.headers, name,
                                        pdesc, flags);
 }
 
@@ -8546,20 +8464,24 @@ static int
 ngx_http_qjs_headers_in_own_property(JSContext *cx, JSPropertyDescriptor *pdesc,
     JSValueConst obj, JSAtom prop)
 {
-    int                  ret;
-    unsigned             flags;
-    ngx_str_t            name, *h;
-    ngx_http_request_t  *r;
+    int                    ret;
+    unsigned               flags;
+    ngx_str_t              name;
+    ngx_http_request_t    *r;
+    ngx_http_js_header_t  *h;
 
-    static ngx_str_t single_headers_in[] = {
-        ngx_string("Content-Type"),
-        ngx_string("ETag"),
-        ngx_string("From"),
-        ngx_string("Max-Forwards"),
-        ngx_string("Referer"),
-        ngx_string("Proxy-Authorization"),
-        ngx_string("User-Agent"),
-        ngx_string(""),
+    static ngx_http_js_header_t  headers_in[] = {
+#define header(name, fl)  { ngx_string(name), fl, 0 }
+        header("Content-Type",        NJS_HEADER_SINGLE),
+        header("ETag",                NJS_HEADER_SINGLE),
+        header("From",                NJS_HEADER_SINGLE),
+        header("Max-Forwards",        NJS_HEADER_SINGLE),
+        header("Referer",             NJS_HEADER_SINGLE),
+        header("Proxy-Authorization", NJS_HEADER_SINGLE),
+        header("User-Agent",          NJS_HEADER_SINGLE),
+        header("Cookie",              NJS_HEADER_SEMICOLON),
+        header("",                    0),
+#undef header
     };
 
     r = JS_GetOpaque(obj, NGX_QJS_CLASS_ID_HTTP_HEADERS_IN);
@@ -8577,11 +8499,11 @@ ngx_http_qjs_headers_in_own_property(JSContext *cx, JSPropertyDescriptor *pdesc,
 
     flags = 0;
 
-    for (h = single_headers_in; h->len > 0; h++) {
-        if (h->len == name.len
-            && ngx_strncasecmp(h->data, name.data, name.len) == 0)
+    for (h = headers_in; h->name.len > 0; h++) {
+        if (h->name.len == name.len
+            && ngx_strncasecmp(h->name.data, name.data, name.len) == 0)
         {
-            flags |= NJS_HEADER_SINGLE;
+            flags = h->flags;
             break;
         }
     }
@@ -8670,7 +8592,7 @@ ngx_http_qjs_headers_out_handler(JSContext *cx, ngx_http_request_t *r,
     ngx_table_elt_t  *header, *h, **ph;
 
     if (flags & NJS_HEADER_GET) {
-        return ngx_http_qjs_header_generic(cx, r, &r->headers_out.headers, NULL,
+        return ngx_http_qjs_header_generic(cx, r, &r->headers_out.headers,
                                            name, pdesc, flags);
     }
 

diff --git a/nginx/t/js_headers.t b/nginx/t/js_headers.t
index 7cf910990719d6144403c1776b0cf5ddd5a9904a..ea8a31c634b731b64b8f17b02f681220cb8bd233 100644 (file)
--- a/nginx/t/js_headers.t
+++ b/nginx/t/js_headers.t
@@ -496,7 +496,7 @@ $t->write_file('test.js', <<EOF);
 
 EOF
 
-$t->try_run('no njs')->plan(51);
+$t->try_run('no njs')->plan(53);
 
 ###############################################################################
 
@@ -600,6 +600,19 @@ like(http(
        . 'Host: localhost' . CRLF . CRLF
 ), qr/foo: bar1,\s?bar2/, 'r.headersIn duplicate generic');
 
+like(http(
+       'GET /hdr_in HTTP/1.0' . CRLF
+       . 'Proxy-Connection: keep-alive' . CRLF
+       . 'Host: localhost' . CRLF . CRLF
+), qr/proxy-connection: keep-alive/, 'r.headersIn no slot header');
+
+like(http(
+       'GET /hdr_in HTTP/1.0' . CRLF
+       . 'Proxy-Connection: foo1' . CRLF
+       . 'Proxy-Connection: foo2' . CRLF
+       . 'Host: localhost' . CRLF . CRLF
+), qr/proxy-connection: foo1,\s?foo2/, 'r.headersIn no slot header duplicate');
+
 like(http_get('/in_lowkey'), qr/X{16}/, 'r.headersIn name is not overwritten');
 
 like(http(