Unterminated string result in ngx_sock_ntop()

[GregIthaca@…]

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.

[Maxim Dounin]

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