krb5 commit: Zero length fields when freeing object contents
Greg Hudson
ghudson at mit.edu
Tue Jan 28 12:02:00 EST 2020
https://github.com/krb5/krb5/commit/4a2c5d259f5a7eda0f0f9028c061fcd032a72de0
commit 4a2c5d259f5a7eda0f0f9028c061fcd032a72de0
Author: Isaac Boukris <iboukris at gmail.com>
Date: Sun Jan 26 21:49:47 2020 +0100
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 at 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 d486853..4cd9ad5 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 5622d56..ab4a764 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 4da14ea..8297fcb 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 a177077..2d6b50b 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 ab2409f..6e38044 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 c010cb2..52bec18 100644
--- a/src/tests/rdreq.c
+++ b/src/tests/rdreq.c
@@ -33,6 +33,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <assert.h>
#include <krb5.h>
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);
More information about the cvs-krb5
mailing list