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

[Simo Sorce]

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

The problem here is that we are unconditionally calling
ipadb_entry_to_mods() and passing 'principal' to it.

If we do not call krb5_unparse_name() then principal is NULL.

If we want to keep parsing the principal as a conditional I am ok with
that, but then we need to test the right condition, the code should be:

+if (!ied || !ied->entry_dn || entry->mask & KMASK_PRINCIPAL) {
+    .. krb5_unparse_name ..
+}

if (!ied || !ied->entry_dn) {
-   .. krb5_unparse_name ..
...

Later on again we call, again unconditionally krb5_free_unparsed_name()
to which we pass principal again, but this function is safe when
principal is NULL.

Simo.


-- 
Simo Sorce * Red Hat, Inc * New York