krb5 commit: Avoid passing null pointers to memcpy/memcmp
Greg Hudson
ghudson at MIT.EDU
Mon Apr 8 15:33:13 EDT 2013
https://github.com/krb5/krb5/commit/31124ffb81e8c0935403a9fdc169dead5ecaa777
commit 31124ffb81e8c0935403a9fdc169dead5ecaa777
Author: Greg Hudson <ghudson at mit.edu>
Date: Mon Apr 8 15:32:31 2013 -0400
Avoid passing null pointers to memcpy/memcmp
By a strict reading of the C standard, memcpy and memcmp have
undefined behavior if their pointer arguments aren't valid object
pointers, even if the length argument is 0. Compilers are becoming
more aggressive about breaking code with undefined behavior, so we
should try to avoid it when possible.
In a krb5_data object, we frequently use NULL as the data value when
the length is 0. Accordingly, we should avoid copying from or
comparing the data field of a length-0 krb5_data object. Add checks
to our wrapper functions (like data_eq and k5_memdup) and to code
which works with possibly-empty krb5_data objects. In a few places,
use wrapper functions to simplify the code rather than adding checks.
src/include/k5-int.h | 15 ++++++++-------
src/kdc/kdc_transit.c | 3 ++-
src/kdc/kdc_util.c | 15 ++++++++++-----
src/lib/crypto/krb/encrypt.c | 3 ++-
src/lib/crypto/krb/s2k_des.c | 11 +++++++----
src/lib/crypto/krb/s2k_pbkdf2.c | 11 +++++++----
src/lib/gssapi/krb5/iakerb.c | 3 ++-
src/lib/krb5/ccache/ccfns.c | 19 ++++---------------
src/lib/krb5/ccache/ccselect_k5identity.c | 5 ++---
src/lib/krb5/krb/authdata.c | 3 +--
src/lib/krb5/krb/chk_trans.c | 3 ++-
src/lib/krb5/krb/conv_princ.c | 6 ++++--
src/lib/krb5/krb/get_in_tkt.c | 11 ++++-------
src/lib/krb5/krb/pr_to_salt.c | 6 ++++--
src/lib/krb5/krb/princ_comp.c | 2 ++
src/lib/krb5/krb/s4u_creds.c | 9 ++++++---
src/lib/krb5/krb/unparse.c | 3 ++-
src/lib/krb5/krb/walk_rtree.c | 4 +---
src/plugins/preauth/securid_sam2/securid2.c | 3 ++-
src/util/support/json.c | 3 ++-
src/util/support/k5buf.c | 3 ++-
21 files changed, 76 insertions(+), 65 deletions(-)
diff --git a/src/include/k5-int.h b/src/include/k5-int.h
index d804047..a489ce3 100644
--- a/src/include/k5-int.h
+++ b/src/include/k5-int.h
@@ -2138,13 +2138,15 @@ krb5_error_code krb5_set_time_offsets(krb5_context, krb5_timestamp,
static inline int
data_eq(krb5_data d1, krb5_data d2)
{
- return (d1.length == d2.length && !memcmp(d1.data, d2.data, d1.length));
+ return (d1.length == d2.length && (d1.length == 0 ||
+ !memcmp(d1.data, d2.data, d1.length)));
}
static inline int
data_eq_string (krb5_data d, const char *s)
{
- return (d.length == strlen(s) && !memcmp(d.data, s, d.length));
+ return (d.length == strlen(s) && (d.length == 0 ||
+ !memcmp(d.data, s, d.length)));
}
static inline krb5_data
@@ -2187,9 +2189,8 @@ alloc_data(krb5_data *data, unsigned int len)
static inline int
authdata_eq(krb5_authdata a1, krb5_authdata a2)
{
- return (a1.ad_type == a2.ad_type
- && a1.length == a2.length
- && !memcmp(a1.contents, a2.contents, a1.length));
+ return (a1.ad_type == a2.ad_type && a1.length == a2.length &&
+ (a1.length == 0 || !memcmp(a1.contents, a2.contents, a1.length)));
}
/* Allocate zeroed memory; set *code to 0 on success or ENOMEM on failure. */
@@ -2210,7 +2211,7 @@ k5memdup(const void *in, size_t len, krb5_error_code *code)
{
void *ptr = k5alloc(len, code);
- if (ptr != NULL)
+ if (ptr != NULL && len > 0)
memcpy(ptr, in, len);
return ptr;
}
@@ -2221,7 +2222,7 @@ k5memdup0(const void *in, size_t len, krb5_error_code *code)
{
void *ptr = k5alloc(len + 1, code);
- if (ptr != NULL)
+ if (ptr != NULL && len > 0)
memcpy(ptr, in, len);
return ptr;
}
diff --git a/src/kdc/kdc_transit.c b/src/kdc/kdc_transit.c
index 9947761..c540ad2 100644
--- a/src/kdc/kdc_transit.c
+++ b/src/kdc/kdc_transit.c
@@ -135,7 +135,8 @@ data2string (krb5_data *d)
char *s;
s = malloc(d->length + 1);
if (s) {
- memcpy(s, d->data, d->length);
+ if (d->length > 0)
+ memcpy(s, d->data, d->length);
s[d->length] = 0;
}
return s;
diff --git a/src/kdc/kdc_util.c b/src/kdc/kdc_util.c
index 4e85f68..9948e1b 100644
--- a/src/kdc/kdc_util.c
+++ b/src/kdc/kdc_util.c
@@ -1091,16 +1091,21 @@ verify_for_user_checksum(krb5_context context,
p += 4;
for (i = 0; i < krb5_princ_size(context, req->user); i++) {
- memcpy(p, krb5_princ_component(context, req->user, i)->data,
- krb5_princ_component(context, req->user, i)->length);
+ if (krb5_princ_component(context, req->user, i)->length > 0) {
+ memcpy(p, krb5_princ_component(context, req->user, i)->data,
+ krb5_princ_component(context, req->user, i)->length);
+ }
p += krb5_princ_component(context, req->user, i)->length;
}
- memcpy(p, krb5_princ_realm(context, req->user)->data,
- krb5_princ_realm(context, req->user)->length);
+ if (krb5_princ_realm(context, req->user)->length > 0) {
+ memcpy(p, krb5_princ_realm(context, req->user)->data,
+ krb5_princ_realm(context, req->user)->length);
+ }
p += krb5_princ_realm(context, req->user)->length;
- memcpy(p, req->auth_package.data, req->auth_package.length);
+ if (req->auth_package.length > 0)
+ memcpy(p, req->auth_package.data, req->auth_package.length);
p += req->auth_package.length;
code = krb5_c_verify_checksum(context,
diff --git a/src/lib/crypto/krb/encrypt.c b/src/lib/crypto/krb/encrypt.c
index f46fb13..d9d575a 100644
--- a/src/lib/crypto/krb/encrypt.c
+++ b/src/lib/crypto/krb/encrypt.c
@@ -60,7 +60,8 @@ krb5_k_encrypt(krb5_context context, krb5_key key,
iov[1].flags = KRB5_CRYPTO_TYPE_DATA;
iov[1].data = make_data(output->ciphertext.data + header_len,
input->length);
- memcpy(iov[1].data.data, input->data, input->length);
+ if (input->length > 0)
+ memcpy(iov[1].data.data, input->data, input->length);
iov[2].flags = KRB5_CRYPTO_TYPE_PADDING;
iov[2].data = make_data(iov[1].data.data + input->length, padding_len);
diff --git a/src/lib/crypto/krb/s2k_des.c b/src/lib/crypto/krb/s2k_des.c
index 61b3c0f..31a613b 100644
--- a/src/lib/crypto/krb/s2k_des.c
+++ b/src/lib/crypto/krb/s2k_des.c
@@ -404,7 +404,8 @@ afs_s2k_oneblock(const krb5_data *data, const krb5_data *salt,
*/
memset(password, 0, sizeof(password));
- memcpy(password, salt->data, min(salt->length, 8));
+ if (salt->length > 0)
+ memcpy(password, salt->data, min(salt->length, 8));
for (i = 0; i < 8; i++) {
if (isupper(password[i]))
password[i] = tolower(password[i]);
@@ -443,7 +444,8 @@ afs_s2k_multiblock(const krb5_data *data, const krb5_data *salt,
if (!password)
return ENOMEM;
- memcpy(password, data->data, data->length);
+ if (data->length > 0)
+ memcpy(password, data->data, data->length);
for (i = data->length, j = 0; j < salt->length; i++, j++) {
password[i] = salt->data[j];
if (isupper(password[i]))
@@ -513,8 +515,9 @@ des_s2k(const krb5_data *pw, const krb5_data *salt, unsigned char *key_out)
copy = malloc(copylen);
if (copy == NULL)
return ENOMEM;
- memcpy(copy, pw->data, pw->length);
- if (salt)
+ if (pw->length > 0)
+ memcpy(copy, pw->data, pw->length);
+ if (salt != NULL && salt->length > 0)
memcpy(copy + pw->length, salt->data, salt->length);
memset(&temp, 0, sizeof(temp));
diff --git a/src/lib/crypto/krb/s2k_pbkdf2.c b/src/lib/crypto/krb/s2k_pbkdf2.c
index 2476865..4ada811 100644
--- a/src/lib/crypto/krb/s2k_pbkdf2.c
+++ b/src/lib/crypto/krb/s2k_pbkdf2.c
@@ -61,8 +61,9 @@ krb5int_dk_string_to_key(const struct krb5_keytypes *ktp,
/* construct input string ( = string + salt), fold it, make_key it */
- memcpy(concat, string->data, string->length);
- if (salt)
+ if (string->length > 0)
+ memcpy(concat, string->data, string->length);
+ if (salt != NULL && salt->length > 0)
memcpy(concat + string->length, salt->data, salt->length);
krb5int_nfold(concatlen*8, concat, keybytes*8, foldstring);
@@ -146,9 +147,11 @@ pbkdf2_string_to_key(const struct krb5_keytypes *ktp, const krb5_data *string,
if (err)
return err;
- memcpy(sandp.data, pepper->data, pepper->length);
+ if (pepper->length > 0)
+ memcpy(sandp.data, pepper->data, pepper->length);
sandp.data[pepper->length] = '\0';
- memcpy(&sandp.data[pepper->length + 1], salt->data, salt->length);
+ if (salt->length > 0)
+ memcpy(&sandp.data[pepper->length + 1], salt->data, salt->length);
salt = &sandp;
}
diff --git a/src/lib/gssapi/krb5/iakerb.c b/src/lib/gssapi/krb5/iakerb.c
index 8c6a082..f30de32 100644
--- a/src/lib/gssapi/krb5/iakerb.c
+++ b/src/lib/gssapi/krb5/iakerb.c
@@ -278,7 +278,8 @@ iakerb_make_token(iakerb_ctx_id_t ctx,
}
data->data = p;
- memcpy(data->data + data->length, request->data, request->length);
+ if (request->length > 0)
+ memcpy(data->data + data->length, request->data, request->length);
data->length += request->length;
if (initialContextToken)
diff --git a/src/lib/krb5/ccache/ccfns.c b/src/lib/krb5/ccache/ccfns.c
index 3154b17..1a0bed0 100644
--- a/src/lib/krb5/ccache/ccfns.c
+++ b/src/lib/krb5/ccache/ccfns.c
@@ -284,15 +284,9 @@ krb5_cc_set_config(krb5_context context, krb5_ccache id,
if (data == NULL) {
ret = krb5_cc_remove_cred(context, id, 0, &cred);
} else {
- cred.ticket.data = malloc(data->length);
- if (cred.ticket.data == NULL) {
- ret = ENOMEM;
- krb5_set_error_message(context, ret, "malloc: out of memory");
+ ret = krb5int_copy_data_contents(context, data, &cred.ticket);
+ if (ret)
goto out;
- }
- cred.ticket.length = data->length;
- memcpy(cred.ticket.data, data->data, data->length);
-
ret = krb5_cc_store_cred(context, id, &cred);
}
out:
@@ -319,14 +313,9 @@ krb5_cc_get_config(krb5_context context, krb5_ccache id,
if (ret)
goto out;
- data->data = malloc(cred.ticket.length);
- if (data->data == NULL) {
- ret = ENOMEM;
- krb5_set_error_message(context, ENOMEM, "malloc: out of memory");
+ ret = krb5int_copy_data_contents(context, &cred.ticket, data);
+ if (ret)
goto out;
- }
- data->length = cred.ticket.length;
- memcpy(data->data, cred.ticket.data, data->length);
TRACE_CC_GET_CONFIG(context, id, principal, key, data);
diff --git a/src/lib/krb5/ccache/ccselect_k5identity.c b/src/lib/krb5/ccache/ccselect_k5identity.c
index adf0fad..bee5416 100644
--- a/src/lib/krb5/ccache/ccselect_k5identity.c
+++ b/src/lib/krb5/ccache/ccselect_k5identity.c
@@ -46,14 +46,13 @@ k5identity_init(krb5_context context, krb5_ccselect_moddata *data_out,
static krb5_boolean
fnmatch_data(const char *pattern, krb5_data *data, krb5_boolean fold_case)
{
+ krb5_error_code ret;
char *str, *p;
int res;
- str = malloc(data->length + 1);
+ str = k5memdup0(data->data, data->length, &ret);
if (str == NULL)
return FALSE;
- memcpy(str, data->data, data->length);
- str[data->length] = '\0';
if (fold_case) {
for (p = str; *p != '\0'; p++) {
diff --git a/src/lib/krb5/krb/authdata.c b/src/lib/krb5/krb/authdata.c
index 546fb82..75b1c6e 100644
--- a/src/lib/krb5/krb/authdata.c
+++ b/src/lib/krb5/krb/authdata.c
@@ -292,8 +292,7 @@ k5_ad_find_module(krb5_context kcontext,
continue;
/* check for name match */
- if (strlen(module->name) != name->length ||
- memcmp(module->name, name->data, name->length) != 0)
+ if (!data_eq_string(*name, module->name))
continue;
ret = module;
diff --git a/src/lib/krb5/krb/chk_trans.c b/src/lib/krb5/krb/chk_trans.c
index 2c29e62..71833e6 100644
--- a/src/lib/krb5/krb/chk_trans.c
+++ b/src/lib/krb5/krb/chk_trans.c
@@ -242,7 +242,8 @@ foreach_realm (krb5_error_code (*fn)(krb5_data *comp,void *data), void *data,
if (p == transit->data) {
if (crealm->length >= MAXLEN)
return KRB5KRB_AP_ERR_ILL_CR_TKT;
- memcpy (last, crealm->data, crealm->length);
+ if (crealm->length > 0)
+ memcpy (last, crealm->data, crealm->length);
last[crealm->length] = '\0';
last_component.length = crealm->length;
}
diff --git a/src/lib/krb5/krb/conv_princ.c b/src/lib/krb5/krb/conv_princ.c
index 04d4b65..c33c67d 100644
--- a/src/lib/krb5/krb/conv_princ.c
+++ b/src/lib/krb5/krb/conv_princ.c
@@ -194,7 +194,8 @@ krb5_524_conv_principal(krb5_context context, krb5_const_principal princ,
compo = &princ->data[1];
if (compo->length >= INST_SZ - 1)
return KRB5_INVALID_PRINCIPAL;
- memcpy(inst, compo->data, compo->length);
+ if (compo->length > 0)
+ memcpy(inst, compo->data, compo->length);
inst[compo->length] = '\0';
}
/* fall through */
@@ -204,7 +205,8 @@ krb5_524_conv_principal(krb5_context context, krb5_const_principal princ,
compo = &princ->data[0];
if (compo->length >= ANAME_SZ)
return KRB5_INVALID_PRINCIPAL;
- memcpy(name, compo->data, compo->length);
+ if (compo->length > 0)
+ memcpy(name, compo->data, compo->length);
name[compo->length] = '\0';
}
break;
diff --git a/src/lib/krb5/krb/get_in_tkt.c b/src/lib/krb5/krb/get_in_tkt.c
index 15f7cc6..59614e7 100644
--- a/src/lib/krb5/krb/get_in_tkt.c
+++ b/src/lib/krb5/krb/get_in_tkt.c
@@ -1073,6 +1073,7 @@ init_creds_validate_reply(krb5_context context,
static void
read_allowed_preauth_type(krb5_context context, krb5_init_creds_context ctx)
{
+ krb5_error_code ret;
krb5_data config;
char *tmp, *p;
@@ -1084,18 +1085,14 @@ read_allowed_preauth_type(krb5_context context, krb5_init_creds_context ctx)
ctx->request->server,
KRB5_CC_CONF_PA_TYPE, &config) != 0)
return;
- tmp = malloc(config.length + 1);
- if (tmp == NULL) {
- krb5_free_data_contents(context, &config);
+ tmp = k5memdup0(config.data, config.length, &ret);
+ krb5_free_data_contents(context, &config);
+ if (tmp == NULL)
return;
- }
- memcpy(tmp, config.data, config.length);
- tmp[config.length] = '\0';
ctx->allowed_preauth_type = strtol(tmp, &p, 10);
if (p == NULL || *p != '\0')
ctx->allowed_preauth_type = KRB5_PADATA_NONE;
free(tmp);
- krb5_free_data_contents(context, &config);
}
static krb5_error_code
diff --git a/src/lib/krb5/krb/pr_to_salt.c b/src/lib/krb5/krb/pr_to_salt.c
index 87fe911..00d0c73 100644
--- a/src/lib/krb5/krb/pr_to_salt.c
+++ b/src/lib/krb5/krb/pr_to_salt.c
@@ -56,11 +56,13 @@ principal2salt_internal(krb5_context context,
if (use_realm) {
offset = pr->realm.length;
- memcpy(ret->data, pr->realm.data, offset);
+ if (offset > 0)
+ memcpy(ret->data, pr->realm.data, offset);
}
for (i = 0; i < pr->length; i++) {
- memcpy(&ret->data[offset], pr->data[i].data, pr->data[i].length);
+ if (pr->data[i].length > 0)
+ memcpy(&ret->data[offset], pr->data[i].data, pr->data[i].length);
offset += pr->data[i].length;
}
return 0;
diff --git a/src/lib/krb5/krb/princ_comp.c b/src/lib/krb5/krb/princ_comp.c
index 994f41d..a693610 100644
--- a/src/lib/krb5/krb/princ_comp.c
+++ b/src/lib/krb5/krb/princ_comp.c
@@ -38,6 +38,8 @@ realm_compare_flags(krb5_context context,
if (realm1->length != realm2->length)
return FALSE;
+ if (realm1->length == 0)
+ return TRUE;
return (flags & KRB5_PRINCIPAL_COMPARE_CASEFOLD) ?
(strncasecmp(realm1->data, realm2->data, realm2->length) == 0) :
diff --git a/src/lib/krb5/krb/s4u_creds.c b/src/lib/krb5/krb/s4u_creds.c
index b7bb9fe..c85c0d4 100644
--- a/src/lib/krb5/krb/s4u_creds.c
+++ b/src/lib/krb5/krb/s4u_creds.c
@@ -161,14 +161,17 @@ make_pa_for_user_checksum(krb5_context context,
p += 4;
for (i = 0; i < req->user->length; i++) {
- memcpy(p, req->user->data[i].data, req->user->data[i].length);
+ if (req->user->data[i].length > 0)
+ memcpy(p, req->user->data[i].data, req->user->data[i].length);
p += req->user->data[i].length;
}
- memcpy(p, req->user->realm.data, req->user->realm.length);
+ if (req->user->realm.length > 0)
+ memcpy(p, req->user->realm.data, req->user->realm.length);
p += req->user->realm.length;
- memcpy(p, req->auth_package.data, req->auth_package.length);
+ if (req->auth_package.length > 0)
+ memcpy(p, req->auth_package.data, req->auth_package.length);
/* Per spec, use hmac-md5 checksum regardless of key type. */
code = krb5_c_make_checksum(context, CKSUMTYPE_HMAC_MD5_ARCFOUR, key,
diff --git a/src/lib/krb5/krb/unparse.c b/src/lib/krb5/krb/unparse.c
index 779121a..5bb64d0 100644
--- a/src/lib/krb5/krb/unparse.c
+++ b/src/lib/krb5/krb/unparse.c
@@ -90,7 +90,8 @@ copy_component_quoting(char *dest, const krb5_data *src, int flags)
int length = src->length;
if (flags & KRB5_PRINCIPAL_UNPARSE_DISPLAY) {
- memcpy(dest, src->data, src->length);
+ if (src->length > 0)
+ memcpy(dest, src->data, src->length);
return src->length;
}
diff --git a/src/lib/krb5/krb/walk_rtree.c b/src/lib/krb5/krb/walk_rtree.c
index 0aed147..2b96628 100644
--- a/src/lib/krb5/krb/walk_rtree.c
+++ b/src/lib/krb5/krb/walk_rtree.c
@@ -105,10 +105,8 @@ krb5_walk_realm_tree( krb5_context context,
if (client->data == NULL || server->data == NULL)
return KRB5_NO_TKT_IN_RLM;
- if (client->length == server->length &&
- memcmp(client->data, server->data, server->length) == 0) {
+ if (data_eq(*client, *server))
return KRB5_NO_TKT_IN_RLM;
- }
retval = rtree_capath_vals(context, client, server, &capvals);
if (retval)
return retval;
diff --git a/src/plugins/preauth/securid_sam2/securid2.c b/src/plugins/preauth/securid_sam2/securid2.c
index 57d4e37..e3c8c7d 100644
--- a/src/plugins/preauth/securid_sam2/securid2.c
+++ b/src/plugins/preauth/securid_sam2/securid2.c
@@ -378,7 +378,8 @@ verify_securid_data_2(krb5_context context, krb5_db_entry *client,
esre2->sam_sad.length, user);
goto cleanup;
}
- memcpy(passcode, esre2->sam_sad.data, esre2->sam_sad.length);
+ if (esre2->sam_sad.length > 0)
+ memcpy(passcode, esre2->sam_sad.data, esre2->sam_sad.length);
securid_user = strdup(user);
if (!securid_user) {
diff --git a/src/util/support/json.c b/src/util/support/json.c
index c7f6fdd..b2dd133 100644
--- a/src/util/support/json.c
+++ b/src/util/support/json.c
@@ -489,7 +489,8 @@ k5_json_string_create_len(const void *data, size_t len,
s = alloc_value(&string_type, len + 1);
if (s == NULL)
return ENOMEM;
- memcpy(s, data, len);
+ if (len > 0)
+ memcpy(s, data, len);
s[len] = '\0';
*val_out = (k5_json_string)s;
return 0;
diff --git a/src/util/support/k5buf.c b/src/util/support/k5buf.c
index 7d491ce..778e68b 100644
--- a/src/util/support/k5buf.c
+++ b/src/util/support/k5buf.c
@@ -119,7 +119,8 @@ k5_buf_add_len(struct k5buf *buf, const char *data, size_t len)
{
if (!ensure_space(buf, len))
return;
- memcpy(buf->data + buf->len, data, len);
+ if (len > 0)
+ memcpy(buf->data + buf->len, data, len);
buf->len += len;
buf->data[buf->len] = '\0';
}
More information about the cvs-krb5
mailing list