Opened 3 years ago

Closed 3 years ago

#2239 closed defect (invalid)

Unterminated string result in ngx_sock_ntop()

Reported by: GregIthaca@… Owned by:
Priority: minor Milestone:
Component: nginx-core Version: 1.19.x
Keywords: CWE-170, CWE-200 Cc:
uname -a: Linux invulnaco 5.4.0-72-generic #80~18.04.1-Ubuntu SMP Mon Apr 12 23:26:25 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.2.9
built by gcc 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)
configure arguments:

Description

I've been studying/running your code for security research, and I've encountered what appears to be a unterminated string condition that has some risk of exposing heap internals. I've verified that as far as I can tell, this issue remains in the latest source code.

In https://github.com/nginx/nginx/blob/branches/stable-1.20/src/event/ngx_event_accept.c#L272 there is a call to ngx_soc_ntop() with the ls->addr_text_max_len argument. In the case of INET_ADDR this value is 15 per https://github.com/nginx/nginx/blob/branches/stable-1.20/src/core/ngx_inet.h#L16

However, any time the length matches or exceeds this, as is the case when a port number is specified or for an address such as 128.100.100.100, the snprintf() operation (e.g. https://github.com/nginx/nginx/blob/branches/stable-1.20/src/core/ngx_inet.c#L206) will leave the resulting text unterminated. Because this is allocated from the heap, residual data in the surrounding block might be exposed.

Trivial fixes would include adjusting the size of the allocated buffer to account for this.

Change History (1)

comment:1 by Maxim Dounin, 3 years ago

Resolution: invalid
Status: newclosed

... will leave the resulting text unterminated.

In nginx, strings are not generally null-terminated, and null-termination is added only when needed. In this particular case null-termination is not expected and never happens: note that ngx_snprintf() as used in the function never null-terminates strings by itself.

Because this is allocated from the heap, residual data in the surrounding block might be exposed.

If you think there are cases where this might happen, that is, cases where c->addr_text.data is used in an assumption it is a null-terminated string instead of using c->addr_text.len, don't hesitate to report this. We are not aware of any such cases in the code.

Note: See TracTickets for help on using tickets.