[Openvpn-devel,S] Change in openvpn[master]: mroute/management: fix unitialized variable (UNINIT)

Message ID e4c4a200aa5e5ed61e748d4d5fbc7c0ecf8bbdfb-HTML@gerrit.openvpn.net
State Superseded
Headers show
Series [Openvpn-devel,S] Change in openvpn[master]: mroute/management: fix unitialized variable (UNINIT) | expand

Commit Message

cron2 (Code Review) Jan. 28, 2025, 1:57 p.m. UTC
Attention is currently required from: flichtenheld, plaisthos.

Hello plaisthos, flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/880?usp=email

to review the following change.


Change subject: mroute/management: fix unitialized variable (UNINIT)
......................................................................

mroute/management: fix unitialized variable (UNINIT)

Fix issue reported by Coverity:
CID 1641564: Uninitialized variables (UNINIT)
Using unitialized value "maddr.proto" when
calling "mroute_addr_equal()".

Fix this by passing the proto along with
IP:port.

While at it, changed the
mroute_addr_print_ex() format to display
the protocol only in case of MR_WITH_PROTO
avoid doing it on virtual addresses when
MR_WITH_PORT is not specified.

Change-Id: I4be0ff4d308213d2ef8ba66bd3178eee1f60fff1
Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
---
M Changes.rst
M src/openvpn/manage.c
M src/openvpn/manage.h
M src/openvpn/mroute.c
M src/openvpn/mroute.h
M src/openvpn/mtcp.c
M src/openvpn/multi.c
7 files changed, 37 insertions(+), 10 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/80/880/1

Patch

diff --git a/Changes.rst b/Changes.rst
index 16ae6fc..d01816b 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -317,6 +317,9 @@ 
   settings will contradict the setting of allow-compression as this almost
   always results in a non-working connection.
 
+- The "kill" by addr management command now requires also the protocol
+  as string e.g. "udp", "tcp".
+
 Common errors with OpenSSL 3.0 and OpenVPN 2.6
 ----------------------------------------------
 Both OpenVPN 2.6 and OpenSSL 3.0 tighten the security considerable, so some
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index 0c77f85..037945c 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -544,13 +544,15 @@ 
         struct buffer buf;
         char p1[128];
         char p2[128];
+        char p3[128];
         int n_killed;
 
         buf_set_read(&buf, (uint8_t *) victim, strlen(victim) + 1);
         buf_parse(&buf, ':', p1, sizeof(p1));
         buf_parse(&buf, ':', p2, sizeof(p2));
+        buf_parse(&buf, ':', p3, sizeof(p3));
 
