diff mbox series

[Openvpn-devel] ntlm: Clarify details on NTLM phase 3 decoding

Message ID 20230802113149.36497-1-dazo+openvpn@eurephia.org
State Accepted
Headers show
Series [Openvpn-devel] ntlm: Clarify details on NTLM phase 3 decoding | expand

Commit Message

David Sommerseth Aug. 2, 2023, 11:31 a.m. UTC 
From: David Sommerseth <davids@openvpn.net>

The code was very clear if we accept that the base64 decode of the
NTLM challenge was truncated or not.  Move the related code lines
closer to where it first used and comment what we are not concerned
about any truncation.

If the decoded result is truncated, the NTLM server side will reject
our new response to the challenge as it will be incorrect.  The
buffer size fixed and known to be in a cleared state before the
decode starts.

Resolves: TOB-OVPN-14
Signed-off-by: David Sommerseth <davids@openvpn.net>
---
 src/openvpn/ntlm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

David Sommerseth Aug. 2, 2023, 11:36 a.m. UTC | #1 
On 02/08/2023 13:31, David Sommerseth wrote:
> From: David Sommerseth <davids@openvpn.net>
> 
> The code was very clear if we accept that the base64 decode of the

There is a "not" missing in the line above:  "The code was not very 
clear ..."

I'm fine with fixing this at commit time.
Gert Doering Aug. 11, 2023, 4:24 p.m. UTC | #2 
Acked-by: Gert Doering <gert@greenie.muc.de>

No functional change, just clarifying "yes we know that this could
happen, but if it does, this is still well-defined".

Test compiled on Linux, just in case I overlooked something.

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

commit f19391139836aa07312cf5b3ebbd00941d22ddc7 (master)
commit 781fa8f200d0e3428a7e4da693707529eeaa66cc (release/2.6)
Author: David Sommerseth
Date:   Wed Aug 2 13:31:49 2023 +0200

     ntlm: Clarify details on NTLM phase 3 decoding

     Signed-off-by: David Sommerseth <davids@openvpn.net>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20230802113149.36497-1-dazo+openvpn@eurephia.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26919.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 0cb0a32f..2e772141 100644
--- a/src/openvpn/ntlm.c
+++ b/src/openvpn/ntlm.c
@@ -207,7 +207,6 @@  ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2,
      */
 
     char pwbuf[sizeof(p->up.password) * 2]; /* for unicode password */
-    uint8_t buf2[128]; /* decoded reply from proxy */
     uint8_t phase3[464];
 
     uint8_t md4_hash[MD4_DIGEST_LENGTH + 5];
@@ -230,8 +229,6 @@  ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2,
 
     bool ntlmv2_enabled = (p->auth_method == HTTP_AUTH_NTLM2);
 
-    CLEAR(buf2);
-
     ASSERT(strlen(p->up.username) > 0);
     ASSERT(strlen(p->up.password) > 0);
 
@@ -264,6 +261,12 @@  ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2,
     /* pad to 21 bytes */
     memset(md4_hash + MD4_DIGEST_LENGTH, 0, 5);
 
+    /* If the decoded challenge is shorter than required by the protocol,
+     * the missing bytes will be NULL, as buf2 is known to be zeroed
+     * when this decode happens.
+     */
+    uint8_t buf2[128]; /* decoded reply from proxy */
+    CLEAR(buf2);
     ret_val = openvpn_base64_decode(phase_2, buf2, -1);
     if (ret_val < 0)
     {