Welcome! Log In Create A New Profile

Advanced

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

Roman Arutyunyan
March 11, 2024 08:46AM
Hi,

On Wed, Mar 06, 2024 at 06:50:26PM +0400, Sergey Kandaurov wrote:
> On Thu, Feb 22, 2024 at 07:17:26PM +0400, Roman Arutyunyan wrote:
> > 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,
> > > >
> > >
>
> > > > On Wed, Jan 24, 2024 at 12:03:06AM +0300, Maxim Dounin wrote:
> > > > > [..]
> > > > > Also, as suggested, using the server address as obtained via PROXY
> > > > > protocol from the client might be a better solution as long as the
> > > > > client address was set via PROXY protocol (regardless of whether
> > > > > address families match or not), and what users expect from the
> > > > > "proty_protocol on;" when chaining stream proxies in the first
> > > > > place.
> > >
> > > > 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.
>
> Checking for the realip context should do the trick, it is created
> once the client address was successfully substituted. We could then
> parse the text repr of server address in the PROXY header and use it
> in PROXY writer. This will allow not to break API.
> Something like this should work called from the stream proxy module,
> it allows to preserve an original server address in the PROXY header
> keeping both addresses unmutated
> (which seems to be the best way to solve this problem).
>
> : extern ngx_module_t ngx_stream_realip_module;
> :
> : if (ngx_stream_get_module_ctx(s, ngx_stream_realip_module)) {
> :
> : local_sockaddr = c->local_sockaddr;
> : local_socklen = c->local_socklen;
> :
> : if (ngx_parse_addr(c->pool, &addr, c->proxy_protocol->dst_addr.data,
> : c->proxy_protocol->dst_addr.len)
> : != NGX_OK)
> : {
> : ngx_stream_proxy_finalize(s, NGX_STREAM_INTERNAL_SERVER_ERROR);
> : return;
> : }
> :
> : ngx_inet_set_port(addr.sockaddr, c->proxy_protocol->dst_port);
> :
> : c->local_sockaddr = addr.sockaddr;
> : c->local_socklen = addr.socklen;
> : }
> :
> : # call ngx_proxy_protocol_write(), other existing stuff
> :
> : if (ngx_stream_get_module_ctx(s, ngx_stream_realip_module)) {
> : c->local_sockaddr = local_sockaddr;
> : c->local_socklen = local_socklen;
> : }
>
> Can't say I am excited from this code, though.

Neither am I.

> Another way is stick to the INADDR_ANY plan removing the server
> address part from the PROXY header if not applicable.
>
> > > >
> > > > I suggest that we follow the first plan (INADDR_ANY etc).
>
> I agree in general, but see below.
>
> > > >
> > > > > [...]
> > > >
> > > > Updated patch attached.
> > > >
>
> > # 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
>
> Well, UNIX-domain socket addresses are somewhat rather exsotic,
> especially in the PROXY protocol. I'd use an example with IPv4/IPv6.
>
> >
> > 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));
>
> Note that although wildcard adresses are typically implemented
> using all-zeroes, this is not obligated by standards and hence
> is an implementation detail. POSIX mandates just the following:
>
> : The <netinet/in.h> header shall define the following symbolic
> : constant for use as a local address in the structure passed to bind():
> :
> : INADDR_ANY
> : IPv4 wildcard address.
>
> (Note: it is "IPv4 local host address." in previous editions.)
>
> : The <netinet/in.h> header shall declare the following external
> : variable:
> :
> : const struct in6_addr in6addr_any
> :
> : This variable is initialized by the system to contain the wildcard
> : IPv6 address. The <netinet/in.h> header also defines the
> : IN6ADDR_ANY_INIT macro. This macro must be constant at compile time
> : and can be used to initialize a variable of type struct in6_addr to
> : the IPv6 wildcard address.
>
> : RATIONALE
> :
> : The INADDR_ANY and INADDR_BROADCAST values are byte-order-neutral
> : and thus their byte order is not specified. Many implementations
> : have additional constants as extensions, such as INADDR_LOOPBACK,
> : that are not byte-order-neutral. Traditionally, these constants are
> : in host byte order, requiring the use of htonl() when using them in
> : a sockaddr_in structure.
>
> So it seems quite legitimate to use a different binary representation.
> To be on the safe side (and improve code readability), it makes sense
> to consistently set wildcard addresses explicitly, using INADDR_ANY
> and in6addr_any. It is not so important in this patch though, because
> addresses do not cross the kernel, used solely for ngx_sock_ntop().
>
> > + 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);
> > }
>
> Picking INADDR_ANY (and IPv6 equivalent) to substitute non-applicable
> addresses due to mismatching address family may be unnecessarily
> strict hiding addresses that can be represented in a different family.
>
> IPv4 address can be represented in the IPv4-mapped IPv6 address format.
> IPv6 address can be represented in the IPv4 format if it's IPv4-mapped.