-        if (strlen(p1) && strlen(p2))
+        if (strlen(p1) && strlen(p2) && strlen(p3))
         {
             /* IP:port specified */
             bool status;
@@ -558,21 +560,23 @@ 
             if (status)
             {
                 const int port = atoi(p2);
+                const int proto = (streq(p3, "tcp")) ? PROTO_TCP_SERVER : ascii2proto(p3);
+
                 if (port > 0 && port < 65536)
                 {
-                    n_killed = (*man->persist.callback.kill_by_addr)(man->persist.callback.arg, addr, port);
+                    n_killed = (*man->persist.callback.kill_by_addr)(man->persist.callback.arg, addr, port, proto);
                     if (n_killed > 0)
                     {
-                        msg(M_CLIENT, "SUCCESS: %d client(s) at address %s:%d killed",
+                        msg(M_CLIENT, "SUCCESS: %d client(s) at address %s:%d:%s killed",
                             n_killed,
                             print_in_addr_t(addr, 0, &gc),
-                            port);
+                            port, p3);
                     }
                     else
                     {
-                        msg(M_CLIENT, "ERROR: client at address %s:%d not found",
+                        msg(M_CLIENT, "ERROR: client at address %s:%d:%s not found",
                             print_in_addr_t(addr, 0, &gc),
-                            port);
+                            port, p3);
                     }
                 }
                 else
diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h
index f501543..02ceb82 100644
--- a/src/openvpn/manage.h
+++ b/src/openvpn/manage.h
@@ -180,7 +180,7 @@ 
     void (*status) (void *arg, const int version, struct status_output *so);
     void (*show_net) (void *arg, const int msglevel);
     int (*kill_by_cn) (void *arg, const char *common_name);
-    int (*kill_by_addr) (void *arg, const in_addr_t addr, const int port);
+    int (*kill_by_addr) (void *arg, const in_addr_t addr, const int port, const int proto);
     void (*delete_event) (void *arg, event_t event);
     int (*n_clients) (void *arg);
     bool (*send_cc_message) (void *arg, const char *message, const char *parameter);
diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
index 74923cf..45777e4 100644
--- a/src/openvpn/mroute.c
+++ b/src/openvpn/mroute.c
@@ -276,6 +276,10 @@ 
                 addr->len = 6;
                 addr->v4.addr = osaddr->addr.in4.sin_addr.s_addr;
                 addr->v4.port = osaddr->addr.in4.sin_port;
+                if (addr->proto != PROTO_NONE)
+                {
+                    addr->type |= MR_WITH_PROTO;
+                }
             }
             else
             {
@@ -295,6 +299,10 @@ 
                 addr->len = 18;
                 addr->v6.addr = osaddr->addr.in6.sin6_addr;
                 addr->v6.port = osaddr->addr.in6.sin6_port;
+                if (addr->proto != PROTO_NONE)
+                {
+                    addr->type |= MR_WITH_PROTO;
+                }
             }
             else
             {
@@ -403,6 +411,10 @@ 
                 {
                     buf_printf(&out, "ARP/");
                 }
+                if (maddr.type & MR_WITH_PROTO)
+                {
+                    buf_printf(&out, "%s:", proto2ascii(maddr.proto, AF_INET, false));
+                }
                 buf_printf(&out, "%s", print_in_addr_t(ntohl(maddr.v4.addr),
                                                        (flags & MAPF_IA_EMPTY_IF_UNDEF) ? IA_EMPTY_IF_UNDEF : 0, gc));
                 if (maddr.type & MR_WITH_NETBITS)
@@ -426,6 +438,10 @@ 
 
             case MR_ADDR_IPV6:
             {
+                if ((maddr.type & MR_WITH_PROTO))
+                {
+                    buf_printf(&out, "%s:", proto2ascii(maddr.proto, AF_INET6, false));
+                }
                 if (IN6_IS_ADDR_V4MAPPED( &maddr.v6.addr ) )
                 {
                     buf_printf(&out, "%s", print_in_addr_t(maddr.v4mappedv6.addr,
@@ -454,7 +470,6 @@ 
                 buf_printf(&out, "UNKNOWN");
                 break;
         }
-        buf_printf(&out, "|%d", maddr.proto);
         return BSTR(&out);
     }
     else
diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h
index 2659695..fbe102a 100644
--- a/src/openvpn/mroute.h
+++ b/src/openvpn/mroute.h
@@ -72,6 +72,9 @@ 
 /* Indicates than IPv4 addr was extracted from ARP packet */
 #define MR_ARP                   16
 
+/* Address type mask indicating that proto # is part of address */
+#define MR_WITH_PROTO            32
+
 struct mroute_addr {
     uint8_t len;    /* length of address */
     uint8_t proto;
diff --git a/src/openvpn/mtcp.c b/src/openvpn/mtcp.c
index 62ed044..9a8b1cb 100644
--- a/src/openvpn/mtcp.c
+++ b/src/openvpn/mtcp.c
@@ -111,6 +111,7 @@ 
     ASSERT(mi->context.c2.link_sockets[0]->info.lsa->actual.dest.addr.sa.sa_family == AF_INET
            || mi->context.c2.link_sockets[0]->info.lsa->actual.dest.addr.sa.sa_family == AF_INET6
            );
+    mi->real.proto = mi->context.c2.link_sockets[0]->info.proto;
     if (!mroute_extract_openvpn_sockaddr(&mi->real,
                                          &mi->context.c2.link_sockets[0]->info.lsa->actual.dest,
                                          true))
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9c8c014..b0e1941 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -794,7 +794,6 @@ 
         {
             goto err;
         }
-        mi->real.proto = ls->info.proto;
         generate_prefix(mi);
     }
 
@@ -3942,7 +3941,8 @@ 
 }
 
 static int
-management_callback_kill_by_addr(void *arg, const in_addr_t addr, const int port)
+management_callback_kill_by_addr(void *arg, const in_addr_t addr,
+                                 const int port, const int proto)
 {
     struct multi_context *m = (struct multi_context *) arg;
     struct hash_iterator hi;
@@ -3957,6 +3957,7 @@ 
     saddr.addr.in4.sin_port = htons(port);
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
+        maddr.proto = proto;
         hash_iterator_init(m->iter, &hi);
         while ((he = hash_iterator_next(&hi)))
         {