[PATCH] SSL: improved validation of ssl_session_cache and ssl_ocsp_cache

[Sergey Kandaurov]

> On 14 Oct 2022, at 20:30, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> 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.

Yep, committed this variant.
Thanks for review.

-- 
Sergey Kandaurov