From fc68e3670624660b7fdf89bc1b7316f5a267d7e0 Mon Sep 17 00:00:00 2001 From: "Oleg A. Mamontov" Date: Wed, 8 Jul 2020 19:44:02 +0300 Subject: [PATCH] ngx_http_cookie_flag_filter_handler() allocated not enough memory for "cookie_name". The strcat() call would write '\0' outside the allocated buffer. The current code also incorrectly matches any cookie whose name ends in "foo" if "set_cookie_flag foo ..." is specified. Both bugs fixed by rewriting the code that matches cookies by name. ngx_http_cookie_flag_filter_append() allocated not enough memory when editing cookie values. Generally, strings in nginx are not NUL-terminated, but there are some exceptions, including the values of request/response headers. While that assumption allows searching for substrings with ngx_strcasestrn(), the edited values were not NUL-terminated. This is fixed by allocating enough memory to have NUL-terminated strings. --- ngx_http_cookie_flag_filter_module.c | 117 ++++++++++++++++----------- 1 file changed, 71 insertions(+), 46 deletions(-) Patch-Source: https://github.com/AirisX/nginx_cookie_flag_module/pull/17 diff --git a/ngx_http_cookie_flag_filter_module.c b/ngx_http_cookie_flag_filter_module.c index b0316aa..63b8269 100644 --- a/ngx_http_cookie_flag_filter_module.c +++ b/ngx_http_cookie_flag_filter_module.c @@ -235,58 +235,89 @@ ngx_http_cookie_flag_filter_init(ngx_conf_t *cf) static ngx_int_t ngx_http_cookie_flag_filter_append(ngx_http_request_t *r, ngx_http_cookie_t *cookie, ngx_table_elt_t *header) { - ngx_str_t tmp; + u_char *data, *p; + size_t len; + ngx_http_cookie_t c; - if (cookie->httponly == 1 && ngx_strcasestrn(header->value.data, "; HttpOnly", 10 - 1) == NULL) { - tmp.data = ngx_pnalloc(r->pool, header->value.len + sizeof("; HttpOnly") - 1); - if (tmp.data == NULL) { - return NGX_ERROR; + c = *cookie; + len = 0; + + if (c.httponly) { + if (ngx_strcasestrn(header->value.data, "; HttpOnly", 10 - 1) == NULL) { + len += sizeof("; HttpOnly") - 1; + } else { + c.httponly = 0; } - tmp.len = ngx_sprintf(tmp.data, "%V; HttpOnly", &header->value) - tmp.data; - header->value.data = tmp.data; - header->value.len = tmp.len; } - if (cookie->secure == 1 && ngx_strcasestrn(header->value.data, "; secure", 8 - 1) == NULL) { - tmp.data = ngx_pnalloc(r->pool, header->value.len + sizeof("; secure") - 1); - if (tmp.data == NULL) { - return NGX_ERROR; + if (c.secure) { + if (ngx_strcasestrn(header->value.data, "; secure", 8 - 1) == NULL) { + len += sizeof("; secure") - 1; + } else { + c.secure = 0; } - tmp.len = ngx_sprintf(tmp.data, "%V; secure", &header->value) - tmp.data; - header->value.data = tmp.data; - header->value.len = tmp.len; } - if (cookie->samesite == 1 && ngx_strcasestrn(header->value.data, "; SameSite", 10 - 1) == NULL) { - tmp.data = ngx_pnalloc(r->pool, header->value.len + sizeof("; SameSite") - 1); - if (tmp.data == NULL) { - return NGX_ERROR; + if (c.samesite) { + if (ngx_strcasestrn(header->value.data, "; SameSite", 10 - 1) == NULL) { + len += sizeof("; SameSite") - 1; + } else { + c.samesite = 0; } - tmp.len = ngx_sprintf(tmp.data, "%V; SameSite", &header->value) - tmp.data; - header->value.data = tmp.data; - header->value.len = tmp.len; } - if (cookie->samesite_lax == 1 && ngx_strcasestrn(header->value.data, "; SameSite=Lax", 14 - 1) == NULL) { - tmp.data = ngx_pnalloc(r->pool, header->value.len + sizeof("; SameSite=Lax") - 1); - if (tmp.data == NULL) { - return NGX_ERROR; + if (c.samesite_lax) { + if (ngx_strcasestrn(header->value.data, "; SameSite=Lax", 14 - 1) == NULL) { + len += sizeof("; SameSite=Lax") - 1; + } else { + c.samesite_lax = 0; } - tmp.len = ngx_sprintf(tmp.data, "%V; SameSite=Lax", &header->value) - tmp.data; - header->value.data = tmp.data; - header->value.len = tmp.len; } - if (cookie->samesite_strict == 1 && ngx_strcasestrn(header->value.data, "; SameSite=Strict", 17 - 1) == NULL) { - tmp.data = ngx_pnalloc(r->pool, header->value.len + sizeof("; SameSite=Strict") - 1); - if (tmp.data == NULL) { - return NGX_ERROR; + if (c.samesite_strict) { + if (ngx_strcasestrn(header->value.data, "; SameSite=Strict", 17 - 1) == NULL) { + len += sizeof("; SameSite=Strict") - 1; + } else { + c.samesite_strict = 0; } - tmp.len = ngx_sprintf(tmp.data, "%V; SameSite=Strict", &header->value) - tmp.data; - header->value.data = tmp.data; - header->value.len = tmp.len; } + if (len == 0) { + return NGX_OK; + } + + data = ngx_pnalloc(r->pool, header->value.len + len + 1); + if (data == NULL) { + return NGX_ERROR; + } + + p = ngx_sprintf(data, "%V", &header->value); + + if (c.httponly) { + p = ngx_sprintf(p, "; HttpOnly"); + } + + if (c.secure) { + p = ngx_sprintf(p, "; secure"); + } + + if (c.samesite) { + p = ngx_sprintf(p, "; SameSite"); + } + + if (c.samesite_lax) { + p = ngx_sprintf(p, "; SameSite=Lax"); + } + + if (c.samesite_strict) { + p = ngx_sprintf(p, "; SameSite=Strict"); + } + + *p = '\0'; + + header->value.data = data; + header->value.len = p - data; + return NGX_OK; } @@ -332,17 +363,11 @@ ngx_http_cookie_flag_filter_handler(ngx_http_request_t *r) // for each security cookie we check whether preset it within Set-Cookie value. If not then we append. for (j = 0; j < flcf->cookies->nelts; j++) { - if (ngx_strncasecmp(cookie[j].cookie_name.data, (u_char *) "*", 1) != 0) { - // append "=" to the security cookie name. The result will be something like "cookie_name=" - char *cookie_name = ngx_pnalloc(r->pool, sizeof("=") - 1 + cookie[j].cookie_name.len); - if (cookie_name == NULL) { - return NGX_ERROR; - } - strcpy(cookie_name, (char *) cookie[j].cookie_name.data); - strcat(cookie_name, "="); + if (cookie[j].cookie_name.len != 1 || cookie[j].cookie_name.data[0] != '*') { + ngx_str_t *cookie_name = &cookie[j].cookie_name; // if Set-Cookie contains a cookie from settings - if (ngx_strcasestrn(header[i].value.data, cookie_name, strlen(cookie_name) - 1) != NULL) { + if (header[i].value.len > cookie_name->len && header[i].value.data[cookie_name->len] == '=' && ngx_strncasecmp(header[i].value.data, cookie_name->data, cookie_name->len) == 0) { ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "filter http_cookie_flag - add flags for cookie \"%V\"", &cookie[j].cookie_name); ngx_int_t res = ngx_http_cookie_flag_filter_append(r, &cookie[j], &header[i]); if (res != NGX_OK) { @@ -350,7 +375,7 @@ ngx_http_cookie_flag_filter_handler(ngx_http_request_t *r) } break; // otherwise default value will be added } - } else if (ngx_strncasecmp(cookie[j].cookie_name.data, (u_char *) "*", 1) == 0) { + } else { ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "filter http_cookie_flag - add default cookie flags"); ngx_int_t res = ngx_http_cookie_flag_filter_append(r, &cookie[j], &header[i]); if (res != NGX_OK) {