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.

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

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.

While I don't think that empty names are fatal, they are certainly
confusing at least in logs, and it might be a good idea to preserve
the name length check.

>
> - name.len = len;
> - name.data = value[i].data + sizeof("shared:") - 1;
> -
> - size.len = value[i].len - j - 1;
> - size.data = name.data + len + 1;
> + size.data = p + 1;
> + size.len = value[i].data + value[i].len - size.data;
>
> n = ngx_parse_size(&size);
>

[...]

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org