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.

11 files changed, 32 insertions, 39 deletions

diff --git a/src/lib/krb5/ccache/ccfns.c b/src/lib/krb5/ccache/ccfns.c
index 3154b17c8..1a0bed0ac 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 adf0fad26..bee541658 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 546fb82dc..75b1c6ec0 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 2c29e62c6..71833e609 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 04d4b6514..c33c67dda 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 15f7cc6dc..59614e713 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 87fe91117..00d0c734f 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 994f41d45..a6936107d 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 b7bb9fe5b..c85c0d44a 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 779121a86..5bb64d00a 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 0aed147f3..2b966287c 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;