From 633f2c5d56cfe4f358fdadb38b63efd5c74df991 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Fri, 14 Oct 2022 16:45:37 +1300 Subject: [PATCH] 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 , original by Joseph Sutton . Signed-off-by: Nico Williams [jsutton@samba.org Zero-initialised header_size in krb5_pac_parse() to avoid a maybe-uninitialized error; added a missing check for ret == 0] --- third_party/heimdal/lib/krb5/pac.c | 656 +++++++++++++++--------- third_party/heimdal/lib/krb5/test_pac.c | 48 +- 2 files changed, 465 insertions(+), 239 deletions(-) diff --git a/third_party/heimdal/lib/krb5/pac.c b/third_party/heimdal/lib/krb5/pac.c index c8f355c8179..c11990a1606 100644 --- a/third_party/heimdal/lib/krb5/pac.c +++ b/third_party/heimdal/lib/krb5/pac.c @@ -37,19 +37,34 @@ #include #include +/* + * 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; @@ -133,6 +148,60 @@ struct heim_type_data pac_object = { NULL }; +/* + * 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; - 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) { - 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 (tmp2 != 0) { - ret = EINVAL; /* Wrong version */ - krb5_set_error_message(context, ret, - 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) { - 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; - } - - 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); - - /* consistency checks */ - if (p->pac->buffers[i].offset_lo & (PAC_ALIGNMENT - 1)) { - ret = EINVAL; - krb5_set_error_message(context, ret, - N_("PAC out of alignment", "")); - goto out; - } - 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_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, - 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; - } - - /* 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, - 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, - 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, - 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, - N_("PAC has multiple attributes info buffers", "")); - goto out; - } - p->attributes_info = &p->pac->buffers[i]; - } - } - - 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); - } *pac = NULL; + p = _heim_alloc_object(&pac_object, sizeof(*p)); + if (p) + sp = krb5_storage_from_readonly_mem(ptr, len); + if (sp == NULL) + ret = krb5_enomem(context); + if (ret == 0) { + krb5_storage_set_flags(sp, KRB5_STORAGE_BYTEORDER_LE); + ret = krb5_ret_uint32(sp, &num_buffers); + } + 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)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); + if (ret == 0) { + p->pac->numbuffers = num_buffers; + p->pac->version = version; + } + + 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 (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", "")); + break; + } + 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 < header_size) { + krb5_set_error_message(context, ret = EINVAL, + N_("PAC offset inside header: %lu %lu", ""), + (unsigned long)p->pac->buffers[i].offset, + (unsigned long)header_size); + break; + } + + /* + * 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", "")); + 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", "")); + 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", "")); + 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", "")); + else + p->attributes_info = &p->pac->buffers[i]; + break; + default: break; + } + } + + if (ret == 0) + ret = krb5_data_copy(&p->data, ptr, len); + if (ret == 0) { + *pac = p; + p = 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; } - /* - * 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); + /* 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; /* - * copy in new data part + * Make place for new PAC INFO BUFFER header */ + 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); - 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); - + /* + * Copy in new data part + */ + 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); @@ -1059,7 +1159,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); @@ -1144,11 +1244,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); @@ -1168,7 +1268,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) @@ -1283,13 +1383,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) { @@ -1354,6 +1461,7 @@ _krb5_pac_sign(krb5_context context, } } + /* Count missing-but-necessary buffers */ if (p->logon_name == NULL) num++; if (p->server_checksum == NULL) @@ -1363,35 +1471,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; } } @@ -1425,11 +1543,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; @@ -1438,26 +1581,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); } @@ -1469,7 +1652,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) { @@ -1482,18 +1665,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; } @@ -1593,7 +1775,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"); -- 2.35.0