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.

diff --git a/src/include/k5-int.h b/src/include/k5-int.h
index d8040475987804ed0869823b69e9307c61cf0537..a489ce3839d159fca1ceff45edd0f91cf1fb34fa 100644 (file)
--- 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 9947761450f1a175e8350e75d4e09d9d26eb7b8d..c540ad26cb885b857b0dcf5242a738c5baf065c7 100644 (file)
--- 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 4e85f68753ecef751ea131d1149d60a3d16d153d..9948e1bbe378ab70275de4398515fa436df98269 100644 (file)
--- 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 f46fb1370f3e5137de80ea809392e525cf91deff..d9d575a4dd46763a9e18c5d05b290c119aab7ab2 100644 (file)
--- 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 61b3c0f012a5dbd50d4463e64f333e7cbc99e870..31a613bebc6185f387bbbec0c75cb6f450ef0be6 100644 (file)
--- 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 2476865f3599f5da37be3279bedd19fed28dc6b0..4ada811ec07c9462a4ad735745cebd6a31d868df 100644 (file)
--- 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 8c6a0823f1c22ae6190e1e44ce834a30844f96de..f30de324aaa33570843ff6a8f6e56dce69cb32a7 100644 (file)
--- 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 3154b17c8f3091dba149526da9a2d9d69f324d8c..1a0bed0acdc8029ff1b58b5e824e7d88e9d81350 100644 (file)
--- 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 adf0fad2695f54a75435adbd2650781f580b5369..bee54165873a4c196e18c6e11ab693b51bec3271 100644 (file)
--- 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 546fb82dc583d90e71062907f50539b07d556a46..75b1c6ec0fec92f9de0764c5f1a8c63b9c8e3520 100644 (file)
--- 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 2c29e62c610d5a91699b1372ed2e8361d324116e..71833e6095b763d98e72873253ad88c0e5463d84 100644 (file)
--- 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 04d4b6514afcb5f88404e25627b65d8d900a9a42..c33c67dda503c94f75810cf94c7f79a791c78ad8 100644 (file)
--- 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 15f7cc6dc6ddfe147591114d6b6ee14cae578345..59614e7135e5c5dc58c255117e87d4a0affa0be6 100644 (file)
--- 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 87fe91117fa5b9efe3a273a9e768eb348940db6a..00d0c734f9e0f50b59291a05423ad75f5ebfcb7d 100644 (file)
--- 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 994f41d45c89b46e4524c27627c171a3127b3539..a6936107decb91759efd721f089548fe4b6725f5 100644 (file)
--- 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 b7bb9fe5b035b11bb5a92fa69642e152f1fc4d09..c85c0d44a33193bc0d2441917da0581f55a27658 100644 (file)
--- 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 779121a860e9ceedfff75a5a44cce824b74ce404..5bb64d00ad74ad7ba4527c9622573a5eb268338a 100644 (file)
--- 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 0aed147f34686c5f2cc5a1e0a5192256637ff431..2b966287c449e3f42410503c965f48bf55c2d6d3 100644 (file)
--- 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 57d4e37d208de990f838bb9533ea9c4b4f75b340..e3c8c7dae22b4de8d2896934670fac7f13686456 100644 (file)
--- 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 c7f6fdd4a9f186e715e40f9f7ae58e39401df225..b2dd1333e705d7e96e4584676b78b327fb3a0072 100644 (file)
--- 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 7d491ce2bdfb611f09d8e32623f1e8eadb5e6123..778e68b39b938135ee0c739f81e43646bf30953a 100644 (file)
--- 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';
 }