Re: [Freeipa-devel] [PATCH 003] Use 'remove-ds.pl' to remove DS instance during server uninstall
On 01/26/2015 05:46 PM, Martin Basti wrote:
On 26/01/15 14:05, Martin Babinsky wrote:
On 01/26/2015 01:59 PM, Martin Babinsky wrote:
On 01/23/2015 05:56 PM, Martin Basti wrote:
On 22/01/15 15:03, Martin Babinsky wrote:
On 01/22/2015 12:38 PM, Martin Babinsky wrote:
On 01/22/2015 12:19 PM, Martin Kosek wrote:
On 01/22/2015 11:58 AM, Martin Babinsky wrote:
On 01/22/2015 11:04 AM, Martin Babinsky wrote:
The attached patch addresses
https://fedorahosted.org/freeipa/ticket/4487.
Using 'remove-ds.pl' script from 389-ds during server/replica
uninstallation should allow for cleaner removal of DS instance
with no
leftovers (opened ports etc).
Martin^3
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Thanks to Martin^2 for explaining the rules governing the placement
of new
attributes into BasePathNamespace class.
Attaching updated patch.
Martin^3
I see you kept erase_ds_instance_data still in the dsinstance. What is
the
motivation? I thought that just like with PKI, with DS we will also
have
uninstall based solely only on the DS uninstall script and remove our
custom
removal.
We can keep the manual removal somewhere in wiki, but I would
personally not
keep/maintain it in FreeIPA code.
I originally thought that I will keep erase_ds_instance-data as a
failsafe method to clean up the dirsrv data in th case that
remove-ds.pl
fails.
But I will remove the method altogether and change the code so that it
prints out some warning about manual removal of DS data when
remove-ds.pl fails.
Martin^2
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Updated patch attached
Martin^3
Hi,
thank you for patch, but I have a few nitpicks:
1) Extra parenthesis
+root_logger.error((Failed to remove CA DS instance.
You may
+ need to remove instance data
manually))
and (twice)
+root_logger.error((Failed to remove DS instance. You
may
+ need to remove instance data
manually))
and
+root_logger.debug(('%s' failed.
+ Attempting to force removal %
paths.REMOVE_DS_PL))
2) Remove unused paths:
USR_LIB_DIRSRV_SLAPD_INSTANCE_DIR_TEMPLATE
SLAPD_INSTANCE_LOCK_TEMPLATE
USR_LIB_SLAPD_INSTANCE_TEMPLATE
Please do double check.
3) IMO we should avoid hardcoded value 'slapd'
instance_name = '-'.join(['slapd', serverid])
you may create module variable DS_INSTANCE_PREFIX and use it. Dont
forget to change it in following place as well.
ipaserver/install/dsinstance.py:117:instance_prefix = 'slapd-'
4)
DS keytab hasn't been removed, it is okay?
It is created by IPA (krbinstance), not by DS install script.
Martin^2
Thanks for suggestions, attaching updated patch
Martin^3
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Missed one little thing. Attaching updated patch.
Martin^3
Thank you, ACK.
Pushed to master: 55b7eed77e5f76c159ba157d020e93aa9d43bdc5
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
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
to 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
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
On Tue, 27 Jan 2015, Petr Spacek wrote:
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.)
It is calling malloc()/realloc() so yes, it does matter. We cache
information about principal via DN in ied-entry_dn because we need the DN
instead of the principal in most cases except the case of adding a
principal to the database.
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)? :-)
What would assert do? abort() the process when compiled without NDEBUG?
I don't think it is what we need for the KDC.
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.
Sometimes line has to be drawn somewhere. Whole MIT Kerberos is built
in this style where defence is happening in upper layers.
--
/ Alexander Bokovoy
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 397 Do not crash when replica is unreachable in ipa-restore
On 01/27/2015 07:59 PM, Rob Crittenden wrote: Martin Kosek wrote: On 01/27/2015 08:40 AM, Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4857. Honza Works like a charm, ACK. Pushed to: master: deb70d5b13ce0e7ec77debb4aa17d75df4c1dedd ipa-4-1: 74853b66f092a057c22ee811e945f631e6d65059 Sorry I missed this earlier, but this could be a timebomb. Ah, and I saw that one as a clear one. It means that there is some master out there that still has its old changelog and is waiting to push changes you may not want back to the restored master(s). This is a long shot, but doesn't changes done in https://fedorahosted.org/freeipa/ticket/4822 prevent other masters to sent updates and actually force them to re-initialize from restored master? Also CCing Thierry. It would definitely be worth testing a scenario like this: 3 masters, A, B, C. Backup A Add a bunch of data shut down C Restore A Re-init B Confirm that that data you added is gone Start up C See what happens. I suspect that data will be re-added. If this is the case, should we print big fat warning in ipa-restore Some of your replication agreements could not be disabled, there are the consequences... yadda yadda yadda... Are you sure you want to continue?? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 397 Do not crash when replica is unreachable in ipa-restore
Martin Kosek wrote: On 01/27/2015 08:40 AM, Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4857. Honza Works like a charm, ACK. Pushed to: master: deb70d5b13ce0e7ec77debb4aa17d75df4c1dedd ipa-4-1: 74853b66f092a057c22ee811e945f631e6d65059 Sorry I missed this earlier, but this could be a timebomb. It means that there is some master out there that still has its old changelog and is waiting to push changes you may not want back to the restored master(s). It would definitely be worth testing a scenario like this: 3 masters, A, B, C. Backup A Add a bunch of data shut down C Restore A Re-init B Confirm that that data you added is gone Start up C See what happens. I suspect that data will be re-added. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
On Tue, 27 Jan 2015, Simo Sorce wrote:
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
to 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.
If you want to go this way, then move out setting principal in
ipadb_entry_to_mods() to a separate function and call it explicitly in
case principal !=
[Freeipa-devel] [PATCH 0005] Fixed some of the issues reported in FreeIPA code by covscan
The attached patch is related to
https://fedorahosted.org/freeipa/ticket/4795 and fixes (hopefully) some
of the defects reported by subsequent scans.
There are also 21 defects reported in asn1/asn1c/*.c files
(http://cov01.lab.eng.brq.redhat.com/covscanhub/task/16545/log/freeipa-4.1.99.201501261541GIT871f9bb-0.fc21/scan-results.html).
Since this code is (semi)-automatically generated by asn1c software, we
should decide what to do with them.
Should I try to fix them by hand and/or report them upstream?
Martin^3
From 4732626ed0fb8ec0fb2074c55955ab570eac58fa Mon Sep 17 00:00:00 2001
From: Martin Babinsky mbabi...@redhat.com
Date: Fri, 16 Jan 2015 15:43:17 +0100
Subject: [PATCH] Fixed issues reported in FreeIPA code by covscan
This patch is related to https://fedorahosted.org/freeipa/ticket/4795.
Performed another scan and fixed/waived some defects reported by Coverity in
IPA C code.
---
daemons/ipa-kdb/ipa_kdb_audit_as.c| 5 +
daemons/ipa-kdb/ipa_kdb_mspac.c | 5 +
daemons/ipa-kdb/ipa_kdb_principals.c | 11 ---
daemons/ipa-sam/ipa_sam.c | 2 +-
.../ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c | 8 ++--
daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 4 +++-
daemons/ipa-slapi-plugins/ipa-uuid/ipa_uuid.c | 3 ++-
daemons/ipa-slapi-plugins/libotp/Makefile.am | 4 +++-
daemons/ipa-slapi-plugins/libotp/otp_config.c | 9 -
util/ipa_krb5.h | 2 ++
10 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_audit_as.c b/daemons/ipa-kdb/ipa_kdb_audit_as.c
index 52c165442bde61d3ce88843b122aae7fe0fae50b..81ccbc2de28681c9c90b932fb14831965e0b246c 100644
--- a/daemons/ipa-kdb/ipa_kdb_audit_as.c
+++ b/daemons/ipa-kdb/ipa_kdb_audit_as.c
@@ -20,6 +20,7 @@
* along with this program. If not, see http://www.gnu.org/licenses/.
*/
+#include syslog.h
#include ipa_kdb.h
#include ipa_pwd.h
@@ -120,7 +121,11 @@ void ipadb_audit_as_req(krb5_context kcontext,
client-last_failed = authtime;
client-mask |= KMASK_LAST_FAILED;
break;
+/*coverity[dead_error_begin]*/
default:
+krb5_klog_syslog(LOG_ERR,
+ Got an unexpected value of error_code: %d\n,
+ error_code);
return;
}
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index a4500070760e83994c8155a12ee6414b5ebee9e0..0f47d1f4bd536e24b9d46a35232ad558b33b4b26 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -54,9 +54,6 @@ struct ipadb_mspac {
time_t last_update;
};
-
-int krb5_klog_syslog(int, const char *, ...);
-
static char *user_pac_attrs[] = {
objectClass,
uid,
@@ -2074,7 +2071,7 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
}
}
-kerr = ipadb_get_pac(context, client_entry ? client_entry : client, pac);
+kerr = ipadb_get_pac(context, client, pac);
if (kerr != 0 kerr != ENOENT) {
goto done;
}
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);
diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
index 07249fd27b362ed6499e372d651192dfc31b5173..ea9c1503f40bfc288703d985530a66539b7d 100644
--- a/daemons/ipa-sam/ipa_sam.c
+++ b/daemons/ipa-sam/ipa_sam.c
@@ -1487,7 +1487,7 @@ static bool ldapgroup2displayentry(struct ldap_search_state *state,
return false;
}
break;
-
+ /*coverity[dead_error_begin]*/
default:
DEBUG(0,(unknown group type: %d\n, group_type));
talloc_free(sid);
diff --git a/daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c b/daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c
index
Re: [Freeipa-devel] [PATCH 0005] Fixed some of the issues reported in FreeIPA code by covscan
On Tue, 27 Jan 2015, Martin Babinsky wrote:
The attached patch is related to
https://fedorahosted.org/freeipa/ticket/4795 and fixes (hopefully)
some of the defects reported by subsequent scans.
NACK overall. If you want to provide fixes, make them separate of each
other and explain each fix. See below for details.
There are also 21 defects reported in asn1/asn1c/*.c files (http://cov01.lab.eng.brq.redhat.com/covscanhub/task/16545/log/freeipa-4.1.99.201501261541GIT871f9bb-0.fc21/scan-results.html).
Since this code is (semi)-automatically generated by asn1c software,
we should decide what to do with them.
Should I try to fix them by hand and/or report them upstream?
No manual fixes, we'll need to find out a way to fix them upstream.
Martin^3
From 4732626ed0fb8ec0fb2074c55955ab570eac58fa Mon Sep 17 00:00:00 2001
From: Martin Babinsky mbabi...@redhat.com
Date: Fri, 16 Jan 2015 15:43:17 +0100
Subject: [PATCH] Fixed issues reported in FreeIPA code by covscan
This patch is related to https://fedorahosted.org/freeipa/ticket/4795.
Performed another scan and fixed/waived some defects reported by Coverity in
IPA C code.
---
daemons/ipa-kdb/ipa_kdb_audit_as.c| 5 +
daemons/ipa-kdb/ipa_kdb_mspac.c | 5 +
daemons/ipa-kdb/ipa_kdb_principals.c | 11 ---
daemons/ipa-sam/ipa_sam.c | 2 +-
.../ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c | 8 ++--
daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 4 +++-
daemons/ipa-slapi-plugins/ipa-uuid/ipa_uuid.c | 3 ++-
daemons/ipa-slapi-plugins/libotp/Makefile.am | 4 +++-
daemons/ipa-slapi-plugins/libotp/otp_config.c | 9 -
util/ipa_krb5.h | 2 ++
10 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_audit_as.c
b/daemons/ipa-kdb/ipa_kdb_audit_as.c
index
52c165442bde61d3ce88843b122aae7fe0fae50b..81ccbc2de28681c9c90b932fb14831965e0b246c
100644
--- a/daemons/ipa-kdb/ipa_kdb_audit_as.c
+++ b/daemons/ipa-kdb/ipa_kdb_audit_as.c
@@ -20,6 +20,7 @@
* along with this program. If not, see http://www.gnu.org/licenses/.
*/
+#include syslog.h
#include ipa_kdb.h
#include ipa_pwd.h
NACK, why do you need syslog.h here?
@@ -120,7 +121,11 @@ void ipadb_audit_as_req(krb5_context kcontext,
client-last_failed = authtime;
client-mask |= KMASK_LAST_FAILED;
break;
+/*coverity[dead_error_begin]*/
default:
+krb5_klog_syslog(LOG_ERR,
+ Got an unexpected value of error_code: %d\n,
+ error_code);
return;
}
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index
a4500070760e83994c8155a12ee6414b5ebee9e0..0f47d1f4bd536e24b9d46a35232ad558b33b4b26
100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -54,9 +54,6 @@ struct ipadb_mspac {
time_t last_update;
};
-
-int krb5_klog_syslog(int, const char *, ...);
-
static char *user_pac_attrs[] = {
objectClass,
uid,
What does this fix have to do with coverity?
Please separate patches and submit one by one.
@@ -2074,7 +2071,7 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
}
}
-kerr = ipadb_get_pac(context, client_entry ? client_entry : client,
pac);
+kerr = ipadb_get_pac(context, client, pac);
if (kerr != 0 kerr != ENOENT) {
goto done;
}
NACK, this change is wrong and breaks conditional delegation.
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);
NACK, we only want to touch entry-princ and fetch it if it does not
exist. The cost of these operations is sufficient enough to postpone.
diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
index
Re: [Freeipa-devel] [PATCH 0004] added dbus-python dependency to freeipa-client
On 26/01/15 17:36, Martin Babinsky wrote: See attached patch related to https://fedorahosted.org/freeipa/ticket/4863. Martin^3 Thank for your patch, IMO client is not dependent on dbus module, but ipapython requires dbus module. We should add this dependency to ipapython package, because AFAIK ipa-client-install do not need dbus at all. Martin^2 -- Martin Basti ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
On Tue, 2015-01-27 at 22:20 +0200, Alexander Bokovoy wrote:
On Tue, 27 Jan 2015, Simo Sorce wrote:
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
to 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.
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
On Tue, 2015-01-27 at 23:04 +0200, Alexander Bokovoy wrote:
On Tue, 27 Jan 2015, Simo Sorce wrote:
On Tue, 2015-01-27 at 22:20 +0200, Alexander Bokovoy wrote:
On Tue, 27 Jan 2015, Simo Sorce wrote:
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
to 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
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
On Tue, 27 Jan 2015, Simo Sorce wrote:
On Tue, 2015-01-27 at 22:20 +0200, Alexander Bokovoy wrote:
On Tue, 27 Jan 2015, Simo Sorce wrote:
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
to 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
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
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
to 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.
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
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
On 01/27/2015 10:15 PM, Simo Sorce wrote:
On Tue, 2015-01-27 at 23:04 +0200, Alexander Bokovoy wrote:
On Tue, 27 Jan 2015, Simo Sorce wrote:
On Tue, 2015-01-27 at 22:20 +0200, Alexander Bokovoy wrote:
On Tue, 27 Jan 2015, Simo Sorce wrote:
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
to 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.
If you want to go this way, then move out setting principal in
ipadb_entry_to_mods() to a separate
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
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
to 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.
--
Petr^2 Spacek
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 301-302] ID override sshpubkey handling
On 01/26/2015 04:33 PM, Tomas Babej wrote:
Hi,
the following two patches make sure that sshpubkeys work both with -mod and
-add commands of ipaoverrideuser objects. Also covers the use cases with unit
tests.
https://fedorahosted.org/freeipa/ticket/4868
Tomas
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Hi,
thanks for the patches but right now we need just a small fix for
ipa-4-1 (attached). Your patches will latter go into ipa-4-2.
--
David Kupka
From 02c42c4935013e711563da88bb2da75700ba6e11 Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Tue, 27 Jan 2015 16:12:19 +0100
Subject: [PATCH] idviews: Allow setting ssh public key on ipauseroverride-add
https://fedorahosted.org/freeipa/ticket/4868
---
ipalib/plugins/idviews.py | 3 +++
1 file changed, 3 insertions(+)
diff --git a/ipalib/plugins/idviews.py b/ipalib/plugins/idviews.py
index df6b80fee6239c97e2133885234408c2816b3774..df403b1193fe18dfadf437a18a3e0b6ffb7575b4 100644
--- a/ipalib/plugins/idviews.py
+++ b/ipalib/plugins/idviews.py
@@ -672,6 +672,7 @@ class idoverrideuser(baseidoverride):
}
object_class = baseidoverride.object_class + ['ipaUserOverride']
+possible_objectclasses = ['ipasshuser', 'ipaSshGroupOfPubKeys']
default_attributes = baseidoverride.default_attributes + [
'homeDirectory', 'uidNumber', 'uid', 'ipaOriginalUid', 'loginShell',
'ipaSshPubkey', 'gidNumber', 'gecos',
@@ -786,6 +787,8 @@ class idoverrideuser_add(baseidoverride_add):
dn = super(idoverrideuser_add, self).pre_callback(ldap, dn,
entry_attrs, attrs_list, *keys, **options)
+entry_attrs['objectclass'].append('ipasshuser')
+
# Update the ipaOriginalUid
self.obj.update_original_uid_reference(entry_attrs)
return dn
--
2.1.0
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 397 Do not crash when replica is unreachable in ipa-restore
On 01/27/2015 08:40 AM, Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4857. Honza Works like a charm, ACK. Pushed to: master: deb70d5b13ce0e7ec77debb4aa17d75df4c1dedd ipa-4-1: 74853b66f092a057c22ee811e945f631e6d65059 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 301-302] ID override sshpubkey handling
Dne 27.1.2015 v 16:22 David Kupka napsal(a): On 01/26/2015 04:33 PM, Tomas Babej wrote: Hi, the following two patches make sure that sshpubkeys work both with -mod and -add commands of ipaoverrideuser objects. Also covers the use cases with unit tests. https://fedorahosted.org/freeipa/ticket/4868 Tomas ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hi, thanks for the patches but right now we need just a small fix for ipa-4-1 (attached). Thanks, ACK. Pushed to: master: 3b87302f5a280c044a8e6a8b4aa08a29e3b4b0d5 ipa-4-1: 0dc7448b3634be443806db45ffead57107213ad6 Your patches will latter go into ipa-4-2. +1 -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
On 01/27/2015 06:05 PM, Petr Spacek wrote:
On 27.1.2015 18:02, Alexander Bokovoy wrote:
-slapi_search_internal_get_entry(sdn, attrs, entry,
-otp_config_plugin_id(otp_config));
+search_result = slapi_search_internal_get_entry(sdn, attrs, entry,
+otp_config_plugin_id(otp_config));
+if (search_result != LDAP_SUCCESS) {
+LOG_TRACE(Entry not found. Error code: %d\n, search_result);
+}
I would rather say something more defensive here:
Unable to access LDAP entry. Perhaps it does not exist? Error code: %d\n
Would it be possible to print the DN? It may be useful in cases where it
actually happened ...
I will look into it.
Martin^3
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
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 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
to 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.
Thank you for understanding.
--
Petr Spacek @ Red Hat
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
This series of patches is related to
https://fedorahosted.org/freeipa/ticket/4795.
The attached patches attempt to address some of the defects encountered
during running covscan on freeipa-master branch.
The complete list is here:
http://cov01.lab.eng.brq.redhat.com/covscanhub/task/16553/log/freeipa-4.1.99.201501270952GITc90286c-0.fc21/scan-results.html
These patches do not fix defects 1 to 22 which occur in a code generated
by asn1c, defect 23 which is related to
https://fedorahosted.org/freeipa/ticket/4861, and defect 27 (dead code).
Each patch contains a detailed description of a defect and a proposed
fix to address it.
Martin^3
From 5ae083c1dcfc1fbe0deb0ee7baf78d3509c60551 Mon Sep 17 00:00:00 2001
From: Martin Babinsky mbabi...@redhat.com
Date: Tue, 27 Jan 2015 13:01:43 +0100
Subject: [PATCH 1/7] added warning message after default: branch
This patch is related this defect reported by covscan on FreeIPA master:
Error: DEADCODE (CWE-561):
/daemons/ipa-kdb/ipa_kdb_audit_as.c:42: cond_const: Condition error_code !=
-1765328353L, taking false branch. Now the value of error_code is equal to
-1765328353.
/daemons/ipa-kdb/ipa_kdb_audit_as.c:42: cond_const: Condition
error_code != -1765328360L, taking false branch. Now the value of
error_code is equal to -1765328360.
/daemons/ipa-kdb/ipa_kdb_audit_as.c:42:
cond_const: Condition error_code != 0, taking false branch. Now the value of
error_code is equal to 0.
/daemons/ipa-kdb/ipa_kdb_audit_as.c:71:
intervals: When switching on error_code, the value of error_code must be
in one of the following intervals: {[-1765328360,-1765328360],
[-1765328353,-1765328353], [0,0]}.
/daemons/ipa-kdb/ipa_kdb_audit_as.c:71:
dead_error_condition: The switch value error_code cannot reach the default
case.
/daemons/ipa-kdb/ipa_kdb_audit_as.c:123: dead_error_begin: Execution
cannot reach this statement: default:.
pspacek pointed out that even if there is a part of code that is practically
unreachable under normal circumstances, we should have a way to emit a warning
message if something strange happens (e.g. memory corruption) and this code is
executed.
That's why I moved 'krb5_klog_syslog' function to 'util/ipa_krb5.h' (to enable
logging for all krb5 related code) and added a warning message.
I don't know if this change is desirable or no so please let me know.
This patch is a part of series related to
https://fedorahosted.org/freeipa/ticket/4795.
---
daemons/ipa-kdb/ipa_kdb_audit_as.c | 4
daemons/ipa-kdb/ipa_kdb_mspac.c| 3 ---
util/ipa_krb5.h| 2 ++
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_audit_as.c b/daemons/ipa-kdb/ipa_kdb_audit_as.c
index 52c165442bde61d3ce88843b122aae7fe0fae50b..5b2a6755a760662b50174aec92af56d6cb14c522 100644
--- a/daemons/ipa-kdb/ipa_kdb_audit_as.c
+++ b/daemons/ipa-kdb/ipa_kdb_audit_as.c
@@ -20,6 +20,7 @@
* along with this program. If not, see http://www.gnu.org/licenses/.
*/
+#include syslog.h
#include ipa_kdb.h
#include ipa_pwd.h
@@ -121,6 +122,9 @@ void ipadb_audit_as_req(krb5_context kcontext,
client-mask |= KMASK_LAST_FAILED;
break;
default:
+krb5_klog_syslog(LOG_ERR,
+ Got an unexpected value of error_code: %d\n,
+ error_code);
return;
}
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index a4500070760e83994c8155a12ee6414b5ebee9e0..22774e02309f0715b49545e0f6f21d599e7afe0a 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -54,9 +54,6 @@ struct ipadb_mspac {
time_t last_update;
};
-
-int krb5_klog_syslog(int, const char *, ...);
-
static char *user_pac_attrs[] = {
objectClass,
uid,
diff --git a/util/ipa_krb5.h b/util/ipa_krb5.h
index 7b877aa665dd6cb4e0c1cf9d8153319cc8f61a20..2153bd57142d1468031d0aa4b5d3f59ef5c890b5 100644
--- a/util/ipa_krb5.h
+++ b/util/ipa_krb5.h
@@ -30,6 +30,8 @@ struct keys_container {
#define KEYTAB_RET_OID 2.16.840.1.113730.3.8.10.2
#define KEYTAB_GET_OID 2.16.840.1.113730.3.8.10.5
+int krb5_klog_syslog(int, const char *, ...);
+
void
ipa_krb5_free_ktypes(krb5_context context, krb5_enctype *val);
--
2.1.0
From 463d521c6a1872e2a00c33163936cd985066a027 Mon Sep 17 00:00:00 2001
From: Martin Babinsky mbabi...@redhat.com
Date: Tue, 27 Jan 2015 13:15:43 +0100
Subject: [PATCH 2/7] proposed fix for a defect reported in 'ipa_kdb_mspac.c'
This patch proposes a fix for the following defect reported by covscan in
FreeIPA master code:
Error: DEADCODE (CWE-561):
/daemons/ipa-kdb/ipa_kdb_mspac.c:2013: assignment: Assigning: client_entry =
NULL.
/daemons/ipa-kdb/ipa_kdb_mspac.c:2077: null: At condition
client_entry, the value of client_entry must be NULL.
/daemons/ipa-kdb/ipa_kdb_mspac.c:2077: dead_error_condition: The condition
client_entry cannot be true.
/daemons/ipa-kdb/ipa_kdb_mspac.c:2077:
dead_error_line: Execution cannot reach
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
On 27.1.2015 18:02, Alexander Bokovoy wrote:
-slapi_search_internal_get_entry(sdn, attrs, entry,
-otp_config_plugin_id(otp_config));
+search_result = slapi_search_internal_get_entry(sdn, attrs, entry,
+otp_config_plugin_id(otp_config));
+if (search_result != LDAP_SUCCESS) {
+LOG_TRACE(Entry not found. Error code: %d\n, search_result);
+}
I would rather say something more defensive here:
Unable to access LDAP entry. Perhaps it does not exist? Error code: %d\n
Would it be possible to print the DN? It may be useful in cases where it
actually happened ...
--
Petr^2 Spacek
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0004] added dbus-python dependency to freeipa-client
On 01/27/2015 10:41 AM, Martin Basti wrote:
On 26/01/15 17:36, Martin Babinsky wrote:
See attached patch related to
https://fedorahosted.org/freeipa/ticket/4863.
Martin^3
Thank for your patch,
IMO client is not dependent on dbus module, but ipapython requires dbus
module.
We should add this dependency to ipapython package, because AFAIK
ipa-client-install do not need dbus at all.
Martin^2
True indeed. I have moved 'dbus-python' dependency to freeipa-python and
removed it from freeipa-server.
Now freeipa-python package should cover dbus-python deps for the whole
freeipa-* suite.
Martin^3
From ab0aa030d4589185841a19640bf8d3ff09769d1c Mon Sep 17 00:00:00 2001
From: Martin Babinsky mbabi...@redhat.com
Date: Tue, 27 Jan 2015 18:11:51 +0100
Subject: [PATCH] Moved dbus-python dependence to freeipa-python package
Removed dbus-python dependency from freeipa-server and added it to
freeipa-python. This should fix https://fedorahosted.org/freeipa/ticket/4863
and also cover dbus-python dependencies in other freeipa-* packages.
---
freeipa.spec.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 4da0732f51e7fa2931daa7c72969c3ab4fa41274..4198a94ef30a148708b23461c8ec14bd19afab38 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -126,7 +126,6 @@ Requires: acl
Requires: python-pyasn1
Requires: memcached
Requires: python-memcached
-Requires: dbus-python
Requires: systemd-units = 38
Requires(pre): systemd-units
Requires(post): systemd-units
@@ -287,6 +286,7 @@ Requires: python-pyasn1
Requires: python-dateutil
Requires: python-yubico
Requires: wget
+Requires: dbus-python
Conflicts: %{alt_name}-python
Obsoletes: %{alt_name}-python %{version}
--
2.1.0
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master
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 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
to 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 ==
