[PATCH] SSL: improved validation of ssl_session_cache and ssl_ocsp_cache

[Maxim Dounin]

Hello!

On Fri, Oct 14, 2022 at 04:33:00PM +0400, Sergey Kandaurov wrote:

> > On 14 Oct 2022, at 00:30, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > 
> > 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.
> 
> Agree.
> 
> > 
> >>             }
> >> 
> >> -            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.
> > 
> 
> Thanks, that's certainly an omission.
> 
> 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
> @@ -1088,13 +1088,15 @@ ngx_http_ssl_session_cache(ngx_conf_t *c
>              p = (u_char *) ngx_strchr(name.data, ':');
>  
>              if (p == NULL) {
> -                ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> -                                   "invalid zone size \"%V\"", &value[i]);
> -                return NGX_CONF_ERROR;
> +                goto invalid;
>              }
>  
>              name.len = p - name.data;
>  
> +            if (name.len == 0) {
> +                goto invalid;
> +            }
> +
>              size.data = p + 1;
>              size.len = value[i].data + value[i].len - size.data;
>  
> 
> (with intention to update other places.)

Looks good.

> > 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.
> 
> Indeed, this is to bring similarity in parsing,
> that's why it comes with such a huge diff.
> 
> Alternatively (my initial version), is to add a simple check.
> Given that the resulting code has subtle differences comparing to
> limit_req/limit_conn, I tend to think it has little sense to unify.
> That said, below is a different approach:

I generally tend to think that ngx_strchr() approach as used in 
limit_req_zone / limit_conn_zone is more readable compared to the 
explicit for loop in ssl_session_cache.  But the idea of changing 
the existing code is indeed questionable.

> 
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1665749669 -14400
> #      Fri Oct 14 16:14:29 2022 +0400
> # Node ID 68bc1f8b35a9709a2b8bef6c2d60b33ac7c2712b
> # 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
> @@ -1097,6 +1097,10 @@ ngx_http_ssl_session_cache(ngx_conf_t *c
>                  goto invalid;
>              }
>  
> +            if (j == value[i].len) {
> +                goto invalid;
> +            }
> +
>              name.len = len;
>              name.data = value[i].data + sizeof("shared:") - 1;
>  

May be just

@@ -1093,7 +1093,7 @@ ngx_http_ssl_session_cache(ngx_conf_t *c
                 len++;
             }
 
-            if (len == 0) {
+            if (len == 0 || j == value[i].len) {
                 goto invalid;
             }
 
?

Either way, looks good.

Feel free to commit the variant you prefer.

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