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

Tue Jan 27 18:00:09 UTC 2015

On 27.1.2015 18:41, Alexander Bokovoy wrote:
> On Tue, 27 Jan 2015, Petr Spacek wrote:
>> 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 <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.
>>> 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.
> assert() (with exit) is wrong here. Logging a note -- OK.
> 
> I'm opposing moving the code because this is one of functions that gets
> called for _every_ Kerberos authentication as we modify last
> authentication time.

Does it matter? Does krb5_unparse_name() have an significant performance
impact? (I did not read the code so I'm asking.)

Given that the function is in authentication path, I think that it should fail
if original assumptions do not hold anymore.

I.e. I would modify the code to do this:
if (principal == NULL && entry->mask & KMASK_PRINCIPAL) {
    krberr = KRB5_KDB_INTERNAL_ERROR;
    log("Sky is falling!");
    goto done;
}

This would require review if all code under done: label can handle all NULL
pointers gracefully.

... Isn't it too much work instead of one assert()? For an error condition
which cannot ever happen (you claimed)? :-)

BTW
there is also an unconditional dereference
ied = (struct ipadb_e_data *)entry->e_data;
without check that entry is not NULL. Maybe it can not happen in current code
but it should be supplemented with assert(entry != NULL) or other check.

-- 
Petr Spacek  @  Red Hat