[PATCH] SSL: improved validation of ssl_session_cache and ssl_ocsp_cache

[Sergey Kandaurov]

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

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