[krbdev.mit.edu #8938] Leash crashes on failure to auto-renew tickets

[Greg Hudson via RT]

Wed Aug 19 02:28:48 2020: Request 8938 was acted upon.
 Transaction: Ticket created by ghudson at mit.edu
       Queue: krb5
     Subject: Leash crashes on failure to auto-renew tickets
       Owner: Nobody
  Requestors: ghudson at mit.edu
      Status: new
 Ticket <URL: https://krbdev.mit.edu/rt/Ticket/Display.html?id=8938 >


If the auto-renew feature is turned on (as it is by default), Leash attempts
once to auto-renew the default ccache when it is 20 minutes from expiring, if
it has a renewable lifetime longer than 20 minutes. The trigger code is inside
CLeashView::PreTranslateMessage(), and it spawns a thread to run
CLeashView::RenewTicket().

CLeashView::RenewTicket() runs Leash_renew(). On success it updates some
variables, schedules a display update, and returns. On failure, it attempts to
read the principal name of the MSLSA ccache and compares it to
ticketinfo.Krb5.principal with strcmp(). If they compare equal, it sets
m_importedTickets and falls back to a ImportTicket operation via a new thread.
Otherwise it falls back to an InitTicket operation via a new thread.
In the current state of the code, ticketinfo.Krb5.principal is always NULL, so
the process crashes inside strcmp() due to the null dereference.

Although ticketinfo.Krb5 is a global, it is managed semi-ephemerally, bracketed
by calls to LeashKRB5ListDefaultTickets() and LeashKRB5FreeTicketInfo(). The
latter function frees and nulls out the principal, ccache_name, and ticket_list
fields, but leaves the other fields set, including btickets (which indicates
whether tickets are present, absent, or expired) and the renewal times. Commit
9bc411e72fce5bed3ed00ae5b09f8c239309bae0 removed the code in RenewTicket() to
ticketinfo.Krb5, introducing this bug.

Failure to renew tickets appears to be the norm rather than the exception when
the default ccache is set to MSLSA. I believe this is due to a Windows update
disabling the AllowTGTSessionKey registry feature. The auto-renewal failure
causes an unsightly popup window (out of scope for this ticket); the crash bug
then leads to a loop where each attempt to authenticate with GSSAPI causes
another renewal attempt, another popup, and another crash.

The Leash code relating to importing tickets is arguably vestigial, meaning
that the buggy code could perhaps be removed rather than fixed. The code
deciding when to import tickets is mostly unchanged from the old KfW 3.2.2
Leash code; however:

* Most of the code to automatically import tickets disables itself if
Leash_importable() returns false. This is the case for any UAC-limited process,
and whenever iterating over the MSLSA cache does not find a TGT with a
retrievable session key.

* The new Ribbon UI does not have a manual import button like the old UI. The
old UI can still be seen by running leash -noribbon, and still has an import
button. This button self-disables if the default ccache is an MSLSA ccache or
if Leash_importable() returns false.

If we decide to fix rather than remove the broken code, there are a couple of
options. We could add in a call to LeashKRB5ListDefaultTickets() (after
acquiring the lock) so that the principal field has a chance of being
populated. Or we could just compare the principal name of the default ccache to
that of the MSLSA cache. We should also add a check that the default ccache is
not the MSLSA cache.

There is a possibly related issue in CLeashView::PreTranslateMessage(). The
code to compose strTimeDate (conditional on CMainFrame::m_isMinimum) uses
ticketinfo.Krb5.principal, which I believe will always be null as there is no
call to LeashKRB5ListDefaultTickets(). This code is unchanged from before
commit 9bc411e72fce5bed3ed00ae5b09f8c239309bae0, but prior to that commit,
ticketinfo.Krb5.principal was not ephemeral (it was a fixed-sized buffer, and
there was no call to partially deconstruct ticketinfo.Krb5 after each use). At
this time I am not sure when that could could be reached.