[Freeipa-devel] [PATCHES] 366-372 Additional Coverity fixes

[Alexander Bokovoy]

On Mon, 24 Nov 2014, Jan Cholasta wrote:
>>>>>>>https://fedorahosted.org/freeipa/ticket/4713
>>>>>>>---
>>>>>>>daemons/ipa-kdb/ipa_kdb_mspac.c | 3 +++
>>>>>>>1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>>diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c
>>>>>>>b/daemons/ipa-kdb/ipa_kdb_mspac.c
>>>>>>>index c8f6c76..debcd1b 100644
>>>>>>>--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
>>>>>>>+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
>>>>>>>@@ -2071,6 +2071,9 @@ krb5_error_code
>>>>>>>ipadb_sign_authdata(krb5_context
>>>>>>>context,
>>>>>>>                            ipactx->kdc_hostname,
>>>>>>>strlen(ipactx->kdc_hostname),
>>>>>>>                            NULL, NULL, &result) == 0) {
>>>>>>>                kerr = ipadb_reinit_mspac(ipactx, true);
>>>>>>>+                if (kerr != 0 && kerr != ENOENT) {
>>>>>>>+                    goto done;
>>>>>>>+                }
>>>>>>>            }
>>>>>>>        }
>>>>>>>
>>>>>>I'm not sure we should drop the sign_authdata request here. If we were
>>>>>>able to re-initialize our view of trusted domains, we simply cannot
>>>>>>re-sign incoming PAC but this is handled in ipadb_verify_pac() and
>>>>>>ipadb_sign_pac() and if the former returns NULL value for PAC, we exit
>>>>>>with a return code of 0 while this change will fail a cross-realm TGT
>>>>>>request unconditionally.
>>>>>>
>>>>>
>>>>>OK, what would be a proper fix? Just ignore the return value of
>>>>>ipadb_reinit_mspac here?
>>>>
>>>>Guys, I did not see the code but all instances of "ignore return
>>>>code" I have
>>>>seen were wrong, including cases where code comment explicitly said
>>>>"we ignore
>>>>return code on purpose" :-)
>>>>
>>>>At least log an error message if you can't think of anything better ...
>>>>
>>>
>>>I don't disagree, if that's the proper fix.
>>>
>>>Alexander, or someone else, could you please finish the review of the
>>>patches? Thanks.
>>I've ACKed other patches but I'm not going to accept this patch. Rest is
>>fine.
>
>That's fine with me, but I'm still going to need a little hint on how 
>this should in fact be fixed.
Right. My main issue is that the change in this patch actually changes
the flow for signing MS-PAC in case we were unable to retrieve our
defaults. We need to ignore the return code of ipadb_reinit_mspac() here
because we are simply not going to add MS PAC with our signature, not
fully denying getting the ticket (that check is in a separate place)
because this affects also our realm's principals. Consider a case when
ipa-adtrust-install wasn't even run, this is where we'll get non-working
MS-PAC defaults and we silently drop the MS-PAC signing.
-- 
/ Alexander Bokovoy