From 90fbe4af03e9063cb29a1d026d18fefd6c767c7d Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Thu, 21 May 2015 14:05:31 -0500 Subject: [PATCH 1/4] CVE-2022-42898 source4/heimdal: Add bswap64() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15203 [jsutton@samba.org backported from Heimdal commit 0271b171e5331f0f562319b887f5f0b058ecc9b4; removed changes to cf/roken-frag.m4 that we don't have] --- source4/heimdal/lib/roken/bswap.c | 17 +++++++++++++++++ source4/heimdal/lib/roken/roken.h.in | 5 +++++ source4/heimdal/lib/roken/version-script.map | 1 + 3 files changed, 23 insertions(+) diff --git a/source4/heimdal/lib/roken/bswap.c b/source4/heimdal/lib/roken/bswap.c index 7f8c1c22b1b..b0c4248da11 100644 --- a/source4/heimdal/lib/roken/bswap.c +++ b/source4/heimdal/lib/roken/bswap.c @@ -34,6 +34,23 @@ #include #include "roken.h" +#ifndef HAVE_BSWAP64 + +ROKEN_LIB_FUNCTION uint64_t ROKEN_LIB_CALL +bswap64 (uint64_t val) +{ + return + (val & 0xffULL) << 56 | + (val & 0xff00ULL) << 40 | + (val & 0xff0000ULL) << 24 | + (val & 0xff000000ULL) << 8 | + (val & 0xff00000000ULL) >> 8 | + (val & 0xff0000000000ULL) >> 24 | + (val & 0xff000000000000ULL) >> 40 | + (val & 0xff00000000000000ULL) >> 56 ; +} +#endif + #ifndef HAVE_BSWAP32 ROKEN_LIB_FUNCTION unsigned int ROKEN_LIB_CALL diff --git a/source4/heimdal/lib/roken/roken.h.in b/source4/heimdal/lib/roken/roken.h.in index a6299aee8e5..13a5d943ba2 100644 --- a/source4/heimdal/lib/roken/roken.h.in +++ b/source4/heimdal/lib/roken/roken.h.in @@ -696,6 +696,11 @@ ROKEN_LIB_FUNCTION void ROKEN_LIB_CALL pidfile (const char*); #endif #endif +#ifndef HAVE_BSWAP64 +#define bswap64 rk_bswap64 +ROKEN_LIB_FUNCTION uint64_t ROKEN_LIB_CALL bswap64(uint64_t); +#endif + #ifndef HAVE_BSWAP32 #define bswap32 rk_bswap32 ROKEN_LIB_FUNCTION unsigned int ROKEN_LIB_CALL bswap32(unsigned int); diff --git a/source4/heimdal/lib/roken/version-script.map b/source4/heimdal/lib/roken/version-script.map index 9229a373cd7..34b42a3639f 100644 --- a/source4/heimdal/lib/roken/version-script.map +++ b/source4/heimdal/lib/roken/version-script.map @@ -38,6 +38,7 @@ HEIMDAL_ROKEN_1.0 { rk_asprintf; rk_bswap16; rk_bswap32; + rk_bswap64; rk_cgetent; rk_cgetstr; rk_cloexec; -- 2.35.0 From a1a2eeb278cd0b504c9229cd7152f74463232fc4 Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Thu, 21 May 2015 14:24:38 -0500 Subject: [PATCH 2/4] CVE-2022-42898 source4/heimdal: Add krb5_ret/store_[u]int64() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15203 [jsutton@samba.org backported from Heimdal commit 996d4c5db3c8aee10b7496591db13f52a575cef5; removed changes to lib/krb5/libkrb5-exports.def.in which we don't have] --- source4/heimdal/lib/krb5/store-int.c | 13 +- source4/heimdal/lib/krb5/store.c | 132 +++++++++++++++++--- source4/heimdal/lib/krb5/version-script.map | 4 + 3 files changed, 133 insertions(+), 16 deletions(-) diff --git a/source4/heimdal/lib/krb5/store-int.c b/source4/heimdal/lib/krb5/store-int.c index d5776297181..542b99abc08 100644 --- a/source4/heimdal/lib/krb5/store-int.c +++ b/source4/heimdal/lib/krb5/store-int.c @@ -34,7 +34,7 @@ #include "krb5_locl.h" KRB5_LIB_FUNCTION krb5_ssize_t KRB5_LIB_CALL -_krb5_put_int(void *buffer, unsigned long value, size_t size) +_krb5_put_int(void *buffer, uint64_t value, size_t size) { unsigned char *p = buffer; int i; @@ -46,7 +46,7 @@ _krb5_put_int(void *buffer, unsigned long value, size_t size) } KRB5_LIB_FUNCTION krb5_ssize_t KRB5_LIB_CALL -_krb5_get_int(void *buffer, unsigned long *value, size_t size) +_krb5_get_int64(void *buffer, uint64_t *value, size_t size) { unsigned char *p = buffer; unsigned long v = 0; @@ -56,3 +56,12 @@ _krb5_get_int(void *buffer, unsigned long *value, size_t size) *value = v; return size; } + +KRB5_LIB_FUNCTION krb5_ssize_t KRB5_LIB_CALL +_krb5_get_int(void *buffer, unsigned long *value, size_t size) +{ + uint64_t v64; + krb5_ssize_t bytes = _krb5_get_int64(buffer, &v64, size); + *value = v64; + return bytes; +} diff --git a/source4/heimdal/lib/krb5/store.c b/source4/heimdal/lib/krb5/store.c index 31afb23c983..df0227b39de 100644 --- a/source4/heimdal/lib/krb5/store.c +++ b/source4/heimdal/lib/krb5/store.c @@ -318,13 +318,13 @@ krb5_storage_to_data(krb5_storage *sp, krb5_data *data) static krb5_error_code krb5_store_int(krb5_storage *sp, - int32_t value, + int64_t value, size_t len) { int ret; - unsigned char v[16]; + unsigned char v[8]; - if(len > sizeof(v)) + if (len > sizeof(v)) return EINVAL; _krb5_put_int(v, value, len); ret = sp->store(sp, v, len); @@ -358,6 +358,33 @@ krb5_store_int32(krb5_storage *sp, return krb5_store_int(sp, value, 4); } +/** + * Store a int64 to storage, byte order is controlled by the settings + * on the storage, see krb5_storage_set_byteorder(). + * + * @param sp the storage to write too + * @param value the value to store + * + * @return 0 for success, or a Kerberos 5 error code on failure. + * + * @ingroup krb5_storage + */ + +KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL +krb5_store_int64(krb5_storage *sp, + int64_t value) +{ + if (BYTEORDER_IS_HOST(sp)) +#ifdef WORDS_BIGENDIAN + ; +#else + value = bswap64(value); /* There's no ntohll() */ +#endif + else if (BYTEORDER_IS_LE(sp)) + value = bswap64(value); + return krb5_store_int(sp, value, 8); +} + /** * Store a uint32 to storage, byte order is controlled by the settings * on the storage, see krb5_storage_set_byteorder(). @@ -377,24 +404,99 @@ krb5_store_uint32(krb5_storage *sp, return krb5_store_int32(sp, (int32_t)value); } +/** + * Store a uint64 to storage, byte order is controlled by the settings + * on the storage, see krb5_storage_set_byteorder(). + * + * @param sp the storage to write too + * @param value the value to store + * + * @return 0 for success, or a Kerberos 5 error code on failure. + * + * @ingroup krb5_storage + */ + +KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL +krb5_store_uint64(krb5_storage *sp, + uint64_t value) +{ + return krb5_store_int64(sp, (int64_t)value); +} + static krb5_error_code krb5_ret_int(krb5_storage *sp, - int32_t *value, + int64_t *value, size_t len) { int ret; - unsigned char v[4]; - unsigned long w; + unsigned char v[8]; + uint64_t w; ret = sp->fetch(sp, v, len); if (ret < 0) return errno; if ((size_t)ret != len) return sp->eof_code; - _krb5_get_int(v, &w, len); + _krb5_get_int64(v, &w, len); *value = w; return 0; } +/** + * Read a int64 from storage, byte order is controlled by the settings + * on the storage, see krb5_storage_set_byteorder(). + * + * @param sp the storage to write too + * @param value the value read from the buffer + * + * @return 0 for success, or a Kerberos 5 error code on failure. + * + * @ingroup krb5_storage + */ + +KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL +krb5_ret_int64(krb5_storage *sp, + int64_t *value) +{ + krb5_error_code ret = krb5_ret_int(sp, value, 8); + if(ret) + return ret; + if(BYTEORDER_IS_HOST(sp)) +#ifdef WORDS_BIGENDIAN + ; +#else + *value = bswap64(*value); /* There's no ntohll() */ +#endif + else if(BYTEORDER_IS_LE(sp)) + *value = bswap64(*value); + return 0; +} + +/** + * Read a uint64 from storage, byte order is controlled by the settings + * on the storage, see krb5_storage_set_byteorder(). + * + * @param sp the storage to write too + * @param value the value read from the buffer + * + * @return 0 for success, or a Kerberos 5 error code on failure. + * + * @ingroup krb5_storage + */ + +KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL +krb5_ret_uint64(krb5_storage *sp, + uint64_t *value) +{ + krb5_error_code ret; + int64_t v; + + ret = krb5_ret_int64(sp, &v); + if (ret == 0) + *value = (uint64_t)v; + + return ret; +} + /** * Read a int32 from storage, byte order is controlled by the settings * on the storage, see krb5_storage_set_byteorder(). @@ -411,12 +513,15 @@ KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL krb5_ret_int32(krb5_storage *sp, int32_t *value) { - krb5_error_code ret = krb5_ret_int(sp, value, 4); - if(ret) + int64_t v; + + krb5_error_code ret = krb5_ret_int(sp, &v, 4); + if (ret) return ret; - if(BYTEORDER_IS_HOST(sp)) + *value = v; + if (BYTEORDER_IS_HOST(sp)) *value = htonl(*value); - else if(BYTEORDER_IS_LE(sp)) + else if (BYTEORDER_IS_LE(sp)) *value = bswap32(*value); return 0; } @@ -434,8 +539,7 @@ krb5_ret_int32(krb5_storage *sp, */ KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL -krb5_ret_uint32(krb5_storage *sp, - uint32_t *value) +krb5_ret_uint32(krb5_storage *sp, uint32_t *value) { krb5_error_code ret; int32_t v; @@ -505,7 +609,7 @@ KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL krb5_ret_int16(krb5_storage *sp, int16_t *value) { - int32_t v = 0; + int64_t v; int ret; ret = krb5_ret_int(sp, &v, 2); if(ret) diff --git a/source4/heimdal/lib/krb5/version-script.map b/source4/heimdal/lib/krb5/version-script.map index 2359001e9da..d0412c08d59 100644 --- a/source4/heimdal/lib/krb5/version-script.map +++ b/source4/heimdal/lib/krb5/version-script.map @@ -550,6 +550,7 @@ HEIMDAL_KRB5_2.0 { krb5_ret_data; krb5_ret_int16; krb5_ret_int32; + krb5_ret_int64; krb5_ret_int8; krb5_ret_keyblock; krb5_ret_principal; @@ -559,6 +560,7 @@ HEIMDAL_KRB5_2.0 { krb5_ret_times; krb5_ret_uint16; krb5_ret_uint32; + krb5_ret_uint64; krb5_ret_uint8; krb5_salttype_to_string; krb5_sendauth; @@ -623,6 +625,7 @@ HEIMDAL_KRB5_2.0 { krb5_store_data; krb5_store_int16; krb5_store_int32; + krb5_store_int64; krb5_store_int8; krb5_store_keyblock; krb5_store_principal; @@ -632,6 +635,7 @@ HEIMDAL_KRB5_2.0 { krb5_store_times; krb5_store_uint16; krb5_store_uint32; + krb5_store_uint64; krb5_store_uint8; krb5_string_to_deltat; krb5_string_to_enctype; -- 2.35.0 From ca3eac467b1d225bb54b8a156f3b3ea2dc107895 Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Wed, 16 Nov 2016 11:39:27 -0600 Subject: [PATCH 3/4] CVE-2022-42898 source4/heimdal: Round #2 of scan-build warnings cleanup BUG: https://bugzilla.samba.org/show_bug.cgi?id=15203 [jsutton@samba.org Kept only the modification to lib/krb5/store.c to avoid a build error] --- source4/heimdal/lib/krb5/store.c | 1 + 1 file changed, 1 insertion(+) diff --git a/source4/heimdal/lib/krb5/store.c b/source4/heimdal/lib/krb5/store.c index df0227b39de..39e51ec3dc8 100644 --- a/source4/heimdal/lib/krb5/store.c +++ b/source4/heimdal/lib/krb5/store.c @@ -431,6 +431,7 @@ krb5_ret_int(krb5_storage *sp, int ret; unsigned char v[8]; uint64_t w; + *value = 0; /* quiets warnings */ ret = sp->fetch(sp, v, len); if (ret < 0) return errno; -- 2.35.0 From 38dcbb82b084b8eeb97614f655a5f1d451ee6e20 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Fri, 14 Oct 2022 16:45:37 +1300 Subject: [PATCH 4/4] CVE-2022-42898 source4/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] [jsutton@samba.org Backported to our older version of Heimdal; removed lib/krb5/test_pac.c which we don't have] --- source4/heimdal/lib/krb5/pac.c | 621 +++++++++++++++++++++------------ 1 file changed, 403 insertions(+), 218 deletions(-) diff --git a/source4/heimdal/lib/krb5/pac.c b/source4/heimdal/lib/krb5/pac.c index 100de904662..0641c0c57bc 100644 --- a/source4/heimdal/lib/krb5/pac.c +++ b/source4/heimdal/lib/krb5/pac.c @@ -34,19 +34,34 @@ #include "krb5_locl.h" #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; @@ -80,6 +95,60 @@ struct krb5_pac_data { static const char zeros[PAC_ALIGNMENT] = { 0 }; +/* + * 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) */ @@ -125,149 +194,150 @@ 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 = calloc(1, 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_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]; - } - } - - 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); - free(p); - } *pac = NULL; + p = calloc(1, 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_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; + 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; } @@ -294,75 +364,109 @@ krb5_pac_init(krb5_context context, krb5_pac *pac) free(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; - 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; } @@ -374,8 +478,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 */ @@ -388,20 +492,19 @@ krb5_pac_get_buffer(krb5_context context, krb5_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; - } - } - return 0; + if (!data) + 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); @@ -466,7 +569,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); @@ -630,7 +733,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); @@ -899,11 +1002,11 @@ krb5_pac_verify(krb5_context context, if (pac->privsvr_checksum->buffersize < 4) return EINVAL; - 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); @@ -923,7 +1026,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) @@ -1019,13 +1122,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) { @@ -1070,6 +1180,7 @@ _krb5_pac_sign(krb5_context context, } } + /* Count missing-but-necessary buffers */ if (p->logon_name == NULL) num++; if (p->server_checksum == NULL) @@ -1079,35 +1190,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->privsvr_checksum)); p->ticket_checksum->type = PAC_TICKET_CHECKSUM; } } @@ -1143,11 +1264,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; @@ -1156,26 +1302,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); } @@ -1187,7 +1373,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) { @@ -1210,18 +1396,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; } @@ -1320,7 +1505,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); -- 2.35.0