What if it's not?

> Such format is supported in various nginx modules and may be enabled
> by "listen ... ipv6only=off" to accept IPv4-mapped IPv6 addresses on a
> wildcard listen socket.
>
> For example, if nginx receives "PROXY TCP6" on "listen 127.0.0.1:8081"
> and is configured to send PROXY protocol further in the stream proxy
> chain, the server address will be updated to "::ffff:127.0.0.1 8081".
> And opposite, receiving "PROXY TCP4" on "listen [::]:8082 ipv6only=off"
> will update the PROXY header server part to "127.0.0.1 8082".
>
> That said, something like this should work on top off your change:
>
> 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,12 +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)
> {
> - socklen_t local_socklen;
> - ngx_uint_t port, lport;
> - struct sockaddr *local_sockaddr;
> - struct sockaddr_in sin;
> + socklen_t local_socklen;
> + ngx_uint_t port, lport;
> + ngx_sockaddr_t sa;
> + struct sockaddr *local_sockaddr;
> #if (NGX_HAVE_INET6)
> - struct sockaddr_in6 sin6;
> + in_addr_t addr4;
> + struct in6_addr *addr6;
> #endif
>
> if (last - buf < NGX_PROXY_PROTOCOL_V1_MAX_HEADER) {
> @@ -323,22 +324,62 @@ ngx_proxy_protocol_write(ngx_connection_
> local_socklen = c->local_socklen;
>
> } else {
> + local_sockaddr = &sa.sockaddr;
> +
> + ngx_memzero(&sa, sizeof(ngx_sockaddr_t));
> + sa.sockaddr.sa_family = c->sockaddr->sa_family;
> + ngx_inet_set_port(&sa.sockaddr, ngx_inet_get_port(c->local_sockaddr));
> +
> 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);
> +
> + if (c->local_sockaddr->sa_family == AF_INET) {
> + addr4 = ntohl(((struct sockaddr_in *) c->local_sockaddr)
> + ->sin_addr.s_addr);
> +
> + addr6 = &sa.sockaddr_in6.sin6_addr;
> + addr6->s6_addr[10] = 0xff;
> + addr6->s6_addr[11] = 0xff;
> + addr6->s6_addr[12] = addr4 >> 24;
> + addr6->s6_addr[13] = addr4 >> 16;
> + addr6->s6_addr[14] = addr4 >> 8;
> + addr6->s6_addr[15] = addr4;
> +
> + } else {
> + sa.sockaddr_in6.sin6_addr = in6addr_any;
> + }
> +
> 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);
> +
> +#if (NGX_HAVE_INET6)
> + if (c->local_sockaddr->sa_family == AF_INET6) {
> + addr6 = &((struct sockaddr_in6 *) c->local_sockaddr)->sin6_addr;
> +
> + if (IN6_IS_ADDR_V4MAPPED(addr6)) {
> + addr4 = addr6->s6_addr[12] << 24;
> + addr4 += addr6->s6_addr[13] << 16;
> + addr4 += addr6->s6_addr[14] << 8;
> + addr4 += addr6->s6_addr[15];
> +
> + sa.sockaddr_in.sin_addr.s_addr = htonl(addr4);
> +
> + } else {
> + sa.sockaddr_in.sin_addr.s_addr = INADDR_ANY;
> + }
> +
> + } else
> +#endif
> + {
> + sa.sockaddr_in.sin_addr.s_addr = INADDR_ANY;
> + }
> +
> break;
> }
> }

I agree we can implement a few more workarounds and make it work.
It all looks like we need a normal solution even if the patch will be bigger.
Attached is a patch converting PROXY protocol addresses from text to binary
representation. IMHO we should've done this long ago. Once it's done,
the mixed family patch will be trivial, attached as well.

--
Roman Arutyunyan
# HG changeset patch
# User Roman Arutyunyan <arut@nginx.com>
# Date 1709913161 -14400
# Fri Mar 08 19:52:41 2024 +0400
# Node ID 07c8c445746c6c74250bad7b42cd26dbb0ed6dd3
# Parent 2ed3f57dca0a664340bca2236c7d614902db4180
Storing PROXY protocol addresses as sockaddr.

Previously they were stored as text. This was beneficial for v1 where the
addresses do arrive as text, however counterproductive for v2, where they
arrive as binary.

The change allows more flexibility when checking whether PROXY protocol
address was set as client address. This will be needed in the followup changes.

