ipa-kdb: more robust handling of principal addition/editing

The patch addresses the following defect reported by covscan in FreeIPA
master:

"""
Error: FORWARD_NULL (CWE-476):
/daemons/ipa-kdb/ipa_kdb_principals.c:1886: assign_zero: Assigning:
"principal" = "NULL".
/daemons/ipa-kdb/ipa_kdb_principals.c:1929:
var_deref_model: Passing null pointer "principal" to "ipadb_entry_to_mods",
which dereferences it.
/daemons/ipa-kdb/ipa_kdb_principals.c:1491:9:
deref_parm_in_call: Function "ipadb_get_ldap_mod_str" dereferences
"principal".
/daemons/ipa-kdb/ipa_kdb_principals.c:1174:5:
deref_parm_in_call: Function "strdup" dereferences "value"
"""

This is a part of series of patches related to
https://fedorahosted.org/freeipa/ticket/4795

Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>

1 files changed, 47 insertions, 23 deletions

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index e158c236e..9d43ebc66 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -1474,10 +1474,38 @@ done:
     return kerr;
 }
 
+static krb5_error_code ipadb_principal_to_mods(krb5_context kcontext,
+                                               struct ipadb_mods *imods,
+                                               char *principal,
+                                               int mod_op)
+{
+    krb5_error_code kerr;
+
+    if (principal == NULL) {
+       kerr = EINVAL;
+       goto done;
+    }
+
+    kerr = ipadb_get_ldap_mod_str(imods, "krbPrincipalName",
+                                  principal, mod_op);
+    if (kerr) {
+        goto done;
+    }
+    kerr = ipadb_get_ldap_mod_str(imods, "ipaKrbPrincipalAlias",
+                                  principal, mod_op);
+    if (kerr) {
+        goto done;
+    }
+
+    kerr = 0;
+
+done:
+    return kerr;
+}
+
 static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
                                            struct ipadb_mods *imods,
                                            krb5_db_entry *entry,
-                                           char *principal,
                                            int mod_op)
 {
     krb5_error_code kerr;
@@ -1486,20 +1514,6 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
 
     /* check each mask flag in order */
 
-    /* KADM5_PRINCIPAL */
-    if (entry->mask & KMASK_PRINCIPAL) {
-        kerr = ipadb_get_ldap_mod_str(imods, "krbPrincipalName",
-                                      principal, mod_op);
-        if (kerr) {
-            goto done;
-        }
-        kerr = ipadb_get_ldap_mod_str(imods, "ipaKrbPrincipalAlias",
-                                      principal, mod_op);
-        if (kerr) {
-            goto done;
-        }
-    }
-
     /* KADM5_PRINC_EXPIRE_TIME */
     if (entry->mask & KMASK_PRINC_EXPIRE_TIME) {
         kerr = ipadb_get_ldap_mod_time(imods,
@@ -1863,8 +1877,12 @@ static krb5_error_code ipadb_add_principal(krb5_context kcontext,
         goto done;
     }
 
-    kerr = ipadb_entry_to_mods(kcontext, imods,
-                               entry, principal, LDAP_MOD_ADD);
+    kerr = ipadb_principal_to_mods(kcontext, imods, principal, LDAP_MOD_ADD);
+    if (kerr != 0) {
+        goto done;
+    }
+
+    kerr = ipadb_entry_to_mods(kcontext, imods, entry, LDAP_MOD_ADD);
     if (kerr != 0) {
         goto done;
     }
@@ -1895,6 +1913,11 @@ static krb5_error_code ipadb_modify_principal(krb5_context kcontext,
         return KRB5_KDB_DBNOTINITED;
     }
 
+    kerr = new_ipadb_mods(&imods);
+    if (kerr) {
+        goto done;
+    }
+
     ied = (struct ipadb_e_data *)entry->e_data;
     if (!ied || !ied->entry_dn) {
         kerr = krb5_unparse_name(kcontext, entry->princ, &principal);
@@ -1919,15 +1942,16 @@ static krb5_error_code ipadb_modify_principal(krb5_context kcontext,
             kerr = KRB5_KDB_INTERNAL_ERROR;
             goto done;
         }
-    }
 
-    kerr = new_ipadb_mods(&imods);
-    if (kerr) {
-        goto done;
+        kerr = ipadb_principal_to_mods(kcontext, imods, principal,
+                                       LDAP_MOD_REPLACE);
+        if (kerr != 0) {
+            goto done;
+        }
+
     }
 
-    kerr = ipadb_entry_to_mods(kcontext, imods,
-                               entry, principal, LDAP_MOD_REPLACE);
+    kerr = ipadb_entry_to_mods(kcontext, imods, entry, LDAP_MOD_REPLACE);
     if (kerr != 0) {
         goto done;
     }