Hello!
On Fri, Oct 14, 2022 at 04:33:00PM +0400, Sergey Kandaurov wrote:
> > On 14 Oct 2022, at 00:30, Maxim Dounin <mdou...@mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Thu, Oct 13, 2022 at 05:02:42PM +0400, Sergey Kandaurov wrote:
> >
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluk...@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 <pluk...@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.
Feel free to commit the variant you prefer.
--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
