diff mbox series

[Openvpn-devel,v1] NTLM: add length check to add_security_buffer

Message ID 20240117085951.27414-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v1] NTLM: add length check to add_security_buffer | expand

Commit Message

Gert Doering Jan. 17, 2024, 8:59 a.m. UTC 
From: Frank Lichtenheld <frank@lichtenheld.com>

Especially ntlmv2_response can be very big, so make sure
we not do exceed the size of the phase3 buffer.

Change-Id: Icea931d29e3e504e23e045539b21013b42172664
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/496
This mail reflects revision 1 of this Change.
Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Gert Doering Jan. 17, 2024, 9:24 a.m. UTC | #1 
This extra check makes sense, the code is not very robust here - maybe
the wording of the message could be made more understandable (what does
"security buffer too big for message buffer" mean?) but at least we
have a check + message now.

I have not tested this for real as I do not have a working NTLM setup,
but Frank has, and the code does not affect anything "not NTLM".

Your patch has been applied to the master branch.

The *check* needs to go into release/2.6 as well, but since the code is
different (due to NTLMv1 removal) there is one extra add_security_buffer()
to be handled by the 2.6 patch (gerrit/493).  Coming next :-)

commit a021de2aabb21a24c7b69aaae1c710a9b6fee429 (master)
Author: Frank Lichtenheld
Date:   Wed Jan 17 09:59:51 2024 +0100

     NTLM: add length check to add_security_buffer

     Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20240117085951.27414-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28037.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ntlm.c b/src/openvpn/ntlm.c
index bc33f41..99d4ae7 100644
--- a/src/openvpn/ntlm.c
+++ b/src/openvpn/ntlm.c
@@ -154,8 +154,13 @@ 
 
 static void
 add_security_buffer(int sb_offset, void *data, int length,
-                    unsigned char *msg_buf, int *msg_bufpos)
+                    unsigned char *msg_buf, int *msg_bufpos, size_t msg_bufsize)
 {
+    if (*msg_bufpos + length > msg_bufsize)
+    {
+        msg(M_WARN, "NTLM: security buffer too big for message buffer");
+        return;
+    }
     /* Adds security buffer data to a message and sets security buffer's
      * offset and length */
     msg_buf[sb_offset] = (unsigned char)length;
@@ -362,15 +367,15 @@ 
 
     /* NTLMv2 response */
     add_security_buffer(0x14, ntlmv2_response, ntlmv2_blob_size + 16,
-                        phase3, &phase3_bufpos);
+                        phase3, &phase3_bufpos, sizeof(phase3));
 
     /* username in ascii */
     add_security_buffer(0x24, username, strlen(username), phase3,
-                        &phase3_bufpos);
+                        &phase3_bufpos, sizeof(phase3));
 
     /* Set domain. If <domain> is empty, default domain will be used
      * (i.e. proxy's domain) */
-    add_security_buffer(0x1c, domain, strlen(domain), phase3, &phase3_bufpos);
+    add_security_buffer(0x1c, domain, strlen(domain), phase3, &phase3_bufpos, sizeof(phase3));
 
     /* other security buffers will be empty */
     phase3[0x10] = phase3_bufpos;     /* lm not used */