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

[Jan Cholasta]

Dne 24.11.2014 v 15:12 Alexander Bokovoy napsal(a):
> 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.

OK, fixed, updated patch attached.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-369.2-Fix-unchecked-return-value-in-ipa-kdb.patch
Type: text/x-patch
Size: 994 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141124/f9960d07/attachment.bin>