[gnutls-devel] session ticket key rotation

[Daniel Kahn Gillmor]

Hi Nikos--

On Mon 2016-11-14 18:38:38 +0900, Nikos Mavrogiannopoulos wrote:
> and a smooth rotation is not possible as I see with the current API.

ok, at least i wasn't missing anythingn obvious :/

> Certainly. My goal for the new releases is to make it as simple as
> possible for applications to use gnutls. I no longer try to push to
> applications all the hard parts :)

This is the right choice, thanks :)

>> I'm imagining that an opaque server-side API for this would look
>> something like:
>>     struct gnutls_ticket_key_pool_int;
>>     typedef struct gnutls_ticket_key_pool_int *gnutls_ticket_key_pool_t;
>>
>>     int gnutls_ticket_key_pool_init(gnutls_ticket_key_pool_t *pool, int numkeys);
>>     int gnutls_ticket_key_pool_free(gnutls_ticket_key_pool_t pool);
>>     int gnutls_ticket_key_pool_rotate(gnutls_ticket_key_pool_t pool);
>
> The tricky part would be making the rotate function semantics easy to
> use, even in multi-threaded environments, and allow efficient access
> to keys at the same time. You wouldn't want a multi-threaded server to
> hold a lock in order to access a ticket key.

Is the problem the expense of taking a read lock itself, or is the
problem a risk of uninitialized memory access?

> That's not an easy goal to achieve; something similar using atomic
> variables of C11 (with fallback) was done in gnutls_rnd() (see
> random.c and  _gnutls_rnd_init).

 Is it possible that it doesn't matter?  Consider the following (very
 rough) implementation (error checking omitted for concision):

     1	   typedef char[SESTKTKEYLEN] _session_ticket_key;
     2	   
     3	   struct gnutls_ticket_key_pool_int {
     4	          int numkeys;
     5	          _session_ticket_key *keys;
     6	          int current_key;
     7	   }
     8	
     9	   int gnutls_ticket_key_pool_init(gnutls_ticket_key_pool_t *pool, int numkeys) {
    10	       *pool = malloc(sizeof(gnutls_ticket_key_pool_int));
    11	       (*pool)->numkeys = numkeys;
    12	       (*pool)->keys = malloc(sizeof(_session_ticket_key) * numkeys);
    13	       (*pool)->current_key = 0;
    14	       gnutls_rnd(GNUTLS_RND_KEY, (*pool)->keys, sizeof(_session_ticket_key) * numkeys);
    15	       
    16	   }
    17	   int gnutls_ticket_key_pool_rotate(gnutls_ticket_key_pool_t pool) {
    18	      _session_ticket_key newkey;
    19	      gnutls_rnd(GNUTLS_RND_KEY, newkey, sizeof(newkey));
    20	      int newkeyidx = (pool->current_key + 1)%pool->numkeys;
    21	      memcpy(pool->keys[newkeyidx], newkey, sizeof(newkey));
    22	      pool->current_key = newkeyidx;
    23	   }


if someone offers a session ticket that belongs to the oldest keyslot
(the one that's about to be overwritten), and the crypto code that
accesses it to decrypt the ticket is in use during the memcpy on line
21, what's the worst that could happen?  It seems to me like the
decryption would fail, and the session wouldn't be able to resume.  This
is the *same* outcome as if it were to happen *after* the the function
completes (because that specific key has been overwritten by data that
can't dcrypt any subsequent message.  The outcome is, the client has to
go through a full handshake.  doesn't seem too bad...

> The change in semantics by that would be that the credentials
> structure would no longer hold only read-only data. However, if the
> rotation is transparent for all types of applications, I have no
> objection on that.

What different kind of application types should i be concerned about
that might behave differently?

> Serialization and de/serialization would simplify things a lot for
> applications. It would require some versioning though to allow
> upgrading to a new ticket key handling at the future.

a bytewise copy of the pool->keys member seems like it ought to do the
trick, and the reasoning about write-collisions looks to me like a
full-buffer memcpy is equivalent to the discussion about internal key
rotation, no?

wdyt?

        --dkg
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 962 bytes
Desc: not available
URL: </pipermail/attachments/20161114/d878887f/attachment.sig>