[PATCH] Avoiding mixed socket families in PROXY protocol v1 (ticket #2594)

[J Carter]

Hello Roman,

On Wed, 21 Feb 2024 17:29:52 +0400
Roman Arutyunyan <arut at nginx.com> wrote:

> Hi,
> 

[...]

> Checking whether the address used in PROXY writer is in fact the address
> that was passed in the PROXY header, is complicated.  This will either require
> setting a flag when PROXY address is set by realip, which is ugly.
> Another approach is checking if the client address written to a PROXY header
> matches the client address in the received PROXY header.  However since
> currently PROXY protocol addresses are stored as text, and not all addresses
> have unique text repersentations, this approach would require refactoring all
> PROXY protocol code + realip modules to switch from text to sockaddr.
> 
> I suggest that we follow the first plan (INADDR_ANY etc).
> 
> > [...]
> 
> Updated patch attached.
> 
> --
> Roman Arutyunyan

> diff --git a/src/core/ngx_proxy_protocol.c b/src/core/ngx_proxy_protocol.c
> --- a/src/core/ngx_proxy_protocol.c
> +++ b/src/core/ngx_proxy_protocol.c
> @@ -279,7 +279,10 @@ ngx_proxy_protocol_read_port(u_char *p, 
>  u_char *
>  ngx_proxy_protocol_write(ngx_connection_t *c, u_char *buf, u_char *last)
>  {
> -    ngx_uint_t  port, lport;
> +    socklen_t               local_socklen;
> +    ngx_uint_t              port, lport;
> +    struct sockaddr        *local_sockaddr;
> +    static ngx_sockaddr_t   default_sockaddr;

I understand you are using the fact static variables are zero 
initalized - to be both INADDR_ANY and "IN6ADDR_ANY", however is
this defined behavior for a union (specifically for ipv6 case) ?

I was under the impression only the first declared member, along with
padding bits were garunteed to be zero'ed.

https://stackoverflow.com/questions/54160137/what-constitutes-as-padding-in-a-union

>  
>      if (last - buf < NGX_PROXY_PROTOCOL_V1_MAX_HEADER) {
>          ngx_log_error(NGX_LOG_ALERT, c->log, 0,
> @@ -312,11 +315,21 @@ ngx_proxy_protocol_write(ngx_connection_
>  
>      *buf++ = ' ';
>  
> -    buf += ngx_sock_ntop(c->local_sockaddr, c->local_socklen, buf, last - buf,
> -                         0);
> +    if (c->sockaddr->sa_family == c->local_sockaddr->sa_family) {
> +        local_sockaddr = c->local_sockaddr;
> +        local_socklen = c->local_socklen;
> +
> +    } else {
> +        default_sockaddr.sockaddr.sa_family = c->sockaddr->sa_family;
> +
> +        local_sockaddr = &default_sockaddr.sockaddr;
> +        local_socklen = sizeof(ngx_sockaddr_t);
> +    }
> +
> +    buf += ngx_sock_ntop(local_sockaddr, local_socklen, buf, last - buf, 0);
>  
>      port = ngx_inet_get_port(c->sockaddr);
> -    lport = ngx_inet_get_port(c->local_sockaddr);
> +    lport = ngx_inet_get_port(local_sockaddr);
>  
>      return ngx_slprintf(buf, last, " %ui %ui" CRLF, port, lport);
>  }
>