FILE ccache corruption, possible fix

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

FILE ccache corruption, possible fix

Nico Williams

I won't recap the IRC discussion from earlier today.  Suffice it to say
that there is a race in krb5_fcc_close_file() between the unlock and the
close(2) of the ccache: the close(2) can drop locks acquired by other
threads between the dropping of locks and the closing of the fildes in
this thread (a writer).

I think the most elegant fix is as follows (note: not yet tested):

 - Don't lock the ccache to read.  If an entry appears to be truncated
   (i.e., EOF too soon), ignore it.  This may require returning a bogus
   ccache entry from krb5_cc_next_cred() in order to keep backwards
   compatibility.

   I.e., assume creds are only appended or otherwise, if overwritten,
   that they are overwritten atomically with equal-sized entries, and
   only to delete them.  (The part about overwrites is tricky, but don't
   focus on that for now; if need be assume that ccaches are
   append-only.)

 - Don't unlock the ccache when writing, just close(2) the file
   descriptor.  This requires knowing in krb5_fcc_close_file() whether
   we're using POSIX file locking.

This doesn't require waiting for "private locks" (a proposed variant of
POSIX file locking that addresses the POSIX file locking drop-locks-on-
first-close insanity) and is fairly simple.

Earlier I'd proposed borrowing ideas from SQLite3, but I think the above
is much simpler and safer.

Nico
--
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: FILE ccache corruption, possible fix

Nico Williams
Oh, of course, readers must delay closing the fildes, or perhaps acquire a
[whole file] write lock immediately before closing the fildes (to make sure
not to be stepping on any writer's toes.

Boy, it'd be nice if O_APPEND was reliable, but it's not.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: FILE ccache corruption, possible fix

Greg Hudson
On 01/22/2014 12:57 AM, Nico Williams wrote:
> Oh, of course, readers must delay closing the fildes, or perhaps acquire a
> [whole file] write lock immediately before closing the fildes (to make sure
> not to be stepping on any writer's toes.

I don't think acquiring a write lock just before closing would help; it
wouldn't conflict with a write lock held by another thread in the same
process.  Delaying closing the fd would work, but as far as I know, that
requires basically all of the bookkeeping that the SQLite code does.

Similarly, "don't unlock the ccache when writing, just close" wouldn't
help; all of the process's POSIX locks on the file are dropped when the
fd is closed.

> Boy, it'd be nice if O_APPEND was reliable, but it's not.

I assume you mean it's only reliable up to PIPE_BUF bytes?  It might be
good enough for you in practice.  I would be fine with making MIT's file
ccache code marshal creds to memory and write() them out all at once
with O_APPEND (like Heimdal does), although it would be too big of a
change to backport.  I think it would let us share the marshalling code
between cc_file.c and cc_keyring.c.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: FILE ccache corruption, possible fix

Nico Williams
In reply to this post by Nico Williams
On Wednesday, January 22, 2014, Nico Williams <[hidden email]> wrote:

> Oh, of course, readers must delay closing the fildes, or perhaps acquire a
> [whole file] write lock immediately before closing the fildes (to make sure
> not to be stepping on any writer's toes.


If we stop the whole open/close business and stat instead to decide when to
reopen the file ccache and then always acquire a write lock before closing
the thing, then that won't suck too much.  Also, caching the open ccache
fildes in the krb5_context would mean deferring the need to acquire a write
lock to ccache reopen times or context free times, so as to not force
serialization of all reads (though at the limit that's what acquiring a
write lock before closing means).
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev