From 4a2c5d259f5a7eda0f0f9028c061fcd032a72de0 Mon Sep 17 00:00:00 2001 From: Isaac Boukris Date: Sun, 26 Jan 2020 21:49:47 +0100 Subject: [PATCH] Zero length fields when freeing object contents In krb5_free_data_contents() and krb5_free_checksum_contents(), zero the length as well as the data pointer to leave the object in a valid state. Add asserts to existing test harnesses to verify the new behavior. In the krb5 GSS mech's kg_checksum_channel_bindings(), remove the code to reallocate the checksum with xmalloc(), as it relied on krb5_free_checksum_contents() leaving the object in an invalid state. This code was added in commit a30fb4c4400f13a2690df7ef910b7ac0ccbcf194 to match an xfree() call, but commit 29337e7c7b796685fb6a03466d32147e17aa2d16 replaced that xfree() with a krb5_free_checksum_contents(). (In addition, the xmalloc and xfree wrappers never evolved to do anything beyond malloc and free.) In kpropd's recv_database(), don't free outbuf until we are done using its length. [ghudson@mit.edu: rewrote commit message; edited doxygen comment changes to mention version] ticket: 8871 (new) --- src/include/krb5/krb5.hin | 4 ++++ src/kprop/kpropd.c | 2 +- src/lib/crypto/crypto_tests/t_cksums.c | 1 + src/lib/gssapi/krb5/util_cksum.c | 16 ---------------- src/lib/krb5/krb/kfree.c | 8 ++++---- src/tests/rdreq.c | 2 ++ 6 files changed, 12 insertions(+), 21 deletions(-) diff --git a/src/include/krb5/krb5.hin b/src/include/krb5/krb5.hin index d486853577..4cd9ad5ffe 100644 --- a/src/include/krb5/krb5.hin +++ b/src/include/krb5/krb5.hin @@ -4692,6 +4692,8 @@ krb5_free_checksum(krb5_context context, krb5_checksum *val); * @param [in] val Checksum structure to free contents of * * This function frees the contents of @a val, but not the structure itself. + * It sets the checksum's data pointer to null and (beginning in release 1.19) + * sets its length to zero. */ void KRB5_CALLCONV krb5_free_checksum_contents(krb5_context context, krb5_checksum *val); @@ -4751,6 +4753,8 @@ krb5_free_octet_data(krb5_context context, krb5_octet_data *val); * @param [in] val Data structure to free contents of * * This function frees the contents of @a val, but not the structure itself. + * It sets the structure's data pointer to null and (beginning in release 1.19) + * sets its length to zero. */ void KRB5_CALLCONV krb5_free_data_contents(krb5_context context, krb5_data *val); diff --git a/src/kprop/kpropd.c b/src/kprop/kpropd.c index 5622d56e14..ab4a764aa5 100644 --- a/src/kprop/kpropd.c +++ b/src/kprop/kpropd.c @@ -1412,7 +1412,6 @@ recv_database(krb5_context context, int fd, int database_fd, } n = write(database_fd, outbuf.data, outbuf.length); krb5_free_data_contents(context, &inbuf); - krb5_free_data_contents(context, &outbuf); if (n < 0) { snprintf(buf, sizeof(buf), "while writing database block starting at offset %d", @@ -1426,6 +1425,7 @@ recv_database(krb5_context context, int fd, int database_fd, send_error(context, fd, KRB5KRB_ERR_GENERIC, buf); } received_size += outbuf.length; + krb5_free_data_contents(context, &outbuf); } /* OK, we've seen the entire file. Did we get too many bytes? */ diff --git a/src/lib/crypto/crypto_tests/t_cksums.c b/src/lib/crypto/crypto_tests/t_cksums.c index 4da14ea438..8297fcbf5c 100644 --- a/src/lib/crypto/crypto_tests/t_cksums.c +++ b/src/lib/crypto/crypto_tests/t_cksums.c @@ -254,6 +254,7 @@ main(int argc, char **argv) } krb5_free_checksum_contents(context, &cksum); + assert(cksum.length == 0); } return status; } diff --git a/src/lib/gssapi/krb5/util_cksum.c b/src/lib/gssapi/krb5/util_cksum.c index a1770774e1..2d6b50b2a2 100644 --- a/src/lib/gssapi/krb5/util_cksum.c +++ b/src/lib/gssapi/krb5/util_cksum.c @@ -39,7 +39,6 @@ kg_checksum_channel_bindings(context, cb, cksum) size_t sumlen; krb5_data plaind; krb5_error_code code; - void *temp; /* initialize the the cksum */ code = krb5_c_checksum_length(context, CKSUMTYPE_RSA_MD5, &sumlen); @@ -88,21 +87,6 @@ kg_checksum_channel_bindings(context, cb, cksum) code = krb5_c_make_checksum(context, CKSUMTYPE_RSA_MD5, 0, 0, &plaind, cksum); - if (code) - goto cleanup; - - if ((temp = xmalloc(cksum->length)) == NULL) { - krb5_free_checksum_contents(context, cksum); - code = ENOMEM; - goto cleanup; - } - - memcpy(temp, cksum->contents, cksum->length); - krb5_free_checksum_contents(context, cksum); - cksum->contents = (krb5_octet *)temp; - - /* success */ -cleanup: if (buf) xfree(buf); return code; diff --git a/src/lib/krb5/krb/kfree.c b/src/lib/krb5/krb/kfree.c index ab2409f172..6e38044050 100644 --- a/src/lib/krb5/krb/kfree.c +++ b/src/lib/krb5/krb/kfree.c @@ -145,6 +145,7 @@ krb5_free_checksum_contents(krb5_context context, krb5_checksum *val) return; free(val->contents); val->contents = NULL; + val->length = 0; } void KRB5_CALLCONV @@ -242,10 +243,9 @@ krb5_free_data_contents(krb5_context context, krb5_data *val) { if (val == NULL) return; - if (val->data) { - free(val->data); - val->data = 0; - } + free(val->data); + val->data = NULL; + val->length = 0; } void KRB5_CALLCONV diff --git a/src/tests/rdreq.c b/src/tests/rdreq.c index c010cb2c71..52bec18c91 100644 --- a/src/tests/rdreq.c +++ b/src/tests/rdreq.c @@ -33,6 +33,7 @@ #include #include #include +#include #include int @@ -105,6 +106,7 @@ main(int argc, char **argv) } krb5_free_data_contents(context, &apreq); + assert(apreq.length == 0); krb5_auth_con_free(context, auth_con); krb5_free_creds(context, cred); krb5_cc_close(context, ccache); -- 2.47.2