[PATCH 11 of 11] SSL: automatic rotation of session ticket keys
Maxim Dounin
mdounin at mdounin.ru
Wed Oct 12 18:05:53 UTC 2022
Hello!
On Wed, Oct 12, 2022 at 05:57:07PM +0400, Sergey Kandaurov wrote:
>
> > On 9 Oct 2022, at 08:59, Maxim Dounin <mdounin at mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Sat, Oct 01, 2022 at 11:58:20AM +0300, Maxim Dounin wrote:
> >
> >> On Thu, Sep 29, 2022 at 08:00:03PM +0400, Sergey Kandaurov wrote:
> >>
> >>>> On 28 Sep 2022, at 22:37, Maxim Dounin <mdounin at mdounin.ru> wrote:
> >>>>
> >>>> On Mon, Sep 26, 2022 at 02:17:18PM +0400, Sergey Kandaurov wrote:
> >>
> >> [...]
> >>
> >>>>> And by the way, while reviewing this patch, I noticed that
> >>>>> OpenSSL doesn't allow a client to gracefully renew TLSv1.2 session
> >>>>> when the client receives a new session ticket in resumed sessions.
> >>>>> In practice, it is visible when client resumes a not yet expired
> >>>>> session encrypted with not a fresh ticket key (after rotation),
> >>>>> which results in sending a new session ticket.
> >>>>> See ssl_update_cache() for the !s->hit condition.
> >>>>> In the opposite, BoringSSL always allows to renew TLSv1.2 sessions.
> >>>>
> >>>> You mean on the client side? Yes, it looks like
> >>>> ngx_ssl_new_client_session() won't be called for such a new
> >>>> session ticket, and updated ticket will be never saved. This
> >>>> might need to be worked around.
> >>>
> >>> Yes, I mean the client side.
> >>>
> >>>>
> >>>> This should be safe with the key rotation logic introduced in this
> >>>> patch though, given that the previous key is preserved till the
> >>>> last ticket encrypted with it is expected to expire.
> >>>>
> >>>> One of the possible solutions might be to avoid re-encryption of
> >>>> tickets with the new key, as the old key is anyway expected to be
> >>>> available till the session expires.
> >>>
> >>> I don't think it's worth the effort. If I got you right, and
> >>> as far as I understand, re-encrypting the ticket essentially
> >>> means sending a fresh session (renewal).
> >>
> >> Well, not really. Re-encryption of a ticket does not imply
> >> session renewal. Further, doing so implies security risk: if we
> >> renew a session during re-encryption, this makes it possible to
> >> create essentially infinite sessions. And, for example, if a
> >> session used a client certificate, this effectively means that
> >> this certificate will never expire and cannot be revoked.
> >>
> >> With TLSv1.2, OpenSSL follows this logic: session expiration time
> >> is set when a session is created, and ticket re-encryption only
> >> re-encrypts the session, but doesn't change session expiration.
> >> As such, any certificate validation which happens during session
> >> creation needs to be redone once session timeout expires - and
> >> this makes it possible to implement certificate revocation.
> >>
> >> On the other hand, as implemented for TLSv1.3 at least in OpenSSL
> >> it seems not to be the case. Every ticket sent to the client
> >> actually creates a new session with _updated_ expiration time. As
> >> such, it is possible to create a session authenticated with a client
> >> certificate, and use this session indefinitely, even after the
> >> certificate will expire and/or will be revoked.
> >>
> >> This seems to be a security issue in OpenSSL.
> >>
> >> BoringSSL seems to behave similarly with TLSv1.3, that is, it
> >> updates session expiration time, making it possible to use an
> >> authenticated session for much longer than session timeout
> >> configured. But BoringSSL also has session's auth_timeout, which
> >> prevents indefinite use of the session. The auth_timeout value is
> >> hardcoded to 7 days (SSL_DEFAULT_SESSION_AUTH_TIMEOUT), and does
> >> not seem to be adjustable (only with SSL_SESSION_set_timeout(),
> >> which is documented to be a function for writing tests).
> >>
> >> I would rather say it is also a security issue in BoringSSL,
> >> though it's slightly better than in OpenSSL due to the 7 days
> >> limit.
> >
> > For the record:
> >
> > https://github.com/openssl/openssl/issues/19341
> >
> > Note that with the automatic ticket key rotation this issue with
> > TLSv1.3 sessions becomes slightly worse in a typical configuration
> > (with ssl_session_cache in shared memory, but without
> > ssl_session_ticket_key explicitly set and/or ssl_session_tickets
> > switched off). Notably, configuration reload is no longer enough
> > to invalidate all tickets, since ticket keys are now preserved in
> > shared memory.
> >
> > For example, consider that a CRL file is updated with new
> > revocations, and nginx configuration is reloaded. New revocations
> > will be loaded by nginx and will appear to work with new sessions,
> > but can be easily bypassed by maintaining a previously established
> > TLSv1.3 session. Previously, it was possible to bypass
> > revocations in such scenario only if ticket keys were explicitly
> > set or if session tickets were switched off and sessions were
> > cached in shared memory.
> >
> > Given that we do not enable TLSv1.3 by default, we probably can
> > ignore this and wait for appropriate fixes from the affected
> > libraries. On the other hand, it might be a good idea to
> > introduce a workaround, especially if we want to enable TLSv1.3 by
> > default in the foreseeable future.
> >
> > The following patch seems to be simple enough and forces session
> > timeouts for TLSv1.3 sessions to be as configured, for both
> > OpenSSL and BoringSSL:
>
> Although somewhat tricky, I like the approach.
> Nitpicking comments inline.
>
> >
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1665286021 -10800
> > # Sun Oct 09 06:27:01 2022 +0300
> > # Node ID c0ec4df7ccbb95b7f2c2842f40012082991bed52
> > # Parent 37a4ac7ba1c5a003ab85f73d77767058af4eae30
> > SSL: workaround for session timeout handling with TLSv1.3.
> >
> > OpenSSL with TLSv1.3 updates the session creation time on session
> > resumption and keeps the session timeout unmodified, making it possible
> > to maintain the session forever, bypassing client certificate expiration
> > and revocation. To make sure session timeouts are actually used, we
> > now update the session creation time and reduce the session timeout
> > accordingly.
> >
> > BoringSSL with TLSv1.3 ignores configured session timeouts and uses a
> > hardcoded timeout instead, 7 days. So we update session timeout to
> > the configured value as soon as a session is created.
> >
> > diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
> > --- a/src/event/ngx_event_openssl.c
> > +++ b/src/event/ngx_event_openssl.c
> > @@ -1086,6 +1086,53 @@ ngx_ssl_info_callback(const ngx_ssl_conn
> >
> > #endif
> >
> > +#ifdef TLS1_3_VERSION
> > +
> > + if ((where & SSL_CB_ACCEPT_LOOP) == SSL_CB_ACCEPT_LOOP
> > + && SSL_version(ssl_conn) == TLS1_3_VERSION)
> > + {
> > + time_t now, time, timeout, conf_timeout;
> > + SSL_SESSION *sess;
>
> Not sure if this should use ngx_ssl_session_t. But, given the macro
> is intended for external consumption, SSL_SESSION should be fine.
Yes, as far as I can see we are using SSL_SESSION and not
ngx_ssl_session_t for tasks internal to ngx_event_openssl.c, so
I've used it here.
> > +
> > + /*
> > + * OpenSSL with TLSv1.3 updates the session creation time on
> > + * session resumption and keeps the session timeout unmodified,
> > + * making it possible to maintain the session forever, bypassing
> > + * client certificate expiration and revocation. To make sure
> > + * session timeouts are actually used, we now update the session
> > + * creation time and reduce the session timeout accordingly.
> > + *
> > + * BoringSSL with TLSv1.3 ignores configured session timeouts
> > + * and uses a hardcoded timeout instead, 7 days. So we update
> > + * session timeout to the configured value as soon as a session
> > + * is created.
> > + */
> > +
> > + c = ngx_ssl_get_connection((ngx_ssl_conn_t *) ssl_conn);
> > + sess = SSL_get0_session(ssl_conn);
> > +
> > + if (!c->ssl->session_timeout_set && sess) {
> > + c->ssl->session_timeout_set = 1;
> > +
> > + now = ngx_time();
> > + time = SSL_SESSION_get_time(sess);
> > + timeout = SSL_SESSION_get_timeout(sess);
> > + conf_timeout = SSL_CTX_get_timeout(c->ssl->session_ctx);
> > +
> > + timeout = ngx_min(timeout, conf_timeout);
> > +
> > + if (now - time >= timeout) {
> > + SSL_SESSION_set1_id_context(sess, (unsigned char *) "", 0);
>
> Why not u_char? If this is to strictly follow the declaration, without
> typedef's, then I wonder why const is omitted in casting 2nd parameter.
> Anyway, this passes compilation on known CI platforms.
> For the record, BoringSSL moved long ago to uint8_t in its headers,
> so is incompatible with CHAR_BIT > 8 (though, POSIX mandates 8).
Because u_char is an nginx type, while
SSL_SESSION_set1_id_context() accepts "unsigned char" arguments.
While u_char is equivalent, I preferred the type as accepted by
the function.
The "const" qualifier is not important here and can (and should) be
omitted, as it only specifies that the function argument is not to
be modified within the function.
> I've been pondering if it's worth to call SSL_SESSION_set1_id_context()
> in order to (obfuscatedly) cancel the going to expire sessions.
> At least, OpenSSL handles this on theirself for sessions that've already
> expired ("now > time + timeout"), rejecting such session, so the only
> viable condition seems to be is when the session is going to be expired
> ("now == time + timeout").
The problem the SSL_SESSION_set1_id_context() solves here is that
it is not possible to ensure that the session will expire.
Notably:
- if you set timeout to 0, this effectively means 3 seconds, due
to the default timeout used in d2i_SSL_SESSION();
- if you set session timeout to any negative value, it is set by
OpenSSL itself to 0 in ssl_session_calculate_timeout();
- if you set session timeout to a positive value, this means that
this session can be reused 1 second later, and hence prolonged
for 1 more second (and therefore can be kept active indefinitely).
As such, SSL_SESSION_set1_id_context() is called to ensure that in
case of "now == time + timeout" the session modified so it won't
be reusable anymore. The "<" part is mostly cosmetic.
> For the record, invalidating session context (or even session removal)
> this way doesn't prevent from reusing the session in this connection,
> since the info callback is called too late, after the ticket has been
> successfully decrypted (with a check for timeout and session context)
> and session restored. Such session will be rejected only the next time.
That's exactly the plan.
> It may have sense though to call it still to handle the going to expire
> sessions that pass the session timeout check on server, see sess_timedout()
> in OpenSSL sources, it has slightly different condition to reject sessions.
> In this case the control goes to the "if (now - time >= timeout) {"
> condition, where we need to take the action, as otherwise OpenSSL
> will update the session creation time, such that the session will
> continue to be resumable for another "timeout" seconds.
> There should be no difference between invalidating context and setting
> intentionally old values for session time and timeout, both should
> work to make it stop from being resumable. So, condition could be
> collapsed to update session time and timeout values in both cases.
> But zero session timeout doesn't seem to pass i2d_SSL_SESSION checks
> in OpenSSL while constructing new session ticket, so invalidating
> session context looks like the only solution.
Yes, basically SSL_SESSION_set1_id_context() seems to be the only
way to make sure it won't be possible to resume the session.
> > +
> > + } else {
> > + SSL_SESSION_set_time(sess, now);
> > + SSL_SESSION_set_timeout(sess, timeout - (now - time));
> > + }
> > + }
> > + }
> > +
> > +#endif
> > +
> > if ((where & SSL_CB_ACCEPT_LOOP) == SSL_CB_ACCEPT_LOOP) {
> > c = ngx_ssl_get_connection((ngx_ssl_conn_t *) ssl_conn);
> >
> > diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h
> > --- a/src/event/ngx_event_openssl.h
> > +++ b/src/event/ngx_event_openssl.h
> > @@ -114,6 +114,7 @@ struct ngx_ssl_connection_s {
> > unsigned no_send_shutdown:1;
> > unsigned shutdown_without_free:1;
> > unsigned handshake_buffer_set:1;
> > + unsigned session_timeout_set:1;
> > unsigned try_early_data:1;
> > unsigned in_early:1;
> > unsigned in_ocsp:1;
> >
>
> Looks good.
The whole patch series pushed to http://mdounin.ru/hg/nginx.
--
Maxim Dounin
http://mdounin.ru/
More information about the nginx-devel
mailing list