Thread safety issue in krb5_verify_init_creds() when passing ccache pointer to null?

[Greg Hudson]

On 5/27/21 4:52 AM, Thomas Sondergaard wrote:
>     krb5_ccache cc = nullptr;
>     const auto err = krb5_verify_init_creds(context, &creds,
> server_princ, kt, &cc, &opts);
>     krb5_principal princ;
>     krb5_cc_get_principal(context, &princ);
>     ...
>     krb5_cc_close(context, cc);

Out of curiosity, what does the code do with cc?  This is the first time
I've heard of a caller using the credentials acquired by
krb5_verify_init_creds().

> The program occasionally crashes in the call to krb5_cc_get_principal.

With what version of the krb5 libraries?  The issue you identified below
is a concern, but a crash indicates a problem in the ccache layer.
Ticket #8202 (fixed in 1.17) addresses a bug that could explain the crash.

> If I understand this correctly I get a ticket cache "MEMORY:rd_req2", which
> is local to the process. So when this function is called from multiple
> threads with ccache being a pointer with the value NULL the threads will
> stomp on the same ticket cache. Is this correct?

It sounds like it.  You could of course work around this issue by either
passing NULL if you don't need the credentials, or creating a ccache in
the caller.

> Shouldn't the ticket cache "MEMORY:rd_req2" in get_vfy_cred() be changed to
> use krb5_cc_new_unique() too?
[...]
> Will calling
> just krb5_cc_close(context, cc) on a ccache created with
> krb5_cc_new_unique() cause it to be leaked or will it automatically be
> destroyed when all references to it have been closed?

It will be leaked.  (Well, the "leaked" objects would still be
theoretically accessible via the process-global memory ccache table, but
with names that nobody remembers.)

This concern may explain why the ticket #3089 fix didn't change this
code.  Changing the code to call krb5_cc_new_unique() could, in theory,
create a memory ccache leak in a currently-working caller (one which
uses krb5_verify_init_creds() serially, of course, not in parallel like
your code).

If we choose to let the current code stand for that reason, we should of
course document the behavior and warn callers not to use this aspect of
the function concurrently.

Another option would be to create the notion of an anonymous memory
cache, which has no resolvable name and does get cleaned up when closed.
 Heimdal recently (in Jan 2020) added this notion, making it happen when
"MEMORY:anonymous" is resolved--which is not strictly
backward-compatible, but presumably no one cares.  (A minor
complication: unlike Heimdal, we have a krb5_cc_dup() function, the
implementation of which would currently break for an unresolvable ccache.)