On 27.1.2015 18:23, Alexander Bokovoy wrote: > On Tue, 27 Jan 2015, 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. > What about following just before creating mods? > > if (principal == NULL && entry->mask & KMASK_PRINCIPAL) { > goto done; > }
I don't know this piece of code so I can't say anything in particular. Are you 100 % sure that it will not cause any harm in future? I'm hesitant to introduce a goto branching which is not testable nowadays. Personally I would prefer either assert() or moving the code as proposed in the patch. Thank you for understanding. -- Petr Spacek @ Red Hat _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
