[PATCH] SSL: improved validation of ssl_session_cache and ssl_ocsp_cache

[Maxim Dounin]

Hello!

On Thu, Oct 13, 2022 at 05:02:42PM +0400, Sergey Kandaurov wrote:

> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1665665717 -14400
> #      Thu Oct 13 16:55:17 2022 +0400
> # Node ID b2eba2994ddcbf9084075f9ae32c3332a628ca7a
> # Parent  81b4326daac70d6de70abbc3fe36d4f6e3da54a2
> SSL: improved validation of ssl_session_cache and ssl_ocsp_cache.
> 
> Now it properly detects invalid shared zone configuration with omitted size.
> Previously it used to read outside of the buffer boundary.
> 
> Found with AddressSanitizer.
> 
> diff --git a/src/http/modules/ngx_http_ssl_module.c b/src/http/modules/ngx_http_ssl_module.c
> --- a/src/http/modules/ngx_http_ssl_module.c
> +++ b/src/http/modules/ngx_http_ssl_module.c
> @@ -1039,10 +1039,10 @@ ngx_http_ssl_session_cache(ngx_conf_t *c
>  {
>      ngx_http_ssl_srv_conf_t *sscf = conf;
>  
> -    size_t       len;
> +    u_char      *p;
>      ngx_str_t   *value, name, size;
>      ngx_int_t    n;
> -    ngx_uint_t   i, j;
> +    ngx_uint_t   i;
>  
>      value = cf->args->elts;
>  
> @@ -1083,25 +1083,20 @@ ngx_http_ssl_session_cache(ngx_conf_t *c
>              && ngx_strncmp(value[i].data, "shared:", sizeof("shared:") - 1)
>                 == 0)
>          {
> -            len = 0;
> +            name.data = value[i].data + sizeof("shared:") - 1;
> +
> +            p = (u_char *) ngx_strchr(name.data, ':');
>  
> -            for (j = sizeof("shared:") - 1; j < value[i].len; j++) {
> -                if (value[i].data[j] == ':') {
> -                    break;
> -                }
> -
> -                len++;
> +            if (p == NULL) {
> +                ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                                   "invalid zone size \"%V\"", &value[i]);
> +                return NGX_CONF_ERROR;

goto invalid?

This seems to be more in line with both previous handling of the 
"len == 0" case, and the remaining handling of the "n == 
NGX_ERROR" case.

>              }
>  
> -            if (len == 0) {
> -                goto invalid;
> -            }
> +            name.len = p - name.data;

This makes it possible to create a shared memory zone with an 
empty name, which was previously forbidden.

Note that limit_req_zone / limit_conn_zone parsing you've copied 
does not allow shared memory zones with empty names due to the 
additional name.len check after parsing of all arguments.

While I don't think that empty names are fatal, they are certainly 
confusing at least in logs, and it might be a good idea to preserve 
the name length check.

>  
> -            name.len = len;
> -            name.data = value[i].data + sizeof("shared:") - 1;
> -
> -            size.len = value[i].len - j - 1;
> -            size.data = name.data + len + 1;
> +            size.data = p + 1;
> +            size.len = value[i].data + value[i].len - size.data;
>  
>              n = ngx_parse_size(&size);
>  

[...]

-- 
Maxim Dounin
http://mdounin.ru/