Attachment 17543 Details for Bug 15134 – patch (v1) for 4.12, 4.15 (and earlier versions)

[patch] patch (v1) for 4.12, 4.15 (and earlier versions)

des3-overflow-v1-4.15.patch (text/plain), 5.00 KB, created by Andrew Bartlett on 2022-10-06 04:51:41 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Bartlett

Created: 2022-10-06 04:51:41 UTC

Size: 5.00 KB

patch

obsolete

>From f6afdf6b1395d1c7b87f4bd83653a5baa5fd5e67 Mon Sep 17 00:00:00 2001
>From: Joseph Sutton <josephsutton@catalyst.net.nz>
>Date: Mon, 15 Aug 2022 16:53:45 +1200
>Subject: [PATCH 1/3] lib/gssapi/krb5: Avoid undefined behaviour in
> _gssapi_verify_pad()
>
>By decrementing 'pad' only when we know it's safe, we ensure we can't
>stray backwards past the start of a buffer, which would be undefined
>behaviour.
>
>In the previous version of the loop, 'i' is the number of bytes left to
>check, and 'pad' is the current byte we're checking. 'pad' was
>decremented at the end of each loop iteration. If 'i' was 1 (so we
>checked the final byte), 'pad' could potentially be pointing to the
>first byte of the input buffer, and the decrement would put it one
>byte behind the buffer.
>
>That would be undefined behaviour.
>
>The patch changes it so that 'pad' is the byte we previously checked,
>which allows us to ensure that we only decrement it when we know we
>have a byte to check.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15134
>
>Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>---
> source4/heimdal/lib/gssapi/krb5/decapsulate.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/source4/heimdal/lib/gssapi/krb5/decapsulate.c b/source4/heimdal/lib/gssapi/krb5/decapsulate.c
>index 86085f56950..4e3fcd659e9 100644
>--- a/source4/heimdal/lib/gssapi/krb5/decapsulate.c
>+++ b/source4/heimdal/lib/gssapi/krb5/decapsulate.c
>@@ -193,13 +193,13 @@ _gssapi_verify_pad(gss_buffer_t wrapped_token,
>     if (wrapped_token->length < 1)
> 	return GSS_S_BAD_MECH;
> 
>-    pad = (u_char *)wrapped_token->value + wrapped_token->length - 1;
>-    padlength = *pad;
>+    pad = (u_char *)wrapped_token->value + wrapped_token->length;
>+    padlength = pad[-1];
> 
>     if (padlength > datalen)
> 	return GSS_S_BAD_MECH;
> 
>-    for (i = padlength; i > 0 && *pad == padlength; i--, pad--)
>+    for (i = padlength; i > 0 && *--pad == padlength; i--)
> 	;
>     if (i != 0)
> 	return GSS_S_BAD_MIC;
>-- 
>2.25.1
>
>
>From 4d41c443b67b938730657d25c3a812a270ec540e Mon Sep 17 00:00:00 2001
>From: Joseph Sutton <josephsutton@catalyst.net.nz>
>Date: Mon, 15 Aug 2022 16:53:55 +1200
>Subject: [PATCH 2/3] lib/gssapi/krb5: Check the result of _gsskrb5_get_mech()
>
>We should make sure that the result of 'total_len - mech_len' won't
>overflow, and that we don't memcmp() past the end of the buffer.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15134
>
>Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>---
> source4/heimdal/lib/gssapi/krb5/decapsulate.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/source4/heimdal/lib/gssapi/krb5/decapsulate.c b/source4/heimdal/lib/gssapi/krb5/decapsulate.c
>index 4e3fcd659e9..031a621eabc 100644
>--- a/source4/heimdal/lib/gssapi/krb5/decapsulate.c
>+++ b/source4/heimdal/lib/gssapi/krb5/decapsulate.c
>@@ -80,6 +80,10 @@ _gssapi_verify_mech_header(u_char **str,
> 
>     if (mech_len != mech->length)
> 	return GSS_S_BAD_MECH;
>+    if (mech_len > total_len)
>+	return GSS_S_BAD_MECH;
>+    if (p - *str > total_len - mech_len)
>+	return GSS_S_BAD_MECH;
>     if (ct_memcmp(p,
> 		  mech->elements,
> 		  mech->length) != 0)
>-- 
>2.25.1
>
>
>From 78214c654bd6689d1813bbea5cfa13e9c78be324 Mon Sep 17 00:00:00 2001
>From: Joseph Sutton <josephsutton@catalyst.net.nz>
>Date: Mon, 15 Aug 2022 16:54:23 +1200
>Subject: [PATCH 3/3] lib/gssapi/krb5: Check buffer length against overflow for
> DES{,3} unwrap
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15134
>
>Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>---
> source4/heimdal/lib/gssapi/krb5/unwrap.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/source4/heimdal/lib/gssapi/krb5/unwrap.c b/source4/heimdal/lib/gssapi/krb5/unwrap.c
>index b3da35ee9e2..b1c09521c4f 100644
>--- a/source4/heimdal/lib/gssapi/krb5/unwrap.c
>+++ b/source4/heimdal/lib/gssapi/krb5/unwrap.c
>@@ -64,6 +64,8 @@ unwrap_des
> 
>   if (IS_DCE_STYLE(context_handle)) {
>      token_len = 22 + 8 + 15; /* 45 */
>+     if (input_message_buffer->length < token_len)
>+	  return GSS_S_BAD_MECH;
>   } else {
>      token_len = input_message_buffer->length;
>   }
>@@ -76,6 +78,11 @@ unwrap_des
>   if (ret)
>       return ret;
> 
>+  len = (p - (u_char *)input_message_buffer->value)
>+      + 22 + 8;
>+  if (len > input_message_buffer->length)
>+      return GSS_S_BAD_MECH;
>+
>   if (memcmp (p, "\x00\x00", 2) != 0)
>     return GSS_S_BAD_SIG;
>   p += 2;
>@@ -215,6 +222,8 @@ unwrap_des3
> 
>   if (IS_DCE_STYLE(context_handle)) {
>      token_len = 34 + 8 + 15; /* 57 */
>+     if (input_message_buffer->length < token_len)
>+	  return GSS_S_BAD_MECH;
>   } else {
>      token_len = input_message_buffer->length;
>   }
>@@ -227,6 +236,11 @@ unwrap_des3
>   if (ret)
>       return ret;
> 
>+  len = (p - (u_char *)input_message_buffer->value)
>+      + 34 + 8;
>+  if (len > input_message_buffer->length)
>+      return GSS_S_BAD_MECH;
>+
>   if (memcmp (p, "\x04\x00", 2) != 0) /* HMAC SHA1 DES3_KD */
>     return GSS_S_BAD_SIG;
>   p += 2;
>-- 
>2.25.1
>

Flags:

jsutton: review+
abartlet: ci-passed+

Actions: View

Attachments on bug 15134: 17455 | 17474 | 17539 | 17540 | 17541 | 17542 | 17543 | 17549 | 17550 | 17551 | 17552 | 17553 | 17554 | 17555 | 17556 | 17557 | 17558 | 17559 | 17561 | 17564 | 17577 | 17598