[Openvpn-devel,1/2] Fix IPv6 in port-share journal

Message ID 8de5660b-d917-4092-8871-250495d8c7a4@gmx.de
State Accepted
Headers show
Series [Openvpn-devel,1/2] Fix IPv6 in port-share journal | expand

Commit Message

corubba Dec. 7, 2024, 11:17 p.m. UTC
getpeername() and getsockname() will truncate the result if it is
larger than the passed-in length. Because here always the size of the
`sa` IPv4 union member was passed in, all larger (aka IPv6) results
were truncated. Instead use the size of the `addr` union, which is the
maximum size of all union members.

The bug was introduced in 0b6450c9.

Fixes https://community.openvpn.net/openvpn/ticket/1358

Signed-off-by: corubba <corubba@gmx.de>
---
 src/openvpn/ps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
2.47.1

Comments

Gert Doering Dec. 8, 2024, 1:52 p.m. UTC | #1
Hi,

On Sun, Dec 08, 2024 at 12:17:05AM +0100, corubba via Openvpn-devel wrote:
> getpeername() and getsockname() will truncate the result if it is
> larger than the passed-in length. Because here always the size of the
> `sa` IPv4 union member was passed in, all larger (aka IPv6) results
> were truncated. Instead use the size of the `addr` union, which is the
> maximum size of all union members.

Thanks for digging into this.  The fix looks correct according to
structure definitions, "man getpeername()" etc. - and now matches
other parts of the code, like socket.c / socket_do_accept()

...
    socklen_t remote_len = sizeof(act->dest.addr);
...
        new_sd = getpeername(sd, &act->dest.addr.sa, &remote_len);

The commit ID you found introduces IPv6 transport support :-) - so before
that, there was no ambiguity here.

So

Acked-By: Gert Doering <gert@greenie.muc.de>

will proceed to merge ASAP.  (I'll change the Trac reference to be
just "Trac: #1358" as this is our standard format).

gert
Gert Doering Dec. 8, 2024, 2:17 p.m. UTC | #2
I have not actually tested this (beyond "GHA run" passed) because I have
no current test setup for incoming port share with journal files - but
the code change is "obviously correct" and our picky compilers do not
object either.

Your patch has been applied to the master and release/2.6 branch (bugfix).

commit dbc0491f20c34cf3b7ab8fe2a55442ea93007ddd (master)
commit f966e67c24af681cdd8c3905f4da07e616a123c5 (release/2.6)
Author: corubba
Date:   Sun Dec 8 00:17:05 2024 +0100

     Fix IPv6 in port-share journal

     Signed-off-by: corubba <corubba@gmx.de>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <8de5660b-d917-4092-8871-250495d8c7a4@gmx.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg30035.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
index 4ca3a129..06bf91a8 100644
--- a/src/openvpn/ps.c
+++ b/src/openvpn/ps.c
@@ -344,8 +344,8 @@  journal_add(const char *journal_dir, struct proxy_connection *pc, struct proxy_c
     char *jfn;
     int fd;

-    slen = sizeof(from.addr.sa);
-    dlen = sizeof(to.addr.sa);
+    slen = sizeof(from.addr);
+    dlen = sizeof(to.addr);
     if (!getpeername(pc->sd, (struct sockaddr *) &from.addr.sa, &slen)
         && !getsockname(cp->sd, (struct sockaddr *) &to.addr.sa, &dlen))
     {