[Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master

[Petr Spacek]

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 <mbabinsk at 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.

-- 
Petr^2 Spacek