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

[Thomas Sondergaard]

On Thu, May 27, 2021 at 5:23 PM Greg Hudson <ghudson at mit.edu> wrote:

> 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().
>

It is a little curious. I didn't write the code, so I am not completely
sure why it is the way it is. The signature of function that contains this
code is:

ResultType verifyCredentials(std::string princ_name, std::string password,
std::string keytab_path, std::string server_principal)

It calls krb5_parse_name to create a principal from princ_name and then
krb5_get_init_creds_passwords to create a krb5_creds. It then calls
krb5_verify_init_creds(). If that checks out it then calls
krb5_cc_get_principal to dig the principal out of the returned krb5_ccache
and then calls krb5_unparse_name and returns that to the caller. Is there
any situation where this returned principal will be different from the one
the caller of the function provided?


>
> > 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.
>

It is on CentOS 8.3.2011 with krb5-libs-1.18.2-5.el8.x86_64


>
> > 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.
>

I'm going to to either create my own ccache - or just drop reading back the
default principal depending on the feedback I get to the question I asked
above.


> > 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.)
>

krb5_mcc_close(krb5_context context, krb5_ccache id) in
krb5/src/lib/krb5/ccache/cc_memory.c is the function that is called when
calling krb5_cc_close on a "MEMORY" type ccache, isn't it?
In krb5-1.82.2-final it looks like this:

krb5_error_code KRB5_CALLCONV
krb5_mcc_close(krb5_context context, krb5_ccache id)
{
    krb5_mcc_data *d = id->data;
    int count;
    free(id);
    k5_cc_mutex_lock(context, &d->lock);
    count = --d->refcount;
    k5_cc_mutex_unlock(context, &d->lock);
    if (count == 0) {
        /* This is the last active handle referencing d and d has been
removed
         * from the table, so we can release it. */
        empty_mcc_cache(context, d);
        free(d->name);
        k5_cc_mutex_destroy(&d->lock);
        free(d);
    }
    return KRB5_OK;
}

When I call krb5_cc_close() on the only and therefore last handle to my
ccache created with krb5_cc_new_unique() I expect the refcount to reach 0
therefore calling empty_mcc_cache() to free the cache? So I was a little
unsure what is leaked. I also looked at krb5_mcc_destroy() in the same file
and it mentions removing the ccache from a table. Is it this empty table
entry that is leaked?

So the conclusion is that I should call krb5_cc_destroy() on the ccache
created with krb5_cc_new_unique() to get rid of it, right?

Thank you for your invaluable help!

Thomas