[Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
Alexander Bokovoy
abokovoy at redhat.com
Wed Jan 28 12:19:21 UTC 2015
On Wed, 28 Jan 2015, Martin Babinsky wrote:
>On 01/28/2015 12:11 PM, Alexander Bokovoy wrote:
>>On Wed, 28 Jan 2015, Martin Babinsky wrote:
>>>>>can you pick this up as the way to fix this coverity issue ?
>>>>>
>>>>>Simo.
>>>>>
>>>>Yes I will try to implement it and post all the updates ASAP.
>>>>
>>>
>>>I have tried to incorporate Alexander's patch to the
>>>'ipa_kdb_principals.c'. However covscan is still not happy about it
>>>and reports FORWARD_NULL defect:
>>>
>>>"""
>>>Error: FORWARD_NULL (CWE-476):
>>>freeipa-4.1.99.201501280935GITbeaceb7/daemons/ipa-kdb/ipa_kdb_principals.c:1926:
>>>assign_zero: Assigning: "principal" = "NULL".
>>>freeipa-4.1.99.201501280935GITbeaceb7/daemons/ipa-kdb/ipa_kdb_principals.c:1974:
>>>var_deref_model: Passing null pointer "principal" to
>>>"ipadb_entry_to_mods", which dereferences it.
>>>freeipa-4.1.99.201501280935GITbeaceb7/daemons/ipa-kdb/ipa_kdb_principals.c:1525:9:
>>>deref_parm_in_call: Function "ipadb_get_ldap_mod_str" dereferences
>>>"principal".
>>>freeipa-4.1.99.201501280935GITbeaceb7/daemons/ipa-kdb/ipa_kdb_principals.c:1174:5:
>>>deref_parm_in_call: Function "strdup" dereferences "value".
>>>"""
>>>
>>>Should I keep the changes and add annotation marking it as false
>>>positive?
>>You actually missed part of my patch which removed 'principal' argument
>>of ipadb_entry_to_mods(). When 'principal' processing is moved to a
>>separate function, 'principal' is not needed anymore in the
>>ipadb_entry_to_mods() and thus can be removed. I didn't finish removing
>>the actual code for the KMASK_PRINCIPAL from the ipadb_entry_to_mods()
>>and this is what Simo called is 'incomplete' in my patch.
>>
>>Removing it would remove errors reported by covscan.
>>
>>
>Oh I'm sorry Alexander, looks like my ability to comprehend written
>text "excels" today.
>
>I'm attaching the updated patch. This one really fixes the defect. If
>it's OK then I will once again send the whole series of patches
>(including updated ones) for review.
Few small comments inline in the patch after I re-read it in more
detail. :)
>
>--
>Martin^3 Babinsky
>From 66367394feebced7ca712075dc804becad6ae667 Mon Sep 17 00:00:00 2001
>From: Martin Babinsky <mbabinsk at redhat.com>
>Date: Wed, 28 Jan 2015 12:53:22 +0100
>Subject: [PATCH 3/7] proposed a fix to 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"
>"""
>
>This is a part of series of patches related to
>https://fedorahosted.org/freeipa/ticket/4795
>
>---
> daemons/ipa-kdb/ipa_kdb_principals.c | 78 ++++++++++++++++++++++++------------
> 1 file changed, 53 insertions(+), 25 deletions(-)
>
>diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
>index e158c236eab5c7c5a7c12664dbde5d51cc55406d..b2752d974a7441851903a44d1a983ff180f7cb4f 100644
>--- a/daemons/ipa-kdb/ipa_kdb_principals.c
>+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
>@@ -1474,10 +1474,43 @@ done:
> return kerr;
> }
>
>+static krb5_error_code ipadb_principal_to_mods(krb5_context kcontext,
>+ struct ipadb_mods *imods,
>+ krb5_db_entry *entry,
you can remove 'entry' argument here.
Given that we call ipadb_principal_to_mods() explicitly now, the check
for (entry->mask & KMASK_PRINCIPAL) is not needed at this level.
If 'principal' is NULL, we'll bail out. If it is not NULL, we add it
into mods, this makes the whole independent of 'entry'. The
differentiation based on the KMASK_PRINCIPAL happens in
ipadb_put_principal() where we decide what higher level helper to call
-- to add principal or to modify it. In both cases we actually add
'principal' to the 'imods' but with different LDAP operation (ADD or
REPLACE).
>+ char *principal,
>+ int mod_op)
>+{
>+ krb5_error_code kerr;
>+
>+ if (principal == NULL) {
>+ kerr = EINVAL;
>+ goto done;
>+ }
>+
>+
>+ /* KADM5_PRINCIPAL */
>+ if (entry->mask & KMASK_PRINCIPAL) {
you can now remove this condition.
>+ kerr = ipadb_get_ldap_mod_str(imods, "krbPrincipalName",
>+ principal, mod_op);
>+ if (kerr) {
>+ goto done;
>+ }
>+ kerr = ipadb_get_ldap_mod_str(imods, "ipaKrbPrincipalAlias",
>+ principal, mod_op);
>+ if (kerr) {
>+ goto done;
>+ }
>+ }
>+
>+ kerr = 0;
>+
>+done:
>+ return kerr;
>+}
>+
> static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
> struct ipadb_mods *imods,
> krb5_db_entry *entry,
>- char *principal,
> int mod_op)
> {
> krb5_error_code kerr;
>@@ -1486,20 +1519,6 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
>
> /* check each mask flag in order */
>
>- /* KADM5_PRINCIPAL */
>- if (entry->mask & KMASK_PRINCIPAL) {
>- kerr = ipadb_get_ldap_mod_str(imods, "krbPrincipalName",
>- principal, mod_op);
>- if (kerr) {
>- goto done;
>- }
>- kerr = ipadb_get_ldap_mod_str(imods, "ipaKrbPrincipalAlias",
>- principal, mod_op);
>- if (kerr) {
>- goto done;
>- }
>- }
>-
> /* KADM5_PRINC_EXPIRE_TIME */
> if (entry->mask & KMASK_PRINC_EXPIRE_TIME) {
> kerr = ipadb_get_ldap_mod_time(imods,
>@@ -1863,8 +1882,13 @@ static krb5_error_code ipadb_add_principal(krb5_context kcontext,
> goto done;
> }
>
>- kerr = ipadb_entry_to_mods(kcontext, imods,
>- entry, principal, LDAP_MOD_ADD);
>+ kerr = ipadb_principal_to_mods(kcontext, imods, entry,
>+ principal, LDAP_MOD_ADD);
>+ if (kerr != 0) {
>+ goto done;
>+ }
>+
>+ kerr = ipadb_entry_to_mods(kcontext, imods, entry, LDAP_MOD_ADD);
> if (kerr != 0) {
> goto done;
> }
>@@ -1895,18 +1919,21 @@ static krb5_error_code ipadb_modify_principal(krb5_context kcontext,
> return KRB5_KDB_DBNOTINITED;
> }
>
>+ kerr = new_ipadb_mods(&imods);
>+ if (kerr) {
>+ 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);
>@@ -1919,15 +1946,16 @@ static krb5_error_code ipadb_modify_principal(krb5_context kcontext,
> kerr = KRB5_KDB_INTERNAL_ERROR;
> goto done;
> }
>- }
>
>- kerr = new_ipadb_mods(&imods);
>- if (kerr) {
>- goto done;
>+ kerr = ipadb_principal_to_mods(kcontext, imods, entry,
>+ principal, LDAP_MOD_REPLACE);
>+ if (kerr != 0) {
>+ goto done;
>+ }
>+
> }
>
>- kerr = ipadb_entry_to_mods(kcontext, imods,
>- entry, principal, LDAP_MOD_REPLACE);
>+ kerr = ipadb_entry_to_mods(kcontext, imods, entry, LDAP_MOD_REPLACE);
> if (kerr != 0) {
> goto done;
> }
>--
>2.1.0
>
--
/ Alexander Bokovoy
More information about the Freeipa-devel
mailing list