> On 14 Oct 2022, at 20:30, Maxim Dounin <mdounin@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@mdounin.ru> wrote:
>>>
>>> Hello!
>>>
>>> On Thu, Oct 13, 2022 at 05:02:42PM +0400, Sergey Kandaurov wrote:
>>>
>>>> # HG changeset patch
>>>> # User Sergey Kandaurov <pluknet@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@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

_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org