> 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.)

> 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:

# 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;

@@ -1187,6 +1191,10 @@ ngx_http_ssl_ocsp_cache(ngx_conf_t *cf,
goto invalid;
}

+ if (j == value[1].len) {
+ goto invalid;
+ }
+
name.len = len;
name.data = value[1].data + sizeof("shared:") - 1;

diff --git a/src/mail/ngx_mail_ssl_module.c b/src/mail/ngx_mail_ssl_module.c
--- a/src/mail/ngx_mail_ssl_module.c
+++ b/src/mail/ngx_mail_ssl_module.c
@@ -686,6 +686,10 @@ ngx_mail_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;

diff --git a/src/stream/ngx_stream_ssl_module.c b/src/stream/ngx_stream_ssl_module.c
--- a/src/stream/ngx_stream_ssl_module.c
+++ b/src/stream/ngx_stream_ssl_module.c
@@ -1077,6 +1077,10 @@ ngx_stream_ssl_session_cache(ngx_conf_t
goto invalid;
}

+ if (j == value[i].len) {
+ goto invalid;
+ }
+
name.len = len;
name.data = value[i].data + sizeof("shared:") - 1;


--
Sergey Kandaurov

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