On Tue, 27 Jan 2015, Simo Sorce wrote:
On Tue, 2015-01-27 at 22:20 +0200, Alexander Bokovoy wrote:
On Tue, 27 Jan 2015, Simo Sorce wrote:
>On Tue, 2015-01-27 at 22:07 +0200, Alexander Bokovoy wrote:
>> On Tue, 27 Jan 2015, Simo Sorce wrote:
>> >On Tue, 2015-01-27 at 18:04 +0100, Petr Spacek wrote:
>> >> On 27.1.2015 17:56, Alexander Bokovoy wrote:
>> >> > On Tue, 27 Jan 2015, Martin Babinsky wrote:
>> >> >> From 23a823c3c5933d5c14342e15c00599af74b84118 Mon Sep 17 00:00:00 2001
>> >> >> From: Martin Babinsky <mbabi...@redhat.com>
>> >> >> Date: Tue, 27 Jan 2015 13:21:33 +0100
>> >> >> Subject: [PATCH 3/7] proposed fix fo a defect reported in
>> >> >> 'ipa_kdb_principals.c'
>> >> >>
>> >> >> 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"
>> >> >> """
>> >> >>
>> >> >> Again I have consulted this with Simo and he pointed out that this may 
indeed
>> >> >> be a bug and that we should always parse principal name.
>> >> >>
>> >> >> This is a part of series of patches related to
>> >> >> https://fedorahosted.org/freeipa/ticket/4795
>> >> >> ---
>> >> >> daemons/ipa-kdb/ipa_kdb_principals.c | 11 ++++-------
>> >> >> 1 file changed, 4 insertions(+), 7 deletions(-)
>> >> >>
>> >> >> diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c
>> >> >> b/daemons/ipa-kdb/ipa_kdb_principals.c
>> >> >> index
>> >> >> 
e158c236eab5c7c5a7c12664dbde5d51cc55406d..760faeef224701c3dc4ee69d16df043e9c622d9a
>> >> >> 100644
>> >> >> --- a/daemons/ipa-kdb/ipa_kdb_principals.c
>> >> >> +++ b/daemons/ipa-kdb/ipa_kdb_principals.c
>> >> >> @@ -1894,19 +1894,16 @@ static krb5_error_code
>> >> >> ipadb_modify_principal(krb5_context kcontext,
>> >> >>     if (!ipactx) {
>> >> >>         return KRB5_KDB_DBNOTINITED;
>> >> >>     }
>> >> >> -
>> >> >> +    kerr = krb5_unparse_name(kcontext, entry->princ, &principal);
>> >> >> +    if (kerr != 0) {
>> >> >> +        goto done;
>> >> >> +    }
>> >> >>     ied = (struct ipadb_e_data *)entry->e_data;
>> >> >>     if (!ied || !ied->entry_dn) {
>> >> >> -        kerr = krb5_unparse_name(kcontext, entry->princ, &principal);
>> >> >> -        if (kerr != 0) {
>> >> >> -            goto done;
>> >> >> -        }
>> >> >> -
>> >> >>         kerr = ipadb_fetch_principals(ipactx, 0, principal, &res);
>> >> >>         if (kerr != 0) {
>> >> >>             goto done;
>> >> >>         }
>> >> >> -
>> >> >>         /* FIXME: no alias allowed for now, should we allow modifies
>> >> >>          * by alias name ? */
>> >> >>         kerr = ipadb_find_principal(kcontext, 0, res, &principal, 
&lentry);
>> >> >> --
>> >> >> 2.1.0
>> >> >>
>> >> > NACK.
>> >> >
>> >> > This part actually looked to me familiar and it was indeed raised in
>> >> > past. I marked the Coverity report for this as a false positive but it
>> >> > sprung again.
>> >> >
>> >> > Last time I wrote (2014-04-07):
>> >> > ------------------------------------------------------
>> >> > I remember looking at this bug already in past and I looked at it again.
>> >> > I think it is false positive. ipadb_modify_principal() is only called
>> >> > from ipadb_put_principal() when entry for the principal has no
>> >> > KMASK_PRINCIPAL flag set. Next, ipadb_modify_principal() calls
>> >> > ipadb_entry_to_mods() with a principal which might be set to NULL.
>> >> > However, ipadb_entry_to_mods() will only dereference this principal when
>> >> > KMASK_PRINCIPAL flag is set in the entry, i.e. never for this code path.
>> >> > -------------------------------------------------------
>> >>
>> >> Hmm... It may work for now but it also may break silently in future if we
>> >> break current assumption "ipadb_modify_principal() is only called from
>> >> ipadb_put_principal() when entry for the principal has no KMASK_PRINCIPAL 
flag
>> >> set".
>> >>
>> >> Personally I'm against this kind of implicit assumptions in code. I was 
bitten
>> >> tooooo may times by exactly this in BIND and I don't think we should do 
that
>> >> in our code.
>> >>
>> >> Alexander, if you really don't want to move krb5_unparse_name() call then 
we
>> >> really need to handle this "impossible" case in some other way. Maybe 
with an
>> >> assert()? It would do nothing as long as your assumption holds and will
>> >> clearly show where the problem is if we break the assumption in future.
>> >>
>> >
>> >The problem here is that we are unconditionally calling
>> >ipadb_entry_to_mods() and passing 'principal' to it.
>> ipadb_entry_to_mods() is fine with principal == NULL because entry->mask
>> doesn't have KMASK_PRINCIPAL in this case. ipadb_modify_principal() is
>> just a helper to make ipadb_put_principal() a simpler logic to
>> understand.
>>
>> Note that in other parts of the code we call ipadb_put_principal(),
>> currently only in ipa_kdb_audit_as.c, so this is our interface to the
>> external users, including the rest of KDC.
>
>Yeah in fact I was pondering changing ipadb_entry_to_mods() to make sure
>'principal' actually is not NULL. I would consider it a good defensive
>measure if Martin wants to add it to this patch.
If you want to go this way, then move out setting principal in
ipadb_entry_to_mods() to a separate function and call it explicitly in
case principal != NULL. The reason for that is because our cached entry
will have ied->entry_dn non-NULL so it means all cached entries will get
to ipadb_entry_to_mods() with principal == NULL. This is our ideal case,
yet you would break it if you'd require 'principal' is not NULL in
ipadb_entry_to_mods().


I never said I require it to be not null, I just said that the function
should check it is not null when entry->mask & KMASK_PRINCIPAL is true.
Currently we do not check, we should check:
if (entry->mask & KMASK_PRINCIPAL) {
  if (principal == NULL) { -> error out }
}

That's all I said.
Ok, this is also a possiblity in ipadb_entry_to_mods().

I would, however, prefer singling this part out to make clear in the
callers ipadb_add_principal() and ipadb_modify_principal() that we
handle 'principal' separately from the rest of the fields.

An attempt is attached.

--
/ Alexander Bokovoy
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c 
b/daemons/ipa-kdb/ipa_kdb_principals.c
index e158c23..735ac12 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -1474,10 +1474,42 @@ 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;
+    }
+
+
+    /* 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;
+        }
+    }
+
+    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;
@@ -1863,8 +1895,14 @@ static krb5_error_code ipadb_add_principal(krb5_context 
kcontext,
         goto done;
     }
 
+    kerr = ipadb_principal_to_mods(kcontext, imods,
+                                   principal, LDAP_MOD_ADD);
+    if (kerr != 0) {
+        goto done;
+    }
+
     kerr = ipadb_entry_to_mods(kcontext, imods,
-                               entry, principal, LDAP_MOD_ADD);
+                               entry, LDAP_MOD_ADD);
     if (kerr != 0) {
         goto done;
     }
@@ -1895,6 +1933,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,11 +1962,13 @@ 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,
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to