Also, PROXY protocol debugging is improved. Now TLVs are logged after parsing.

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
@@ -67,9 +67,12 @@ typedef struct {


static u_char *ngx_proxy_protocol_read_addr(ngx_connection_t *c, u_char *p,
- u_char *last, ngx_str_t *addr);
+ u_char *last, ngx_addr_t *addr);
static u_char *ngx_proxy_protocol_read_port(u_char *p, u_char *last,
- in_port_t *port, u_char sep);
+ struct sockaddr *sa, u_char sep);
+#if (NGX_DEBUG)
+static void ngx_proxy_protocol_debug(ngx_connection_t *c, ngx_uint_t version);
+#endif
static u_char *ngx_proxy_protocol_v2_read(ngx_connection_t *c, u_char *buf,
u_char *last);
static ngx_int_t ngx_proxy_protocol_lookup_tlv(ngx_connection_t *c,
@@ -101,6 +104,7 @@ ngx_proxy_protocol_read(ngx_connection_t
{
size_t len;
u_char *p;
+ ngx_addr_t addr;
ngx_proxy_protocol_t *pp;

static const u_char signature[] = "\r\n\r\n\0\r\nQUIT\n";
@@ -141,22 +145,28 @@ ngx_proxy_protocol_read(ngx_connection_t
return NULL;
}

- p = ngx_proxy_protocol_read_addr(c, p, last, &pp->src_addr);
+ p = ngx_proxy_protocol_read_addr(c, p, last, &addr);
if (p == NULL) {
goto invalid;
}

- p = ngx_proxy_protocol_read_addr(c, p, last, &pp->dst_addr);
+ pp->src_sockaddr = addr.sockaddr;
+ pp->src_socklen = addr.socklen;
+
+ p = ngx_proxy_protocol_read_addr(c, p, last, &addr);
if (p == NULL) {
goto invalid;
}

- p = ngx_proxy_protocol_read_port(p, last, &pp->src_port, ' ');
+ pp->dst_sockaddr = addr.sockaddr;
+ pp->dst_socklen = addr.socklen;
+
+ p = ngx_proxy_protocol_read_port(p, last, pp->src_sockaddr, ' ');
if (p == NULL) {
goto invalid;
}

- p = ngx_proxy_protocol_read_port(p, last, &pp->dst_port, CR);
+ p = ngx_proxy_protocol_read_port(p, last, pp->dst_sockaddr, CR);
if (p == NULL) {
goto invalid;
}
@@ -169,11 +179,11 @@ ngx_proxy_protocol_read(ngx_connection_t
goto invalid;
}

- ngx_log_debug4(NGX_LOG_DEBUG_CORE, c->log, 0,
- "PROXY protocol src: %V %d, dst: %V %d",
- &pp->src_addr, pp->src_port, &pp->dst_addr, pp->dst_port);
+ c->proxy_protocol = pp;

- c->proxy_protocol = pp;
+#if (NGX_DEBUG)
+ ngx_proxy_protocol_debug(c, 1);
+#endif

return p;

@@ -202,7 +212,7 @@ invalid:

static u_char *
ngx_proxy_protocol_read_addr(ngx_connection_t *c, u_char *p, u_char *last,
- ngx_str_t *addr)
+ ngx_addr_t *addr)
{
size_t len;
u_char ch, *pos;
@@ -231,20 +241,16 @@ ngx_proxy_protocol_read_addr(ngx_connect

len = p - pos - 1;

- addr->data = ngx_pnalloc(c->pool, len);
- if (addr->data == NULL) {
+ if (ngx_parse_addr(c->pool, addr, pos, len) != NGX_OK) {
return NULL;
}

- ngx_memcpy(addr->data, pos, len);
- addr->len = len;
-
return p;
}


static u_char *
-ngx_proxy_protocol_read_port(u_char *p, u_char *last, in_port_t *port,
+ngx_proxy_protocol_read_port(u_char *p, u_char *last, struct sockaddr *sa,
u_char sep)
{
size_t len;
@@ -270,11 +276,68 @@ ngx_proxy_protocol_read_port(u_char *p,
return NULL;
}

- *port = (in_port_t) n;
+ ngx_inet_set_port(sa, n);

return p;
}

+#if (NGX_DEBUG)
+
+static void
+ngx_proxy_protocol_debug(ngx_connection_t *c, ngx_uint_t version)
+{
+ u_char *p;
+ size_t n, len;
+ ngx_str_t src_addr, dst_addr;
+ ngx_proxy_protocol_tlv_t *tlv;
+ u_char src_text[NGX_SOCKADDR_STRLEN];
+ u_char dst_text[NGX_SOCKADDR_STRLEN];
+
+ if ((c->log->log_level & NGX_LOG_DEBUG_CORE) == 0) {
+ return;
+ }
+
+ src_addr.data = src_text;
+ src_addr.len = ngx_sock_ntop(c->proxy_protocol->src_sockaddr,
+ c->proxy_protocol->src_socklen,
+ src_text, NGX_SOCKADDR_STRLEN, 1);
+
+ dst_addr.data = dst_text;
+ dst_addr.len = ngx_sock_ntop(c->proxy_protocol->dst_sockaddr,
+ c->proxy_protocol->dst_socklen,
+ dst_text, NGX_SOCKADDR_STRLEN, 1);
+
+ ngx_log_debug3(NGX_LOG_DEBUG_CORE, c->log, 0,
+ "PROXY protocol v%ui %V %V", version, &src_addr, &dst_addr);
+
+ p = c->proxy_protocol->tlvs.data;
+ n = c->proxy_protocol->tlvs.len;
+
+ for ( ;; ) {
+ if (n < sizeof(ngx_proxy_protocol_tlv_t)) {
+ break;
+ }
+
+ tlv = (ngx_proxy_protocol_tlv_t *) p;
+ len = ngx_proxy_protocol_parse_uint16(tlv->len);
+
+ p += sizeof(ngx_proxy_protocol_tlv_t);
+ n -= sizeof(ngx_proxy_protocol_tlv_t);
+
+ if (n < len) {
+ break;
+ }
+
+ ngx_log_debug3(NGX_LOG_DEBUG_CORE, c->log, 0,
+ "PROXY protocol tlv:%02xi %*xs",
+ (ngx_uint_t) tlv->type, len, p);
+
+ p += len;
+ n -= len;
+ }
+}
+
+#endif

u_char *
ngx_proxy_protocol_write(ngx_connection_t *c, u_char *buf, u_char *last)
@@ -327,13 +390,13 @@ ngx_proxy_protocol_v2_read(ngx_connectio
{
u_char *end;
size_t len;
- socklen_t socklen;
ngx_uint_t version, command, family, transport;
- ngx_sockaddr_t src_sockaddr, dst_sockaddr;
+ struct sockaddr_in *sin;
ngx_proxy_protocol_t *pp;
ngx_proxy_protocol_header_t *header;
ngx_proxy_protocol_inet_addrs_t *in;
#if (NGX_HAVE_INET6)
+ struct sockaddr_in6 *sin6;
ngx_proxy_protocol_inet6_addrs_t *in6;
#endif

@@ -394,18 +457,29 @@ ngx_proxy_protocol_v2_read(ngx_connectio

in = (ngx_proxy_protocol_inet_addrs_t *) buf;

- src_sockaddr.sockaddr_in.sin_family = AF_INET;
- src_sockaddr.sockaddr_in.sin_port = 0;
- ngx_memcpy(&src_sockaddr.sockaddr_in.sin_addr, in->src_addr, 4);
+ sin = ngx_pcalloc(c->pool, sizeof(struct sockaddr_in));
+ if (sin == NULL) {
+ return NULL;
+ }
+
+ sin->sin_family = AF_INET;
+ ngx_memcpy(&sin->sin_addr, in->src_addr, 4);
+ sin->sin_port = htons(ngx_proxy_protocol_parse_uint16(in->src_port));
+
+ pp->src_sockaddr = (struct sockaddr *) sin;
+ pp->src_socklen = sizeof(struct sockaddr_in);

- dst_sockaddr.sockaddr_in.sin_family = AF_INET;
- dst_sockaddr.sockaddr_in.sin_port = 0;
- ngx_memcpy(&dst_sockaddr.sockaddr_in.sin_addr, in->dst_addr, 4);
+ sin = ngx_pcalloc(c->pool, sizeof(struct sockaddr_in));
+ if (sin == NULL) {
+ return NULL;
+ }

- pp->src_port = ngx_proxy_protocol_parse_uint16(in->src_port);
- pp->dst_port = ngx_proxy_protocol_parse_uint16(in->dst_port);
+ sin->sin_family = AF_INET;
+ ngx_memcpy(&sin->sin_addr, in->dst_addr, 4);
+ sin->sin_port = htons(ngx_proxy_protocol_parse_uint16(in->dst_port));

- socklen = sizeof(struct sockaddr_in);
+ pp->dst_sockaddr = (struct sockaddr *) sin;
+ pp->dst_socklen = sizeof(struct sockaddr_in);

buf += sizeof(ngx_proxy_protocol_inet_addrs_t);

@@ -421,18 +495,29 @@ ngx_proxy_protocol_v2_read(ngx_connectio

in6 = (ngx_proxy_protocol_inet6_addrs_t *) buf;

- src_sockaddr.sockaddr_in6.sin6_family = AF_INET6;
- src_sockaddr.sockaddr_in6.sin6_port = 0;
- ngx_memcpy(&src_sockaddr.sockaddr_in6.sin6_addr, in6->src_addr, 16);
+ sin6 = ngx_pcalloc(c->pool, sizeof(struct sockaddr_in6));
+ if (sin6 == NULL) {
+ return NULL;
+ }
+
+ sin6->sin6_family = AF_INET6;
+ ngx_memcpy(&sin6->sin6_addr, in6->src_addr, 16);
+ sin6->sin6_port = htons(ngx_proxy_protocol_parse_uint16(in6->src_port));
+
+ pp->src_sockaddr = (struct sockaddr *) sin6;
+ pp->src_socklen = sizeof(struct sockaddr_in6);

- dst_sockaddr.sockaddr_in6.sin6_family = AF_INET6;
- dst_sockaddr.sockaddr_in6.sin6_port = 0;
- ngx_memcpy(&dst_sockaddr.sockaddr_in6.sin6_addr, in6->dst_addr, 16);
+ sin6 = ngx_pcalloc(c->pool, sizeof(struct sockaddr_in6));
+ if (sin6 == NULL) {
+ return NULL;
+ }

- pp->src_port = ngx_proxy_protocol_parse_uint16(in6->src_port);
- pp->dst_port = ngx_proxy_protocol_parse_uint16(in6->dst_port);
+ sin6->sin6_family = AF_INET6;
+ ngx_memcpy(&sin6->sin6_addr, in6->dst_addr, 16);
+ sin6->sin6_port = htons(ngx_proxy_protocol_parse_uint16(in6->dst_port));

- socklen = sizeof(struct sockaddr_in6);
+ pp->dst_sockaddr = (struct sockaddr *) sin6;
+ pp->dst_socklen = sizeof(struct sockaddr_in6);

buf += sizeof(ngx_proxy_protocol_inet6_addrs_t);

@@ -447,26 +532,6 @@ ngx_proxy_protocol_v2_read(ngx_connectio
return end;
}

- pp->src_addr.data = ngx_pnalloc(c->pool, NGX_SOCKADDR_STRLEN);
- if (pp->src_addr.data == NULL) {
- return NULL;
- }
-
- pp->src_addr.len = ngx_sock_ntop(&src_sockaddr.sockaddr, socklen,
- pp->src_addr.data, NGX_SOCKADDR_STRLEN, 0);
-
- pp->dst_addr.data = ngx_pnalloc(c->pool, NGX_SOCKADDR_STRLEN);
- if (pp->dst_addr.data == NULL) {
- return NULL;
- }
-
- pp->dst_addr.len = ngx_sock_ntop(&dst_sockaddr.sockaddr, socklen,
- pp->dst_addr.data, NGX_SOCKADDR_STRLEN, 0);
-
- ngx_log_debug4(NGX_LOG_DEBUG_CORE, c->log, 0,
- "PROXY protocol v2 src: %V %d, dst: %V %d",
- &pp->src_addr, pp->src_port, &pp->dst_addr, pp->dst_port);
-
if (buf < end) {
pp->tlvs.data = ngx_pnalloc(c->pool, end - buf);
if (pp->tlvs.data == NULL) {
@@ -479,6 +544,10 @@ ngx_proxy_protocol_v2_read(ngx_connectio

c->proxy_protocol = pp;

+#if (NGX_DEBUG)
+ ngx_proxy_protocol_debug(c, 2);
+#endif
+
return end;
}

diff --git a/src/core/ngx_proxy_protocol.h b/src/core/ngx_proxy_protocol.h
--- a/src/core/ngx_proxy_protocol.h
+++ b/src/core/ngx_proxy_protocol.h
@@ -18,10 +18,12 @@


struct ngx_proxy_protocol_s {
- ngx_str_t src_addr;
- ngx_str_t dst_addr;
- in_port_t src_port;
- in_port_t dst_port;
+ struct sockaddr *src_sockaddr;
+ struct sockaddr *dst_sockaddr;
+
+ socklen_t src_socklen;
+ socklen_t dst_socklen;
+
ngx_str_t tlvs;
};

diff --git a/src/http/modules/ngx_http_realip_module.c b/src/http/modules/ngx_http_realip_module.c
--- a/src/http/modules/ngx_http_realip_module.c
+++ b/src/http/modules/ngx_http_realip_module.c
@@ -152,6 +152,8 @@ ngx_http_realip_handler(ngx_http_request
return NGX_DECLINED;
}

+ c = r->connection;
+
switch (rlcf->type) {

case NGX_HTTP_REALIP_XREALIP:
@@ -179,14 +181,18 @@ ngx_http_realip_handler(ngx_http_request

case NGX_HTTP_REALIP_PROXY:

- if (r->connection->proxy_protocol == NULL) {
+ if (c->proxy_protocol == NULL) {
return NGX_DECLINED;
}

- value = &r->connection->proxy_protocol->src_addr;
- xfwd = NULL;
+ if (ngx_cidr_match(c->sockaddr, rlcf->from) != NGX_OK) {
+ return NGX_DECLINED;
+ }

- break;
+ addr.sockaddr = c->proxy_protocol->src_sockaddr;
+ addr.socklen = c->proxy_protocol->src_socklen;
+
+ return ngx_http_realip_set_addr(r, &addr);

default: /* NGX_HTTP_REALIP_HEADER */

@@ -225,8 +231,6 @@ ngx_http_realip_handler(ngx_http_request

found:

- c = r->connection;
-
addr.sockaddr = c->sockaddr;
addr.socklen = c->socklen;
/* addr.name = c->addr_text; */
@@ -235,10 +239,6 @@ found:
rlcf->recursive)
!= NGX_DECLINED)
{
- if (rlcf->type == NGX_HTTP_REALIP_PROXY) {
- ngx_inet_set_port(addr.sockaddr, c->proxy_protocol->src_port);
- }
-
return ngx_http_realip_set_addr(r, &addr);
}

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
@@ -201,20 +201,16 @@ static ngx_http_variable_t ngx_http_cor
{ ngx_string("remote_port"), NULL, ngx_http_variable_remote_port, 0, 0, 0 },

{ ngx_string("proxy_protocol_addr"), NULL,
- ngx_http_variable_proxy_protocol_addr,
- offsetof(ngx_proxy_protocol_t, src_addr), 0, 0 },
+ ngx_http_variable_proxy_protocol_addr, 0, 0, 0 },

{ ngx_string("proxy_protocol_port"), NULL,
- ngx_http_variable_proxy_protocol_port,
- offsetof(ngx_proxy_protocol_t, src_port), 0, 0 },
+ ngx_http_variable_proxy_protocol_port, 0, 0, 0 },

{ ngx_string("proxy_protocol_server_addr"), NULL,
- ngx_http_variable_proxy_protocol_addr,
- offsetof(ngx_proxy_protocol_t, dst_addr), 0, 0 },
+ ngx_http_variable_proxy_protocol_addr, 1, 0, 0 },

{ ngx_string("proxy_protocol_server_port"), NULL,
- ngx_http_variable_proxy_protocol_port,
- offsetof(ngx_proxy_protocol_t, dst_port), 0, 0 },
+ ngx_http_variable_proxy_protocol_port, 1, 0, 0 },

{ ngx_string("proxy_protocol_tlv_"), NULL,
ngx_http_variable_proxy_protocol_tlv,
@@ -1340,7 +1336,8 @@ static ngx_int_t
ngx_http_variable_proxy_protocol_addr(ngx_http_request_t *r,
ngx_http_variable_value_t *v, uintptr_t data)
{
- ngx_str_t *addr;
+ u_char *p;
+ size_t n;
ngx_proxy_protocol_t *pp;

pp = r->connection->proxy_protocol;
@@ -1349,13 +1346,25 @@ ngx_http_variable_proxy_protocol_addr(ng
return NGX_OK;
}

- addr = (ngx_str_t *) ((char *) pp + data);
-
- v->len = addr->len;
+ p = ngx_pnalloc(r->connection->pool, NGX_SOCKADDR_STRLEN);
+ if (p == NULL) {
+ return NGX_ERROR;
+ }
+
+ if (data) {
+ n = ngx_sock_ntop(pp->dst_sockaddr, pp->dst_socklen,
+ p, NGX_SOCKADDR_STRLEN, 0);
+
+ } else {
+ n = ngx_sock_ntop(pp->src_sockaddr, pp->src_socklen,
+ p, NGX_SOCKADDR_STRLEN, 0);
+ }
+
+ v->len = n;
v->valid = 1;
v->no_cacheable = 0;
v->not_found = 0;
- v->data = addr->data;
+ v->data = p;

return NGX_OK;
}
@@ -1384,7 +1393,12 @@ ngx_http_variable_proxy_protocol_port(ng
return NGX_ERROR;
}

- port = *(in_port_t *) ((char *) pp + data);
+ if (data) {
+ port = ngx_inet_get_port(pp->dst_sockaddr);
+
+ } else {
+ port = ngx_inet_get_port(pp->src_sockaddr);
+ }

if (port > 0 && port < 65536) {
v->len = ngx_sprintf(v->data, "%ui", port) - v->data;
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
@@ -1249,11 +1249,11 @@ ngx_mail_auth_http_create_request(ngx_ma

if (c->proxy_protocol) {
len += sizeof("Proxy-Protocol-Addr: ") - 1
- + c->proxy_protocol->src_addr.len + sizeof(CRLF) - 1
+ + NGX_SOCKADDR_STRLEN + sizeof(CRLF) - 1
+ sizeof("Proxy-Protocol-Port: ") - 1
+ sizeof("65535") - 1 + sizeof(CRLF) - 1
+ sizeof("Proxy-Protocol-Server-Addr: ") - 1
- + c->proxy_protocol->dst_addr.len + sizeof(CRLF) - 1
+ + NGX_SOCKADDR_STRLEN + sizeof(CRLF) - 1
+ sizeof("Proxy-Protocol-Server-Port: ") - 1
+ sizeof("65535") - 1 + sizeof(CRLF) - 1;
}
@@ -1352,21 +1352,23 @@ ngx_mail_auth_http_create_request(ngx_ma
if (c->proxy_protocol) {
b->last = ngx_cpymem(b->last, "Proxy-Protocol-Addr: ",
sizeof("Proxy-Protocol-Addr: ") - 1);
- b->last = ngx_copy(b->last, c->proxy_protocol->src_addr.data,
- c->proxy_protocol->src_addr.len);
+ b->last += ngx_sock_ntop(c->proxy_protocol->src_sockaddr,
+ c->proxy_protocol->src_socklen,
+ b->last, b->end - b->last, 0);
*b->last++ = CR; *b->last++ = LF;

b->last = ngx_sprintf(b->last, "Proxy-Protocol-Port: %d" CRLF,
- c->proxy_protocol->src_port);
+ (int) ngx_inet_get_port(c->proxy_protocol->src_sockaddr));

b->last = ngx_cpymem(b->last, "Proxy-Protocol-Server-Addr: ",
sizeof("Proxy-Protocol-Server-Addr: ") - 1);
- b->last = ngx_copy(b->last, c->proxy_protocol->dst_addr.data,
- c->proxy_protocol->dst_addr.len);
+ b->last += ngx_sock_ntop(c->proxy_protocol->dst_sockaddr,
+ c->proxy_protocol->dst_socklen,
+ b->last, b->end - b->last, 0);
*b->last++ = CR; *b->last++ = LF;

b->last = ngx_sprintf(b->last, "Proxy-Protocol-Server-Port: %d" CRLF,
- c->proxy_protocol->dst_port);
+ (int) ngx_inet_get_port(c->proxy_protocol->dst_sockaddr));
}

if (s->auth_method == NGX_MAIL_AUTH_NONE) {
diff --git a/src/mail/ngx_mail_realip_module.c b/src/mail/ngx_mail_realip_module.c
--- a/src/mail/ngx_mail_realip_module.c
+++ b/src/mail/ngx_mail_realip_module.c
@@ -87,14 +87,8 @@ ngx_mail_realip_handler(ngx_mail_session
return NGX_OK;
}

- if (ngx_parse_addr(c->pool, &addr, c->proxy_protocol->src_addr.data,
- c->proxy_protocol->src_addr.len)
- != NGX_OK)
- {
- return NGX_OK;
- }
-
- ngx_inet_set_port(addr.sockaddr, c->proxy_protocol->src_port);
+ addr.sockaddr = c->proxy_protocol->src_sockaddr;
+ addr.socklen = c->proxy_protocol->src_socklen;

return ngx_mail_realip_set_addr(s, &addr);
}
diff --git a/src/stream/ngx_stream_realip_module.c b/src/stream/ngx_stream_realip_module.c
--- a/src/stream/ngx_stream_realip_module.c
+++ b/src/stream/ngx_stream_realip_module.c
@@ -116,14 +116,8 @@ ngx_stream_realip_handler(ngx_stream_ses
return NGX_DECLINED;
}

- if (ngx_parse_addr(c->pool, &addr, c->proxy_protocol->src_addr.data,
- c->proxy_protocol->src_addr.len)
- != NGX_OK)
- {
- return NGX_DECLINED;
- }
-
- ngx_inet_set_port(addr.sockaddr, c->proxy_protocol->src_port);
+ addr.sockaddr = c->proxy_protocol->src_sockaddr;
+ addr.socklen = c->proxy_protocol->src_socklen;

return ngx_stream_realip_set_addr(s, &addr);
}
diff --git a/src/stream/ngx_stream_variables.c b/src/stream/ngx_stream_variables.c
--- a/src/stream/ngx_stream_variables.c
+++ b/src/stream/ngx_stream_variables.c
@@ -66,20 +66,16 @@ static ngx_stream_variable_t ngx_stream
ngx_stream_variable_remote_port, 0, 0, 0 },

{ ngx_string("proxy_protocol_addr"), NULL,
- ngx_stream_variable_proxy_protocol_addr,
- offsetof(ngx_proxy_protocol_t, src_addr), 0, 0 },
+ ngx_stream_variable_proxy_protocol_addr, 0, 0, 0 },

{ ngx_string("proxy_protocol_port"), NULL,
- ngx_stream_variable_proxy_protocol_port,
- offsetof(ngx_proxy_protocol_t, src_port), 0, 0 },
+ ngx_stream_variable_proxy_protocol_port, 0, 0, 0 },

{ ngx_string("proxy_protocol_server_addr"), NULL,
- ngx_stream_variable_proxy_protocol_addr,
- offsetof(ngx_proxy_protocol_t, dst_addr), 0, 0 },
+ ngx_stream_variable_proxy_protocol_addr, 1, 0, 0 },

{ ngx_string("proxy_protocol_server_port"), NULL,
- ngx_stream_variable_proxy_protocol_port,
- offsetof(ngx_proxy_protocol_t, dst_port), 0, 0 },
+ ngx_stream_variable_proxy_protocol_port, 1, 0, 0 },

{ ngx_string("proxy_protocol_tlv_"), NULL,
ngx_stream_variable_proxy_protocol_tlv,
@@ -573,7 +569,8 @@ static ngx_int_t
ngx_stream_variable_proxy_protocol_addr(ngx_stream_session_t *s,
ngx_stream_variable_value_t *v, uintptr_t data)
{
- ngx_str_t *addr;
+ u_char *p;
+ size_t n;
ngx_proxy_protocol_t *pp;

pp = s->connection->proxy_protocol;
@@ -582,13 +579,25 @@ ngx_stream_variable_proxy_protocol_addr(
return NGX_OK;
}

- addr = (ngx_str_t *) ((char *) pp + data);
+ p = ngx_pnalloc(s->connection->pool, NGX_SOCKADDR_STRLEN);
+ if (p == NULL) {
+ return NGX_ERROR;
+ }

- v->len = addr->len;
+ if (data) {
+ n = ngx_sock_ntop(pp->dst_sockaddr, pp->dst_socklen,
+ p, NGX_SOCKADDR_STRLEN, 0);
+
+ } else {
+ n = ngx_sock_ntop(pp->src_sockaddr, pp->src_socklen,
+ p, NGX_SOCKADDR_STRLEN, 0);
+ }
+
+ v->len = n;
v->valid = 1;
v->no_cacheable = 0;
v->not_found = 0;
- v->data = addr->data;
+ v->data = p;

return NGX_OK;
}
@@ -617,7 +626,12 @@ ngx_stream_variable_proxy_protocol_port(
return NGX_ERROR;
}

- port = *(in_port_t *) ((char *) pp + data);
+ if (data) {
+ port = ngx_inet_get_port(pp->dst_sockaddr);
+
+ } else {
+ port = ngx_inet_get_port(pp->src_sockaddr);
+ }

if (port > 0 && port < 65536) {
v->len = ngx_sprintf(v->data, "%ui", port) - v->data;
# HG changeset patch
# User Roman Arutyunyan <arut@nginx.com>
# Date 1710081227 -14400
# Sun Mar 10 18:33:47 2024 +0400
# Node ID 6957c271a15c789be51d042de6835ce02141fe58
# Parent 07c8c445746c6c74250bad7b42cd26dbb0ed6dd3
Pass PROXY protocol header from client to upstream (ticket #2010).

When creating a PROXY protocol header in mail or stream, server address is
taken from c->local_sockaddr, which is normally the real server address of the
connection. If prior to that client address was substituted by the realip
module, then client and server addresses can have different address families,
which breaks PROXY protocol syntax. The change fixes this by using server
address from client PROXY protocol header 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
@@ -375,11 +375,24 @@ ngx_proxy_protocol_write(ngx_connection_

*buf++ = ' ';

- buf += ngx_sock_ntop(c->local_sockaddr, c->local_socklen, buf, last - buf,
- 0);
+ if (c->proxy_protocol
+ && ngx_cmp_sockaddr(c->proxy_protocol->src_sockaddr,
+ c->proxy_protocol->src_socklen,
+ c->sockaddr, c->socklen, 1)
+ == NGX_OK)
+ {
+ buf += ngx_sock_ntop(c->proxy_protocol->dst_sockaddr,
+ c->proxy_protocol->dst_socklen,
+ buf, last - buf, 0);
+ lport = ngx_inet_get_port(c->proxy_protocol->dst_sockaddr);
+
+ } else {
+ buf += ngx_sock_ntop(c->local_sockaddr, c->local_socklen,
+ buf, last - buf, 0);
+ lport = ngx_inet_get_port(c->local_sockaddr);
+ }

port = ngx_inet_get_port(c->sockaddr);
- lport = ngx_inet_get_port(c->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
Subject Author Views Posted

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

Roman Arutyunyan 307 January 22, 2024 05:52AM

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

Maxim Dounin 57 January 22, 2024 07:00AM

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

Roman Arutyunyan 53 January 22, 2024 10:50AM

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

Maxim Dounin 57 January 23, 2024 04:04PM

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

Roman Arutyunyan 44 February 21, 2024 08:32AM

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

J Carter 54 February 21, 2024 09:00PM

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

Roman Arutyunyan 53 February 22, 2024 10:18AM

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

Sergey Kandaurov 44 March 06, 2024 09:52AM

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

Roman Arutyunyan 45 March 11, 2024 08:46AM

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

Sergey Kandaurov 40 March 13, 2024 01:10PM

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

Roman Arutyunyan 42 March 21, 2024 10:58AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 311
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready