[Freeipa-devel] [PATCHES 0005-0011] Fix some of the defects reported by covscan on freeipa-master

[Alexander Bokovoy]

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