Hi,

On Thu, Feb 22, 2024 at 01:59:25AM +0000, J Carter wrote:
> Hello Roman,
>
> On Wed, 21 Feb 2024 17:29:52 +0400
> Roman Arutyunyan <arut@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

It's not clear what exactly is meant by padding in that particular statement.
It may as well be everything beyong the first member. However C99 does not
require zeroing out the padding anyway.

I can hardly believe there's a platform where the second union member may
in fact be non-zero in this case. The entire union is allocated in the BSS
segment and is not even present in the binary file.

The reasons why C standard has this grey area are clear. Zeroing out a chunk
of memory may not be equivalent to initializing particular members to NULL, 0.0
etc on certain platforms. However nginx already heavily relies on ngx_memzero()
for initializing most structures. For platforms where this works, there are
no reasons why a static union would not be allocated in a BSS segment.

However it's not hard to avoid the issue completely, just to be on the safe
side. Thanks for noticing this.

> > 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);
> > }
> >
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Roman Arutyunyan
# HG changeset patch
# User Roman Arutyunyan <arut@nginx.com>
# Date 1708522464 -14400
# Wed Feb 21 17:34:24 2024 +0400
# Node ID 2d9bb7b49d64576fa29a673133129f16de3cfbbe
# Parent 44da04c2d4db94ad4eefa84b299e07c5fa4a00b9
Avoiding mixed socket families in PROXY protocol v1 (ticket #2010).

When using realip module, remote and local addresses of a connection can belong
to different address families. This previously resulted in generating PROXY
protocol headers like this:

PROXY TCP4 127.0.0.1 unix:/tmp/nginx1.sock 55544 0

The PROXY protocol v1 specification does not allow mixed families. The change
substitutes server address with zero address in this case:

PROXY TCP4 127.0.0.1 0.0.0.0 55544 0

As an alternative, "PROXY UNKNOWN" header could be used, which unlike this
header does not contain any useful information about the client.

Also, the above mentioned format for unix socket address is not specified in
PROXY protocol v1 and is a by-product of internal nginx representation of it.
The change eliminates such addresses from PROXY protocol headers as well.

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,13 @@ 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;
+ struct sockaddr_in sin;
+#if (NGX_HAVE_INET6)
+ struct sockaddr_in6 sin6;
+#endif

if (last - buf < NGX_PROXY_PROTOCOL_V1_MAX_HEADER) {
ngx_log_error(NGX_LOG_ALERT, c->log, 0,
@@ -312,11 +318,35 @@ 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 {
+ switch (c->sockaddr->sa_family) {
+
+#if (NGX_HAVE_INET6)
+ case AF_INET6:
+ ngx_memzero(&sin6, sizeof(struct sockaddr_in6));
+ sin6.sin6_family = AF_INET6;
+ local_sockaddr = (struct sockaddr *) &sin6;
+ local_socklen = sizeof(struct sockaddr_in6);
+ break;
+#endif
+
+ default: /* AF_INET */
+ ngx_memzero(&sin, sizeof(struct sockaddr));
+ sin.sin_family = AF_INET;
+ local_sockaddr = (struct sockaddr *) &sin;
+ local_socklen = sizeof(struct sockaddr_in);
+ break;
+ }
+ }
+
+ 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);
}
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel