CVE-2022-42898 third_party/heimdal: PAC parse integer overflows

Catch overflows that result from adding PAC_INFO_BUFFER_SIZE.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15203

Heavily edited by committer Nico Williams <nico@twosigma.com>, original by
Joseph Sutton <josephsutton@catalyst.net.nz>.

Signed-off-by: Nico Williams <nico@twosigma.com>

[jsutton@samba.org Zero-initialised header_size in krb5_pac_parse() to
 avoid a maybe-uninitialized error; added a missing check for ret == 0]

Autobuild-User(master): Jule Anger <janger@samba.org>
Autobuild-Date(master): Tue Nov 15 17:02:52 UTC 2022 on sn-devel-184

2 files changed, 444 insertions, 218 deletions

diff --git a/third_party/heimdal/lib/krb5/pac.c b/third_party/heimdal/lib/krb5/pac.c
index b923981908d..e6dfe2aef3d 100644
--- a/third_party/heimdal/lib/krb5/pac.c
+++ b/third_party/heimdal/lib/krb5/pac.c
@@ -37,19 +37,34 @@
 #include <wind.h>
 #include <assert.h>
 
+/*
+ * https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-pac/3341cfa2-6ef5-42e0-b7bc-4544884bf399
+ */
 struct PAC_INFO_BUFFER {
-    uint32_t type;
-    uint32_t buffersize;
-    uint32_t offset_hi;
-    uint32_t offset_lo;
+    uint32_t type;          /* ULONG   ulType       in the original */
+    uint32_t buffersize;    /* ULONG   cbBufferSize in the original */
+    uint64_t offset;        /* ULONG64 Offset       in the original
+                             * this being the offset from the beginning of the
+                             * struct PACTYPE to the beginning of the buffer
+                             * containing data of type ulType
+                             */
 };
 
+/*
+ * https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-pac/6655b92f-ab06-490b-845d-037e6987275f
+ */
 struct PACTYPE {
-    uint32_t numbuffers;
-    uint32_t version;
-    struct PAC_INFO_BUFFER buffers[1];
+    uint32_t numbuffers;    /* named cBuffers of type ULONG in the original */
+    uint32_t version;       /* Named Version  of type ULONG in the original */
+    struct PAC_INFO_BUFFER buffers[1]; /* an ellipsis (...) in the original */
 };
 
+/*
+ * A PAC starts with a PACTYPE header structure that is followed by an array of
+ * numbuffers PAC_INFO_BUFFER structures, each of which points to a buffer
+ * beyond the last PAC_INFO_BUFFER structures.
+ */
+
 struct krb5_pac_data {
     struct PACTYPE *pac;
     krb5_data data;
@@ -134,6 +149,60 @@ struct heim_type_data pac_object = {
 };
 
 /*
+ * Returns the size of the PACTYPE header + the PAC_INFO_BUFFER array.  This is
+ * also the end of the whole thing, and any offsets to buffers from
+ * thePAC_INFO_BUFFER[] entries have to be beyond it.
+ */
+static krb5_error_code
+pac_header_size(krb5_context context, uint32_t num_buffers, uint32_t *result)
+{
+    krb5_error_code ret;
+    uint32_t header_size;
+
+    /* Guard against integer overflow */
+    if (num_buffers > UINT32_MAX / PAC_INFO_BUFFER_SIZE) {
+	ret = EOVERFLOW;
+	krb5_set_error_message(context, ret, "PAC has too many buffers");
+	return ret;
+    }
+    header_size = PAC_INFO_BUFFER_SIZE * num_buffers;
+
+    /* Guard against integer overflow */
+    if (header_size > UINT32_MAX - PACTYPE_SIZE) {
+	ret = EOVERFLOW;
+	krb5_set_error_message(context, ret, "PAC has too many buffers");
+	return ret;
+    }
+    header_size += PACTYPE_SIZE;
+
+    *result = header_size;
+
+    return 0;
+}
+
+/* Output `size' + `addend' + padding for alignment if it doesn't overflow */
+static krb5_error_code
+pac_aligned_size(krb5_context context,
+                 uint32_t size,
+                 uint32_t addend,
+                 uint32_t *aligned_size)
+{
+    krb5_error_code ret;
+
+    if (size > UINT32_MAX - addend ||
+        (size + addend) > UINT32_MAX - (PAC_ALIGNMENT - 1)) {
+	ret = EOVERFLOW;
+	krb5_set_error_message(context, ret, "integer overrun");
+	return ret;
+    }
+    size += addend;
+    size += PAC_ALIGNMENT - 1;
+    size &= ~(PAC_ALIGNMENT - 1);
+    *aligned_size = size;
+    return 0;
+}
+
+/*
  * HMAC-MD5 checksum over any key (needed for the PAC routines)
  */
 
@@ -184,165 +253,164 @@ KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL
 krb5_pac_parse(krb5_context context, const void *ptr, size_t len,
 	       krb5_pac *pac)
 {
-    krb5_error_code ret;
+    krb5_error_code ret = 0;
     krb5_pac p;
     krb5_storage *sp = NULL;
-    uint32_t i, tmp, tmp2, header_end;
+    uint32_t i, num_buffers, version, header_size = 0;
+    uint32_t prev_start = 0;
+    uint32_t prev_end = 0;
 
+    *pac = NULL;
     p = _heim_alloc_object(&pac_object, sizeof(*p));
-    if (p == NULL) {
-	ret = krb5_enomem(context);
-	goto out;
-    }
-
-    sp = krb5_storage_from_readonly_mem(ptr, len);
-    if (sp == NULL) {
+    if (p)
+        sp = krb5_storage_from_readonly_mem(ptr, len);
+    if (sp == NULL)
 	ret = krb5_enomem(context);
-	goto out;
-    }
-    krb5_storage_set_flags(sp, KRB5_STORAGE_BYTEORDER_LE);
-
-    CHECK(ret, krb5_ret_uint32(sp, &tmp), out);
-    CHECK(ret, krb5_ret_uint32(sp, &tmp2), out);
-    if (tmp < 1) {
-	ret = EINVAL; /* Too few buffers */
-	krb5_set_error_message(context, ret, N_("PAC has too few buffers", ""));
-	goto out;
+    if (ret == 0) {
+        krb5_storage_set_flags(sp, KRB5_STORAGE_BYTEORDER_LE);
+        ret = krb5_ret_uint32(sp, &num_buffers);
     }
-    if (tmp2 != 0) {
-	ret = EINVAL; /* Wrong version */
-	krb5_set_error_message(context, ret,
+    if (ret == 0)
+        ret = krb5_ret_uint32(sp, &version);
+    if (ret == 0 && num_buffers < 1)
+	krb5_set_error_message(context, ret = EINVAL,
+                               N_("PAC has too few buffers", ""));
+    if (ret == 0 && num_buffers > 1000)
+	krb5_set_error_message(context, ret = EINVAL,
+                               N_("PAC has too many buffers", ""));
+    if (ret == 0 && version != 0)
+	krb5_set_error_message(context, ret = EINVAL,
 			       N_("PAC has wrong version %d", ""),
-			       (int)tmp2);
-	goto out;
-    }
-
-    p->pac = calloc(1,
-		    sizeof(*p->pac) + (sizeof(p->pac->buffers[0]) * (tmp - 1)));
-    if (p->pac == NULL) {
+			       (int)version);
+    if (ret == 0)
+        ret = pac_header_size(context, num_buffers, &header_size);
+    if (ret == 0 && header_size > len)
+        krb5_set_error_message(context, ret = EOVERFLOW,
+                               N_("PAC encoding invalid, would overflow buffers", ""));
+    if (ret == 0)
+        p->pac = calloc(1, header_size);
+    if (ret == 0 && p->pac == NULL)
 	ret = krb5_enomem(context);
-	goto out;
-    }
-
-    p->pac->numbuffers = tmp;
-    p->pac->version = tmp2;
 
-    header_end = PACTYPE_SIZE + (PAC_INFO_BUFFER_SIZE * p->pac->numbuffers);
-    if (header_end > len) {
-	ret = EINVAL;
-	goto out;
+    if (ret == 0) {
+        p->pac->numbuffers = num_buffers;
+        p->pac->version = version;
     }
 
-    for (i = 0; i < p->pac->numbuffers; i++) {
-	CHECK(ret, krb5_ret_uint32(sp, &p->pac->buffers[i].type), out);
-	CHECK(ret, krb5_ret_uint32(sp, &p->pac->buffers[i].buffersize), out);
-	CHECK(ret, krb5_ret_uint32(sp, &p->pac->buffers[i].offset_lo), out);
-	CHECK(ret, krb5_ret_uint32(sp, &p->pac->buffers[i].offset_hi), out);
+    for (i = 0; ret == 0 && i < p->pac->numbuffers; i++) {
+        ret = krb5_ret_uint32(sp, &p->pac->buffers[i].type);
+        if (ret == 0)
+            ret = krb5_ret_uint32(sp, &p->pac->buffers[i].buffersize);
+        if (ret == 0)
+            ret = krb5_ret_uint64(sp, &p->pac->buffers[i].offset);
+        if (ret)
+            break;
 
-	/* consistency checks */
-	if (p->pac->buffers[i].offset_lo & (PAC_ALIGNMENT - 1)) {
-	    ret = EINVAL;
-	    krb5_set_error_message(context, ret,
+	/* Consistency checks (we don't check for wasted space) */
+	if (p->pac->buffers[i].offset & (PAC_ALIGNMENT - 1)) {
+	    krb5_set_error_message(context, ret = EINVAL,
 				   N_("PAC out of alignment", ""));
-	    goto out;
+	    break;
 	}
-	if (p->pac->buffers[i].offset_hi) {
-	    ret = EINVAL;
-	    krb5_set_error_message(context, ret,
-				   N_("PAC high offset set", ""));
-	    goto out;
+	if (p->pac->buffers[i].offset > len ||
+            p->pac->buffers[i].buffersize > len ||
+            len - p->pac->buffers[i].offset < p->pac->buffers[i].buffersize) {
+	    krb5_set_error_message(context, ret = EOVERFLOW,
+				   N_("PAC buffer overflow", ""));
+	    break;
 	}
-	if (p->pac->buffers[i].offset_lo > len) {
-	    ret = EINVAL;
-	    krb5_set_error_message(context, ret,
-				   N_("PAC offset overflow", ""));
-	    goto out;
-	}
-	if (p->pac->buffers[i].offset_lo < header_end) {
-	    ret = EINVAL;
-	    krb5_set_error_message(context, ret,
+	if (p->pac->buffers[i].offset < header_size) {
+	    krb5_set_error_message(context, ret = EINVAL,
 				   N_("PAC offset inside header: %lu %lu", ""),
-				   (unsigned long)p->pac->buffers[i].offset_lo,
-				   (unsigned long)header_end);
-	    goto out;
-	}
-	if (p->pac->buffers[i].buffersize > len - p->pac->buffers[i].offset_lo){
-	    ret = EINVAL;
-	    krb5_set_error_message(context, ret, N_("PAC length overflow", ""));
-	    goto out;
+				   (unsigned long)p->pac->buffers[i].offset,
+				   (unsigned long)header_size);
+	    break;
 	}
 
-	/* let save pointer to data we need later */
-	if (p->pac->buffers[i].type == PAC_SERVER_CHECKSUM) {
-	    if (p->server_checksum) {
-		ret = EINVAL;
-		krb5_set_error_message(context, ret,
+        /*
+         * We'd like to check for non-overlapping of buffers, but the buffers
+         * need not be in the same order as the PAC_INFO_BUFFER[] entries
+         * pointing to them!  To fully check for overlap we'd have to have an
+         * O(N^2) loop after we parse all the PAC_INFO_BUFFER[].
+         *
+         * But we can check that each buffer does not overlap the previous
+         * buffer.
+         */
+        if (prev_start) {
+            if (p->pac->buffers[i].offset >= prev_start &&
+                p->pac->buffers[i].offset <  prev_end) {
+                krb5_set_error_message(context, ret = EINVAL,
+                                       N_("PAC overlap", ""));
+                break;
+            }
+            if (p->pac->buffers[i].offset < prev_start &&
+                p->pac->buffers[i].offset +
+                p->pac->buffers[i].buffersize > prev_start) {
+                krb5_set_error_message(context, ret = EINVAL,
+                                       N_("PAC overlap", ""));
+                break;
+            }
+        }
+        prev_start = p->pac->buffers[i].offset;
+        prev_end = p->pac->buffers[i].offset + p->pac->buffers[i].buffersize;
+
+	/* Let's save pointers to buffers we'll need later */
+        switch (p->pac->buffers[i].type) {
+        case PAC_SERVER_CHECKSUM:
+	    if (p->server_checksum)
+		krb5_set_error_message(context, ret = EINVAL,
 				       N_("PAC has multiple server checksums", ""));
-		goto out;
-	    }
-	    p->server_checksum = &p->pac->buffers[i];
-	} else if (p->pac->buffers[i].type == PAC_PRIVSVR_CHECKSUM) {
-	    if (p->privsvr_checksum) {
-		ret = EINVAL;
-		krb5_set_error_message(context, ret,
-				       N_("PAC has multiple KDC checksums", ""));
-		goto out;
-	    }
-	    p->privsvr_checksum = &p->pac->buffers[i];
-	} else if (p->pac->buffers[i].type == PAC_LOGON_NAME) {
-	    if (p->logon_name) {
-		ret = EINVAL;
-		krb5_set_error_message(context, ret,
-				       N_("PAC has multiple logon names", ""));
-		goto out;
-	    }
-	    p->logon_name = &p->pac->buffers[i];
-	} else if (p->pac->buffers[i].type == PAC_UPN_DNS_INFO) {
-	    if (p->upn_dns_info) {
-		ret = EINVAL;
-		krb5_set_error_message(context, ret,
+	    else
+                p->server_checksum = &p->pac->buffers[i];
+            break;
+        case PAC_PRIVSVR_CHECKSUM:
+	    if (p->privsvr_checksum)
+                krb5_set_error_message(context, ret = EINVAL,
+                                       N_("PAC has multiple KDC checksums", ""));
+            else
+                p->privsvr_checksum = &p->pac->buffers[i];
+            break;
+        case PAC_LOGON_NAME:
+	    if (p->logon_name)
+		krb5_set_error_message(context, ret = EINVAL,
+                                       N_("PAC has multiple logon names", ""));
+            else
+                p->logon_name = &p->pac->buffers[i];
+            break;
+        case PAC_UPN_DNS_INFO:
+	    if (p->upn_dns_info)
+		krb5_set_error_message(context, ret = EINVAL,
 				       N_("PAC has multiple UPN DNS info buffers", ""));
-		goto out;
-	    }
-	    p->upn_dns_info = &p->pac->buffers[i];
-	} else if (p->pac->buffers[i].type == PAC_TICKET_CHECKSUM) {
-	    if (p->ticket_checksum) {
-		ret = EINVAL;
-		krb5_set_error_message(context, ret,
+            else
+                p->upn_dns_info = &p->pac->buffers[i];
+            break;
+        case PAC_TICKET_CHECKSUM:
+	    if (p->ticket_checksum)
+		krb5_set_error_message(context, ret = EINVAL,
 				       N_("PAC has multiple ticket checksums", ""));
-		goto out;
-	    }
-	    p->ticket_checksum = &p->pac->buffers[i];
-	} else if (p->pac->buffers[i].type == PAC_ATTRIBUTES_INFO) {
-	    if (p->attributes_info) {
-		ret = EINVAL;
-		krb5_set_error_message(context, ret,
+            else
+                p->ticket_checksum = &p->pac->buffers[i];
+            break;
+        case PAC_ATTRIBUTES_INFO:
+	    if (p->attributes_info)
+		krb5_set_error_message(context, ret = EINVAL,
 				       N_("PAC has multiple attributes info buffers", ""));
-		goto out;
-	    }
-	    p->attributes_info = &p->pac->buffers[i];
-	}
+            else
+                p->attributes_info = &p->pac->buffers[i];
+            break;
+        default: break;
+        }
     }
 
-    ret = krb5_data_copy(&p->data, ptr, len);
-    if (ret)
-	goto out;
-
-    krb5_storage_free(sp);
-
-    *pac = p;
-    return 0;
-
-out:
-    if (sp)
-	krb5_storage_free(sp);
-    if (p) {
-	if (p->pac)
-	    free(p->pac);
-	krb5_pac_free(context, p);
+    if (ret == 0)
+        ret = krb5_data_copy(&p->data, ptr, len);
+    if (ret == 0) {
+        *pac = p;
+        p = NULL;
     }
-    *pac = NULL;
-
+    if (sp)
+        krb5_storage_free(sp);
+    krb5_pac_free(context, p);
     return ret;
 }
 
@@ -369,77 +437,111 @@ krb5_pac_init(krb5_context context, krb5_pac *pac)
 	krb5_pac_free(context, p);
 	return krb5_enomem(context);
     }
+    memset(p->data.data, 0, p->data.length);
 
     *pac = p;
     return 0;
 }
 
+/**
+ * Add a PAC buffer `nd' of type `type' to the pac `p'.
+ *
+ * @param context
+ * @param p
+ * @param type
+ * @param nd
+ *
+ * @return 0 on success or a Kerberos or system error.
+ */
 KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL
 krb5_pac_add_buffer(krb5_context context, krb5_pac p,
-		    uint32_t type, const krb5_data *data)
+		    uint32_t type, const krb5_data *nd)
 {
     krb5_error_code ret;
     void *ptr;
-    size_t len, offset, header_end, old_end;
+    size_t old_len = p->data.length;
+    uint32_t len, offset, header_size;
     uint32_t i;
+    uint32_t num_buffers;
 
-    assert(data->data != NULL);
+    assert(nd->data != NULL);
 
-    len = p->pac->numbuffers;
+    num_buffers = p->pac->numbuffers;
+    ret = pac_header_size(context, num_buffers + 1, &header_size);
+    if (ret)
+	return ret;
 
-    ptr = realloc(p->pac,
-		  sizeof(*p->pac) + (sizeof(p->pac->buffers[0]) * len));
+    ptr = realloc(p->pac, header_size);
     if (ptr == NULL)
 	return krb5_enomem(context);
 
     p->pac = ptr;
+    p->pac->buffers[num_buffers].type = 0;
+    p->pac->buffers[num_buffers].buffersize = 0;
+    p->pac->buffers[num_buffers].offset = 0;
 
-    for (i = 0; i < len; i++)
-	p->pac->buffers[i].offset_lo += PAC_INFO_BUFFER_SIZE;
-
-    offset = p->data.length + PAC_INFO_BUFFER_SIZE;
-
-    p->pac->buffers[len].type = type;
-    p->pac->buffers[len].buffersize = data->length;
-    p->pac->buffers[len].offset_lo = offset;
-    p->pac->buffers[len].offset_hi = 0;
-
-    old_end = p->data.length;
-    len = p->data.length + data->length + PAC_INFO_BUFFER_SIZE;
-    if (len < p->data.length) {
-	krb5_set_error_message(context, EINVAL, "integer overrun");
-	return EINVAL;
+    /*
+     * Check that we can adjust all the buffer offsets in the existing
+     * PAC_INFO_BUFFERs, since changing the size of PAC_INFO_BUFFER[] means
+     * changing the offsets of buffers following that array.
+     *
+     * We don't adjust them until we can't fail.
+     */
+    for (i = 0; i < num_buffers; i++) {
+	if (p->pac->buffers[i].offset > UINT32_MAX - PAC_INFO_BUFFER_SIZE) {
+	    krb5_set_error_message(context, ret = EOVERFLOW,
+                                   "too many / too large PAC buffers");
+	    return ret;
+	}
     }
 
-    /* align to PAC_ALIGNMENT */
-    len = ((len + PAC_ALIGNMENT - 1) / PAC_ALIGNMENT) * PAC_ALIGNMENT;
+    /*
+     * The new buffer's offset must be past the end of the buffers we have
+     * (p->data), which is the sum of the header and p->data.length.
+     */
 
+    /* Set offset = p->data.length + PAC_INFO_BUFFER_SIZE + alignment */
+    ret = pac_aligned_size(context, p->data.length, PAC_INFO_BUFFER_SIZE, &offset);
+    if (ret == 0)
+        /* Set the new length = offset + nd->length + alignment */
+        ret = pac_aligned_size(context, offset, nd->length, &len);
+    if (ret) {
+	krb5_set_error_message(context, ret, "PAC buffer too large");
+        return ret;
+    }
     ret = krb5_data_realloc(&p->data, len);
     if (ret) {
 	krb5_set_error_message(context, ret, N_("malloc: out of memory", ""));
 	return ret;
     }
 
+    /* Zero out the new allocation to zero out any padding */
+    memset((char *)p->data.data + old_len, 0, len - old_len);
+
+    p->pac->buffers[num_buffers].type = type;
+    p->pac->buffers[num_buffers].buffersize = nd->length;
+    p->pac->buffers[num_buffers].offset = offset;
+
+    /* Adjust all the buffer offsets in the existing PAC_INFO_BUFFERs now */
+    for (i = 0; i < num_buffers; i++)
+	p->pac->buffers[i].offset += PAC_INFO_BUFFER_SIZE;
+
     /*
-     * make place for new PAC INFO BUFFER header
+     * Make place for new PAC INFO BUFFER header
      */
-    header_end = PACTYPE_SIZE + (PAC_INFO_BUFFER_SIZE * p->pac->numbuffers);
-    memmove((unsigned char *)p->data.data + header_end + PAC_INFO_BUFFER_SIZE,
-	    (unsigned char *)p->data.data + header_end ,
-	    old_end - header_end);
-    memset((unsigned char *)p->data.data + header_end, 0, PAC_INFO_BUFFER_SIZE);
+    header_size -= PAC_INFO_BUFFER_SIZE;
+    memmove((unsigned char *)p->data.data + header_size + PAC_INFO_BUFFER_SIZE,
+	    (unsigned char *)p->data.data + header_size ,
+	    old_len - header_size);
+    /* Clear the space where we would put the new PAC_INFO_BUFFER[] element */
+    memset((unsigned char *)p->data.data + header_size, 0,
+           PAC_INFO_BUFFER_SIZE);
 
     /*
-     * copy in new data part
+     * Copy in new data part
      */
-
-    memcpy((unsigned char *)p->data.data + offset,
-	   data->data, data->length);
-    memset((unsigned char *)p->data.data + offset + data->length,
-	   0, p->data.length - offset - data->length);
-
+    memcpy((unsigned char *)p->data.data + offset, nd->data, nd->length);
     p->pac->numbuffers += 1;
-
     return 0;
 }
 
@@ -451,8 +553,8 @@ krb5_pac_add_buffer(krb5_context context, krb5_pac p,
  * @param type type of buffer to get
  * @param data return data, free with krb5_data_free().
  *
- * @return Returns 0 to indicate success. Otherwise an kerberos et
- * error code is returned, see krb5_get_error_message().
+ * @return Returns 0 to indicate success, ENOENT to indicate that a buffer of
+ * the given type was not found, or a Kerberos or system error code.
  *
  * @ingroup krb5_pac
  */
@@ -465,21 +567,19 @@ krb5_pac_get_buffer(krb5_context context, krb5_const_pac p,
     uint32_t i;
 
     for (i = 0; i < p->pac->numbuffers; i++) {
-	const size_t len = p->pac->buffers[i].buffersize;
-	const size_t offset = p->pac->buffers[i].offset_lo;
+	size_t len = p->pac->buffers[i].buffersize;
+	size_t offset = p->pac->buffers[i].offset;
 
 	if (p->pac->buffers[i].type != type)
 	    continue;
 
-	if (data) {
-	    ret = krb5_data_copy(data, (unsigned char *)p->data.data + offset, len);
-	    if (ret) {
-		krb5_set_error_message(context, ret, N_("malloc: out of memory", ""));
-		return ret;
-	    }
-	}
+	if (!data)
+            return 0;
 
-	return 0;
+        ret = krb5_data_copy(data, (unsigned char *)p->data.data + offset, len);
+        if (ret)
+            krb5_set_error_message(context, ret, N_("malloc: out of memory", ""));
+	return ret;
     }
     krb5_set_error_message(context, ENOENT, "No PAC buffer of type %lu was found",
 			   (unsigned long)type);
@@ -578,7 +678,7 @@ verify_checksum(krb5_context context,
 
     memset(&cksum, 0, sizeof(cksum));
 
-    sp = krb5_storage_from_mem((char *)data->data + sig->offset_lo,
+    sp = krb5_storage_from_mem((char *)data->data + sig->offset,
 			       sig->buffersize);
     if (sp == NULL)
 	return krb5_enomem(context);
@@ -738,7 +838,7 @@ parse_upn_dns_info(krb5_context context,
     *canon_princ = NULL;
     krb5_data_zero(sid);
 
-    sp = krb5_storage_from_readonly_mem((const char *)data->data + upndnsinfo->offset_lo,
+    sp = krb5_storage_from_readonly_mem((const char *)data->data + upndnsinfo->offset,
 					upndnsinfo->buffersize);
     if (sp == NULL)
 	return krb5_enomem(context);
@@ -842,7 +942,7 @@ verify_logonname(krb5_context context,
     char *principal_string = NULL;
     char *logon_string = NULL;
 
-    sp = krb5_storage_from_readonly_mem((const char *)data->data + logon_name->offset_lo,
+    sp = krb5_storage_from_readonly_mem((const char *)data->data + logon_name->offset,
 					logon_name->buffersize);
     if (sp == NULL)
 	return krb5_enomem(context);
@@ -1062,7 +1162,7 @@ parse_attributes_info(krb5_context context,
 
     *pac_attributes = 0;
 
-    sp = krb5_storage_from_readonly_mem((const char *)data->data + attributes_info->offset_lo,
+    sp = krb5_storage_from_readonly_mem((const char *)data->data + attributes_info->offset,
 					attributes_info->buffersize);
     if (sp == NULL)
 	return krb5_enomem(context);
@@ -1147,11 +1247,11 @@ krb5_pac_verify(krb5_context context,
 	if (ret)
 	    return ret;
 
-	memset((char *)copy->data + pac->server_checksum->offset_lo + 4,
+	memset((char *)copy->data + pac->server_checksum->offset + 4,
 	       0,
 	       pac->server_checksum->buffersize - 4);
 
-	memset((char *)copy->data + pac->privsvr_checksum->offset_lo + 4,
+	memset((char *)copy->data + pac->privsvr_checksum->offset + 4,
 	       0,
 	       pac->privsvr_checksum->buffersize - 4);
 
@@ -1171,7 +1271,7 @@ krb5_pac_verify(krb5_context context,
 			      pac->privsvr_checksum,
 			      &pac->data,
 			      (char *)pac->data.data
-			      + pac->server_checksum->offset_lo + 4,
+			      + pac->server_checksum->offset + 4,
 			      pac->server_checksum->buffersize - 4,
 			      privsvr);
 	if (ret)
@@ -1286,13 +1386,20 @@ _krb5_pac_sign(krb5_context context,
     size_t server_size, priv_size;
     uint32_t server_offset = 0, priv_offset = 0, ticket_offset = 0;
     uint32_t server_cksumtype = 0, priv_cksumtype = 0;
-    int num = 0;
-    size_t i, sz;
+    uint32_t num = 0;
+    uint32_t i, sz;
     krb5_data logon, d;
 
     krb5_data_zero(&d);
     krb5_data_zero(&logon);
 
+    /*
+     * Set convenience buffer pointers.
+     *
+     * This could really stand to be moved to krb5_pac_add_buffer() and/or
+     * utility function, so that when this function gets called they must
+     * already have been set.
+     */
     for (i = 0; i < p->pac->numbuffers; i++) {
 	if (p->pac->buffers[i].type == PAC_SERVER_CHECKSUM) {
 	    if (p->server_checksum == NULL) {
@@ -1357,6 +1464,7 @@ _krb5_pac_sign(krb5_context context,
 	}
     }
 
+    /* Count missing-but-necessary buffers */
     if (p->logon_name == NULL)
 	num++;
     if (p->server_checksum == NULL)
@@ -1366,35 +1474,45 @@ _krb5_pac_sign(krb5_context context,
     if (p->ticket_sign_data.length != 0 && p->ticket_checksum == NULL)
 	num++;
 
+    /* Allocate any missing-but-necessary buffers */
     if (num) {
 	void *ptr;
+	uint32_t old_len, len;
 
-	ptr = realloc(p->pac, sizeof(*p->pac) + (sizeof(p->pac->buffers[0]) * (p->pac->numbuffers + num - 1)));
+	if (p->pac->numbuffers > UINT32_MAX - num) {
+	    ret = EINVAL;
+	    krb5_set_error_message(context, ret, "integer overrun");
+	    goto out;
+	}
+	ret = pac_header_size(context, p->pac->numbuffers, &old_len);
+        if (ret == 0)
+            ret = pac_header_size(context, p->pac->numbuffers + num, &len);
+	if (ret)
+	    goto out;
+
+	ptr = realloc(p->pac, len);
 	if (ptr == NULL) {
 	    ret = krb5_enomem(context);
             goto out;
         }
-
+        memset((char *)ptr + old_len, 0, len - old_len);
 	p->pac = ptr;
 
+
 	if (p->logon_name == NULL) {
 	    p->logon_name = &p->pac->buffers[p->pac->numbuffers++];
-	    memset(p->logon_name, 0, sizeof(*p->logon_name));
 	    p->logon_name->type = PAC_LOGON_NAME;
 	}
 	if (p->server_checksum == NULL) {
 	    p->server_checksum = &p->pac->buffers[p->pac->numbuffers++];
-	    memset(p->server_checksum, 0, sizeof(*p->server_checksum));
 	    p->server_checksum->type = PAC_SERVER_CHECKSUM;
 	}
 	if (p->privsvr_checksum == NULL) {
 	    p->privsvr_checksum = &p->pac->buffers[p->pac->numbuffers++];
-	    memset(p->privsvr_checksum, 0, sizeof(*p->privsvr_checksum));
 	    p->privsvr_checksum->type = PAC_PRIVSVR_CHECKSUM;
 	}
 	if (p->ticket_sign_data.length != 0 && p->ticket_checksum == NULL) {
 	    p->ticket_checksum = &p->pac->buffers[p->pac->numbuffers++];
-	    memset(p->ticket_checksum, 0, sizeof(*p->ticket_checksum));
 	    p->ticket_checksum->type = PAC_TICKET_CHECKSUM;
 	}
     }
@@ -1428,11 +1546,36 @@ _krb5_pac_sign(krb5_context context,
 
     krb5_storage_set_flags(spdata, KRB5_STORAGE_BYTEORDER_LE);
 
+    /* `sp' has the header, `spdata' has the buffers */
     CHECK(ret, krb5_store_uint32(sp, p->pac->numbuffers), out);
     CHECK(ret, krb5_store_uint32(sp, p->pac->version), out);
 
-    end = PACTYPE_SIZE + (PAC_INFO_BUFFER_SIZE * p->pac->numbuffers);
+    ret = pac_header_size(context, p->pac->numbuffers, &end);
+    if (ret)
+        goto out;
 
+    /*
+     * For each buffer we write its contents to `spdata' and then append the
+     * PAC_INFO_BUFFER for that buffer into the header in `sp'.  The logical
+     * end of the whole thing is kept in `end', which functions as the offset
+     * to write in the buffer's PAC_INFO_BUFFER, then we update it at the
+     * bottom so that the next buffer can be written there.
+     *
+     * TODO?  Maybe rewrite all of this so that:
+     *
+     *  - we use krb5_pac_add_buffer() to add the buffers we produce
+     *  - we use the krb5_data of the concatenated buffers that's maintained by
+     *    krb5_pac_add_buffer() so we don't need `spdata' here
+     *
+     * We do way too much here, and that makes this code hard to read.  Plus we
+     * throw away all the work done in krb5_pac_add_buffer().  On the other
+     * hand, krb5_pac_add_buffer() has to loop over all the buffers, so if we
+     * call krb5_pac_add_buffer() here in a loop, we'll be accidentally
+     * quadratic, but we only need to loop over adding the buffers we add,
+     * which is very few, so not quite quadratic.  We should also cap the
+     * number of buffers we're willing to accept in a PAC we parse to something
+     * reasonable, like a few tens.
+     */
     for (i = 0; i < p->pac->numbuffers; i++) {
 	uint32_t len;
 	size_t sret;
@@ -1441,26 +1584,66 @@ _krb5_pac_sign(krb5_context context,
 	/* store data */
 
 	if (p->pac->buffers[i].type == PAC_SERVER_CHECKSUM) {
+	    if (server_size > UINT32_MAX - 4) {
+		ret = EINVAL;
+		krb5_set_error_message(context, ret, "integer overrun");
+		goto out;
+	    }
 	    len = server_size + 4;
+	    if (end > UINT32_MAX - 4) {
+		ret = EINVAL;
+		krb5_set_error_message(context, ret, "integer overrun");
+		goto out;
+	    }
 	    server_offset = end + 4;
 	    CHECK(ret, krb5_store_uint32(spdata, server_cksumtype), out);
 	    CHECK(ret, fill_zeros(context, spdata, server_size), out);
 	} else if (p->pac->buffers[i].type == PAC_PRIVSVR_CHECKSUM) {
+	    if (priv_size > UINT32_MAX - 4) {
+		ret = EINVAL;
+		krb5_set_error_message(context, ret, "integer overrun");
+		goto out;
+	    }
 	    len = priv_size + 4;
+	    if (end > UINT32_MAX - 4) {
+		ret = EINVAL;
+		krb5_set_error_message(context, ret, "integer overrun");
+		goto out;
+	    }
 	    priv_offset = end + 4;
 	    CHECK(ret, krb5_store_uint32(spdata, priv_cksumtype), out);
 	    CHECK(ret, fill_zeros(context, spdata, priv_size), out);
 	    if (rodc_id != 0) {
+		if (len > UINT32_MAX - sizeof(rodc_id)) {
+		    ret = EINVAL;
+		    krb5_set_error_message(context, ret, "integer overrun");
+		    goto out;
+		}
 		len += sizeof(rodc_id);
 		CHECK(ret, fill_zeros(context, spdata, sizeof(rodc_id)), out);
 	    }
 	} else if (p->ticket_sign_data.length != 0 &&
 		   p->pac->buffers[i].type == PAC_TICKET_CHECKSUM) {
+	    if (priv_size > UINT32_MAX - 4) {
+		ret = EINVAL;
+		krb5_set_error_message(context, ret, "integer overrun");
+		goto out;
+	    }
 	    len = priv_size + 4;
+	    if (end > UINT32_MAX - 4) {
+		ret = EINVAL;
+		krb5_set_error_message(context, ret, "integer overrun");
+		goto out;
+	    }
 	    ticket_offset = end + 4;
 	    CHECK(ret, krb5_store_uint32(spdata, priv_cksumtype), out);
 	    CHECK(ret, fill_zeros(context, spdata, priv_size), out);
 	    if (rodc_id != 0) {
+		if (len > UINT32_MAX - sizeof(rodc_id)) {
+		    ret = EINVAL;
+		    krb5_set_error_message(context, ret, "integer overrun");
+		    goto out;
+		}
 		len += sizeof(rodc_id);
 		CHECK(ret, krb5_store_uint16(spdata, rodc_id), out);
 	    }
@@ -1472,7 +1655,7 @@ _krb5_pac_sign(krb5_context context,
 	    }
 	} else {
 	    len = p->pac->buffers[i].buffersize;
-	    ptr = (char *)p->data.data + p->pac->buffers[i].offset_lo;
+	    ptr = (char *)p->data.data + p->pac->buffers[i].offset;
 
 	    sret = krb5_storage_write(spdata, ptr, len);
 	    if (sret != len) {
@@ -1485,18 +1668,17 @@ _krb5_pac_sign(krb5_context context,
 	/* write header */
 	CHECK(ret, krb5_store_uint32(sp, p->pac->buffers[i].type), out);
 	CHECK(ret, krb5_store_uint32(sp, len), out);
-	CHECK(ret, krb5_store_uint32(sp, end), out);
-	CHECK(ret, krb5_store_uint32(sp, 0), out);
+	CHECK(ret, krb5_store_uint64(sp, end), out); /* offset */
 
 	/* advance data endpointer and align */
 	{
-	    int32_t e;
+	    uint32_t e;
 
-	    end += len;
-	    e = ((end + PAC_ALIGNMENT - 1) / PAC_ALIGNMENT) * PAC_ALIGNMENT;
-	    if ((int32_t)end != e) {
-		CHECK(ret, fill_zeros(context, spdata, e - end), out);
-	    }
+	    ret = pac_aligned_size(context, end, len, &e);
+            if (ret == 0 && end + len != e)
+                ret = fill_zeros(context, spdata, e - (end + len));
+	    if (ret)
+		goto out;
 	    end = e;
 	}
 
@@ -1596,7 +1778,7 @@ krb5_pac_get_kdc_checksum_info(krb5_context context,
 	return KRB5KDC_ERR_BADOPTION;
     }
 
-    sp = krb5_storage_from_mem((char *)pac->data.data + sig->offset_lo,
+    sp = krb5_storage_from_mem((char *)pac->data.data + sig->offset,
 			       sig->buffersize);
     if (sp == NULL)
 	return krb5_enomem(context);
diff --git a/third_party/heimdal/lib/krb5/test_pac.c b/third_party/heimdal/lib/krb5/test_pac.c
index 51b4fff16fc..70da1cb6266 100644
--- a/third_party/heimdal/lib/krb5/test_pac.c
+++ b/third_party/heimdal/lib/krb5/test_pac.c
@@ -838,6 +838,7 @@ check_ticket_signature(krb5_context context,
     krb5_crypto crypto;
     krb5_data data, orig_pac_ad;
     Ticket ticket;
+    AuthorizationDataElement ad;
     EncTicketPart et;
     krb5_principal client;
     krb5_pac pac;
@@ -889,8 +890,10 @@ check_ticket_signature(krb5_context context,
     heim_assert(rodc_id == tkt->rodc_id, "Wrong RODCIdentifier");
 
     /* Try to resign the PAC */
-    orig_pac_ad = et.authorization_data->val[0].ad_data;
-    et.authorization_data->val[0].ad_data.data = NULL;
+    ret = copy_AuthorizationDataElement(&et.authorization_data->val[0], &ad);
+    if (ret)
+	t_err(context, tkt->name, "remove_AuthorizationData", ret);
+    orig_pac_ad = ad.ad_data;
 
     ret = remove_AuthorizationData(et.authorization_data, 0);
     if (ret)
@@ -905,6 +908,8 @@ check_ticket_signature(krb5_context context,
     heim_assert(krb5_data_cmp(&et.authorization_data->val[0].ad_data,
 			      &orig_pac_ad) == 0, "PACs differ");
 
+    free_AuthorizationDataElement(&ad);
+
     /* Sign and verify a clean PAC */
     krb5_pac_free(context, pac);
     ret = krb5_pac_init(context, &pac);
@@ -958,6 +963,7 @@ main(int argc, char **argv)
     krb5_pac pac;
     krb5_data data;
     krb5_principal p, p2;
+    unsigned char bad_pac[sizeof(saved_pac)];
 
     ret = krb5_init_context(&context);
     if (ret)
@@ -970,6 +976,44 @@ main(int argc, char **argv)
     if (ret)
 	krb5_err(context, 1, ret, "krb5_parse_name");
 
+    /* Attempt to parse a truncated PAC */
+    ret = krb5_pac_parse(context, saved_pac, sizeof(saved_pac) >> 1, &pac);
+    if (ret == 0)
+	krb5_err(context, 1, ret, "krb5_pac_parse parsed a short PAC");
+
+    /* Attempt to parse a PAC with a buffer claiming too large a length */
+    memcpy(bad_pac, saved_pac, sizeof(saved_pac));
+    bad_pac[13] += 1;
+
+    ret = krb5_pac_parse(context, bad_pac, sizeof(saved_pac), &pac);
+    if (ret == 0)
+	krb5_err(context, 1, ret, "krb5_pac_parse parsed a malicious PAC");
+
+    /* Attempt to parse a PAC with a buffer offset too far in */
+    memcpy(bad_pac, saved_pac, sizeof(saved_pac));
+    bad_pac[16] += 1;
+
+    ret = krb5_pac_parse(context, bad_pac, sizeof(saved_pac), &pac);
+    if (ret == 0)
+	krb5_err(context, 1, ret, "krb5_pac_parse parsed a malicious PAC");
+
+    /* Attempt to parse a PAC with a buffer offset too far back */
+    memcpy(bad_pac, saved_pac, sizeof(saved_pac));
+    bad_pac[16] -= 1;
+
+    ret = krb5_pac_parse(context, bad_pac, sizeof(saved_pac), &pac);
+    if (ret == 0)
+	krb5_err(context, 1, ret, "krb5_pac_parse parsed a malicious PAC");
+
+    /* Attempt to parse a PAC with an incorrect buffer count */
+    memcpy(bad_pac, saved_pac, sizeof(saved_pac));
+    bad_pac[0] += 1;
+
+    ret = krb5_pac_parse(context, bad_pac, sizeof(saved_pac), &pac);
+    if (ret == 0)
+	krb5_err(context, 1, ret, "krb5_pac_parse parsed a malicious PAC");
+
+    /* Parse a well-formed PAC */
     ret = krb5_pac_parse(context, saved_pac, sizeof(saved_pac), &pac);
     if (ret)
 	krb5_err(context, 1, ret, "krb5_pac_parse");