Hello!

On Fri, Oct 27, 2023 at 02:58:44PM +0300, Vladimir Homutov via nginx-devel wrote:

> Patch subject is complete summary.
>
>
> src/core/ngx_cycle.c | 10 ++++++----
> src/core/ngx_resolver.c | 2 +-
> src/core/ngx_string.c | 15 +++++++++++++++
> src/http/modules/ngx_http_proxy_module.c | 4 ++--
> src/http/ngx_http_file_cache.c | 4 +++-
> src/http/ngx_http_variables.c | 3 +++
> src/mail/ngx_mail_auth_http_module.c | 12 +++++++++---
> src/stream/ngx_stream_script.c | 4 +++-
> 8 files changed, 42 insertions(+), 12 deletions(-)
>
>

> # HG changeset patch
> # User Vladimir Khomutov <vl@wbsrv.ru>
> # Date 1698407658 -10800
> # Fri Oct 27 14:54:18 2023 +0300
> # Node ID ef9f124b156aff0e9f66057e438af835bd7a60d2
> # Parent ea1f29c2010cda4940b741976f103d547308815a
> Core: avoid calling memcpy() in edge cases.
>
> diff --git a/src/core/ngx_cycle.c b/src/core/ngx_cycle.c
> --- a/src/core/ngx_cycle.c
> +++ b/src/core/ngx_cycle.c
> @@ -115,10 +115,12 @@ ngx_init_cycle(ngx_cycle_t *old_cycle)
> old_cycle->conf_file.len + 1);
>
> cycle->conf_param.len = old_cycle->conf_param.len;
> - cycle->conf_param.data = ngx_pstrdup(pool, &old_cycle->conf_param);
> - if (cycle->conf_param.data == NULL) {
> - ngx_destroy_pool(pool);
> - return NULL;
> + if (cycle->conf_param.len) {
> + cycle->conf_param.data = ngx_pstrdup(pool, &old_cycle->conf_param);
> + if (cycle->conf_param.data == NULL) {
> + ngx_destroy_pool(pool);
> + return NULL;
> + }
> }
>
>
> diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
> --- a/src/core/ngx_resolver.c
> +++ b/src/core/ngx_resolver.c
> @@ -4206,7 +4206,7 @@ ngx_resolver_dup(ngx_resolver_t *r, void
>
> dst = ngx_resolver_alloc(r, size);
>
> - if (dst == NULL) {
> + if (dst == NULL || size == 0 || src == NULL) {
> return dst;
> }
>

This looks simply wrong: ngx_resolver_dup() with src == NULL and
with size != 0 should dereference the NULL pointer and segfault.

Also, I can't say I'm happy with allocation error handling mixed
with (size == 0) special case handling.

> diff --git a/src/core/ngx_string.c b/src/core/ngx_string.c
> --- a/src/core/ngx_string.c
> +++ b/src/core/ngx_string.c
> @@ -252,6 +252,11 @@ ngx_vslprintf(u_char *buf, u_char *last,
> case 'V':
> v = va_arg(args, ngx_str_t *);
>
> + if (v->len == 0 || v->data == NULL) {
> + fmt++;
> + continue;
> + }
> +
> buf = ngx_sprintf_str(buf, last, v->data, v->len, hex);
> fmt++;
>
> @@ -260,6 +265,11 @@ ngx_vslprintf(u_char *buf, u_char *last,
> case 'v':
> vv = va_arg(args, ngx_variable_value_t *);
>
> + if (vv->len == 0 || vv->data == NULL) {
> + fmt++;
> + continue;
> + }
> +
> buf = ngx_sprintf_str(buf, last, vv->data, vv->len, hex);
> fmt++;
>
> @@ -268,6 +278,11 @@ ngx_vslprintf(u_char *buf, u_char *last,
> case 's':
> p = va_arg(args, u_char *);
>
> + if (slen == 0 || p == NULL) {
> + fmt++;
> + continue;
> + }
> +
> buf = ngx_sprintf_str(buf, last, p, slen, hex);
> fmt++;
>

I tend to think that these should be handled in ngx_sprintf_str()
or in ngx_cpymem(), if at all, see below.

> diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c
> --- a/src/http/modules/ngx_http_proxy_module.c
> +++ b/src/http/modules/ngx_http_proxy_module.c
> @@ -1205,7 +1205,7 @@ ngx_http_proxy_create_key(ngx_http_reque
>
> key->data = p;
>
> - if (r->valid_location) {
> + if (r->valid_location && ctx->vars.uri.len) {
> p = ngx_copy(p, ctx->vars.uri.data, ctx->vars.uri.len);
> }
>
> @@ -1422,7 +1422,7 @@ ngx_http_proxy_create_request(ngx_http_r
> b->last = ngx_copy(b->last, r->unparsed_uri.data, r->unparsed_uri.len);
>
> } else {
> - if (r->valid_location) {
> + if (r->valid_location && ctx->vars.uri.len) {
> b->last = ngx_copy(b->last, ctx->vars.uri.data, ctx->vars.uri.len);
> }
>
> diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
> --- a/src/http/ngx_http_file_cache.c
> +++ b/src/http/ngx_http_file_cache.c
> @@ -1270,7 +1270,9 @@ ngx_http_file_cache_set_header(ngx_http_
>
> if (c->etag.len <= NGX_HTTP_CACHE_ETAG_LEN) {
> h->etag_len = (u_char) c->etag.len;
> - ngx_memcpy(h->etag, c->etag.data, c->etag.len);
> + if (c->etag.len) {
> + ngx_memcpy(h->etag, c->etag.data, c->etag.len);
> + }
> }
>
> if (c->vary.len) {
> diff --git a/src/http/ngx_http_variables.c b/src/http/ngx_http_variables.c
> --- a/src/http/ngx_http_variables.c
> +++ b/src/http/ngx_http_variables.c
> @@ -2157,6 +2157,9 @@ ngx_http_variable_request_body(ngx_http_
>
> for ( /* void */ ; cl; cl = cl->next) {
> buf = cl->buf;
> + if (buf->last == buf->pos) {
> + continue;
> + }
> p = ngx_cpymem(p, buf->pos, buf->last - buf->pos);
> }
>
> diff --git a/src/mail/ngx_mail_auth_http_module.c b/src/mail/ngx_mail_auth_http_module.c
> --- a/src/mail/ngx_mail_auth_http_module.c
> +++ b/src/mail/ngx_mail_auth_http_module.c
> @@ -1314,11 +1314,15 @@ ngx_mail_auth_http_create_request(ngx_ma
> *b->last++ = CR; *b->last++ = LF;
>
> b->last = ngx_cpymem(b->last, "Auth-User: ", sizeof("Auth-User: ") - 1);
> - b->last = ngx_copy(b->last, login.data, login.len);
> + if (login.len) {
> + b->last = ngx_copy(b->last, login.data, login.len);
> + }
> *b->last++ = CR; *b->last++ = LF;
>
> b->last = ngx_cpymem(b->last, "Auth-Pass: ", sizeof("Auth-Pass: ") - 1);
> - b->last = ngx_copy(b->last, passwd.data, passwd.len);
> + if (passwd.len) {
> + b->last = ngx_copy(b->last, passwd.data, passwd.len);
> + }
> *b->last++ = CR; *b->last++ = LF;
>
> if (s->auth_method != NGX_MAIL_AUTH_PLAIN && s->salt.len) {
> @@ -1375,7 +1379,9 @@ ngx_mail_auth_http_create_request(ngx_ma
>
> b->last = ngx_cpymem(b->last, "Auth-SMTP-Helo: ",
> sizeof("Auth-SMTP-Helo: ") - 1);
> - b->last = ngx_copy(b->last, s->smtp_helo.data, s->smtp_helo.len);
> + if (s->smtp_helo.len) {
> + b->last = ngx_copy(b->last, s->smtp_helo.data, s->smtp_helo.len);
> + }
> *b->last++ = CR; *b->last++ = LF;
>
> b->last = ngx_cpymem(b->last, "Auth-SMTP-From: ",

If at all, these probably should check for login.len, passwd.len,
and s->smtp_helo.len, similarly to other cases nearby, and avoid
sending the headers altogether.

> diff --git a/src/stream/ngx_stream_script.c b/src/stream/ngx_stream_script.c
> --- a/src/stream/ngx_stream_script.c
> +++ b/src/stream/ngx_stream_script.c
> @@ -842,7 +842,9 @@ ngx_stream_script_copy_var_code(ngx_stre
>
> if (value && !value->not_found) {
> p = e->pos;
> - e->pos = ngx_copy(p, value->data, value->len);
> + if (value->len) {
> + e->pos = ngx_copy(p, value->data, value->len);
> + }
>
> ngx_log_debug2(NGX_LOG_DEBUG_STREAM,
> e->session->connection->log, 0,

Obviously enough, there should be a corresponding change in
ngx_http_script_copy_var_code().

Overall, similarly to the other patch in the series, I'm highly
sceptical about doing such scattered changes based on the UB
sanitizer reports from some test runs. Rather, we should use UB
sanitizer reports to identify problematic patterns, and fix these
patterns all other the code (if at all).

Further, in this particular case I tend to think that the problem
is not with nginx code, but rather with the memcpy() interface UB
sanitizer tries to enforce. It should be completely safe to call
memcpy(p, NULL, 0), and if it doesn't, we might consider adding
appropriate guards at interface level, such as in ngx_memcpy() /
ngx_cpymem() wrappers, and not in each call. Trying to check
length everywhere is just ugly and unreadable.

Also, while recent versions of gcc are known to miscompile some
code which uses memcpy(p, NULL, 0) (see [1], with "-O2" or with
"-O1 -ftree-vrp" optimization flags in my tests), I don't think
this affects nginx code. If it does, we might also consider
force-switching off relevant optimizations (if enabled, as we use
"-O1" by default).

[1] https://stackoverflow.com/questions/5243012/is-it-guaranteed-to-be-safe-to-perform-memcpy0-0-0

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel