diff mbox series

[Openvpn-devel,v2] interactive.c: Fix potential stack overflow issue

Message ID 20240319152803.1801-2-lev@openvpn.net
State Accepted
Headers show
Series [Openvpn-devel,v2] interactive.c: Fix potential stack overflow issue | expand

Commit Message

Lev Stipakov March 19, 2024, 3:27 p.m. UTC 
When reading message from the pipe, we first peek the pipe to get the size
of the message waiting to be read and then read the message. A compromised
OpenVPN process could send an excessively large message, which would result
in a stack-allocated message buffer overflow.

To address this, we terminate the misbehaving process if the peeked message
size exceeds the maximum allowable size.

CVE: 2024-27459
Microsoft case number: 85932

Reported-by: Vladimir Tokarev <vtokarev@microsoft.com>
Change-Id: Ib5743cba0741ea11f9ee62c4978b2c6789b81ada
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Heiko Hund <heiko@openvpn.net>
---

 v2: added CVE and MSFT case number

 src/openvpnserv/interactive.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

Comments

Gert Doering March 19, 2024, 6:08 p.m. UTC | #1 
As for the two previous windows/CVE patches, this patch was sent "with
ACK included" to the openvpn-devel@ list because it was developed under
embargo (CVE), and reviewed and ACKed in a closed group.  I have verified
that this patch is identical to the "v2" version that Heiko and the original
reporter saw and ACKed. 

The patch looks larger than the actual code change, because to do the
size check the union typedef needs to move outside the function where
it was "local" before.  The actual check is very straightforward - "if
there is more data in the pipe than can fit into a pipe_message_t,
log an error and close this thread" (thus, abandon the process on the
other end that pretends to be openvpn.exe but is misbehaving).

In itself, this bug is annoying, but can not be directly exploited
(because you cannot "just talk to this pipe", but you need to be
openvpn.exe from the install directory).  Combined with other potential
flaws that give an attacker the opportunity to swap the openvpn.exe
binary or get a malicious plugin loaded, this could end up being a
local privilege escalation to SYSTEM.  No exploit is known so far - this
was found by code inspection for missing bounds checks.


I have test compiled this on MinGW and GHA, but did not actually run it.

Your patch has been applied to the master and release/2.6 branch
(security relevant bugfix).  A direct cherrypick to 2.5 fails due to
"sufficiently different code and data structures" so I've asked Lev
to send a 2.5 version which I could review-and-ACK then.

commit 989b22cb6e007fd1addcfaf7d12f4fec9fbc9639 (master)
commit 9b2693feff9c49b9485cf94797c1c3502259dbe1 (release/2.6)

Author: Lev Stipakov
Date:   Tue Mar 19 17:27:11 2024 +0200

     interactive.c: Fix potential stack overflow issue

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Heiko Hund <heiko@openvpn.net>
     Message-Id: <20240319152803.1801-2-lev@openvpn.net>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28420.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering March 19, 2024, 6:08 p.m. UTC | #2 
As for the two previous windows/CVE patches, this patch was sent "with
ACK included" to the openvpn-devel@ list because it was developed under
embargo (CVE), and reviewed and ACKed in a closed group.  I have verified
that this patch is identical to the "v2" version that Heiko and the original
reporter saw and ACKed. 

The patch looks larger than the actual code change, because to do the
size check the union typedef needs to move outside the function where
it was "local" before.  The actual check is very straightforward - "if
there is more data in the pipe than can fit into a pipe_message_t,
log an error and close this thread" (thus, abandon the process on the
other end that pretends to be openvpn.exe but is misbehaving).

In itself, this bug is annoying, but can not be directly exploited
(because you cannot "just talk to this pipe", but you need to be
openvpn.exe from the install directory).  Combined with other potential
flaws that give an attacker the opportunity to swap the openvpn.exe
binary or get a malicious plugin loaded, this could end up being a
local privilege escalation to SYSTEM.  No exploit is known so far - this
was found by code inspection for missing bounds checks.


I have test compiled this on MinGW and GHA, but did not actually run it.

Your patch has been applied to the master and release/2.6 branch
(security relevant bugfix).  A direct cherrypick to 2.5 fails due to
"sufficiently different code and data structures" so I've asked Lev
to send a 2.5 version which I could review-and-ACK then.

commit 989b22cb6e007fd1addcfaf7d12f4fec9fbc9639 (master)
commit 9b2693feff9c49b9485cf94797c1c3502259dbe1 (release/2.6)

Author: Lev Stipakov
Date:   Tue Mar 19 17:27:11 2024 +0200

     interactive.c: Fix potential stack overflow issue

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Heiko Hund <heiko@openvpn.net>
     Message-Id: <20240319152803.1801-2-lev@openvpn.net>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28420.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 32c8996c..24e3f341 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -106,6 +106,18 @@  typedef struct {
     struct tun_ring *receive_ring;
 } ring_buffer_maps_t;
 
+typedef union {
+    message_header_t header;
+    address_message_t address;
+    route_message_t route;
+    flush_neighbors_message_t flush_neighbors;
+    block_dns_message_t block_dns;
+    dns_cfg_message_t dns;
+    enable_dhcp_message_t dhcp;
+    register_ring_buffers_message_t rrb;
+    set_mtu_message_t mtu;
+    wins_cfg_message_t wins;
+} pipe_message_t;
 
 static DWORD
 AddListItem(list_item_t **pfirst, LPVOID data)
@@ -1610,19 +1622,7 @@  static VOID
 HandleMessage(HANDLE pipe, HANDLE ovpn_proc,
               DWORD bytes, DWORD count, LPHANDLE events, undo_lists_t *lists)
 {
-    DWORD read;
-    union {
-        message_header_t header;
-        address_message_t address;
-        route_message_t route;
-        flush_neighbors_message_t flush_neighbors;
-        block_dns_message_t block_dns;
-        dns_cfg_message_t dns;
-        enable_dhcp_message_t dhcp;
-        register_ring_buffers_message_t rrb;
-        set_mtu_message_t mtu;
-        wins_cfg_message_t wins;
-    } msg;
+    pipe_message_t msg;
     ack_message_t ack = {
         .header = {
             .type = msg_acknowledgement,
@@ -1632,7 +1632,7 @@  HandleMessage(HANDLE pipe, HANDLE ovpn_proc,
         .error_number = ERROR_MESSAGE_DATA
     };
 
-    read = ReadPipeAsync(pipe, &msg, bytes, count, events);
+    DWORD read = ReadPipeAsync(pipe, &msg, bytes, count, events);
     if (read != bytes || read < sizeof(msg.header) || read != msg.header.size)
     {
         goto out;
@@ -2059,6 +2059,13 @@  RunOpenvpn(LPVOID p)
             break;
         }
 
+        if (bytes > sizeof(pipe_message_t))
+        {
+            /* process at the other side of the pipe is misbehaving, shut it down */
+            MsgToEventLog(MSG_FLAGS_ERROR, TEXT("OpenVPN process sent too large payload length to the pipe (%lu bytes), it will be terminated"), bytes);
+            break;
+        }
+
         HandleMessage(ovpn_pipe, proc_info.hProcess, bytes, 1, &exit_event, &undo_lists);
     }