[PATCH 04 of 11] SSL: explicit session id length checking

[Maxim Dounin]

Hello!

On Thu, Sep 15, 2022 at 09:40:32AM +0400, Sergey Kandaurov wrote:

> > On 26 Aug 2022, at 07:01, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > 
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1661481949 -10800
> > #      Fri Aug 26 05:45:49 2022 +0300
> > # Node ID f4ae0f4ee928cf20346530e96f1431314ecd0171
> > # Parent  86d827338fdd13ea899d618b0bcb2be23469cbac
> > SSL: explicit session id length checking.
> > 
> > Session ids are not expected to be longer than 32 bytes, but this is
> > theoretically possible with TLSv1.3, where session ids are essentially
> > arbitrary and sent as session tickets.  Since on 64-bit platforms we
> > use fixed 32-byte buffer for session ids, added an explicit length check
> > to make sure the buffer is large enough.
> > 
> 
> I don't follow how session ids could be "essentially arbitrary"
> (except a library bug that justifies such safety belt).
> For TLSv1.3, this callback is used to update session cache as part
> of constructing NewSessionTicket.  It's called after generating
> dummy session ids, which, and regardless of protocol version, are
> capped to SSL3_SSL_SESSION_ID_LENGTH (32).

In TLSv1.2 and below, session id lengths are limited by the 
protocol.  For example, in TLSv1.2:

   opaque SessionID<0..32>;

In TLSv1.3 there is no such limitation, and any length can be used 
by the library (well, almost any, up to 2^16-1 bytes).  While 
OpenSSL currently uses 32 bytes, there is no guarantee that at 
some point it won't switch to using, for example, 64 bytes.

> > 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
> > @@ -3842,6 +3842,14 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_
> >     p = buf;
> >     i2d_SSL_SESSION(sess, &p);
> > 
> > +    session_id = (u_char *) SSL_SESSION_get_id(sess, &session_id_length);
> > +
> > +    /* do not cache sessions with too long session id */
> > +
> > +    if (session_id_length > 32) {
> > +        return 0;
> > +    }
> > +
> 
> The check can be moved above the cpu expensive i2d_SSL_SESSION call.
> Or rather move i2d_SSL_SESSION closer to corresponding ngx_memcpy().
> (but see my reply on the next patch)

There is no practical difference, as this check is not expected 
to catch anything.

> >     c = ngx_ssl_get_connection(ssl_conn);
> > 
> >     ssl_ctx = c->ssl->session_ctx;
> > @@ -3886,8 +3894,6 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_
> >         }
> >     }
> > 
> > -    session_id = (u_char *) SSL_SESSION_get_id(sess, &session_id_length);
> > -
> > #if (NGX_PTR_SIZE == 8)
> > 
> >     id = sess_id->sess_id;
> > 

-- 
Maxim Dounin
http://mdounin.ru/