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.
--
Martin^3 Babinsky
From 66367394feebced7ca712075dc804becad6ae667 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@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,
+ char *principal,
+ int mod_op)
+{
+ krb5_error_code kerr;
+
+ if (principal == NULL) {
+ kerr = EINVAL;
+ goto done;
+ }
+
+
+ /* 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;
+ }
+ }
+
+ 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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
