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.
The very fact we've gone over this a few times and we are discussing it
means the interface is not clear and can lead to errors in future. So we
need to improve the code flow.
> >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.
> The question is what we want to achieve. What KMASK_PRINCIPAL is set on
> the entry, we create a new principal. If it is not set, we modify
> existing principal. That's it.
Sorry I fail to parse this statement, my aim is to make the code clearer
so that future modifications will not trip on this issue.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
