Welcome! Log In Create A New Profile

Advanced

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

Roman Arutyunyan
February 22, 2024 10:18AM
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
Subject Author Views Posted

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

Roman Arutyunyan 354 January 22, 2024 05:52AM

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

Maxim Dounin 73 January 22, 2024 07:00AM

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

Roman Arutyunyan 67 January 22, 2024 10:50AM

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

Maxim Dounin 69 January 23, 2024 04:04PM

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

Roman Arutyunyan 64 February 21, 2024 08:32AM

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

J Carter 74 February 21, 2024 09:00PM

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

Roman Arutyunyan 67 February 22, 2024 10:18AM

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

Sergey Kandaurov 62 March 06, 2024 09:52AM

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

Roman Arutyunyan 72 March 11, 2024 08:46AM

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

Sergey Kandaurov 68 March 13, 2024 01:10PM

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

Roman Arutyunyan 83 March 21, 2024 10:58AM



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

Online Users

Guests: 335
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 466 on July 09, 2024
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready