Hello!

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

> > On 26 Aug 2022, at 07:01, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > # HG changeset patch
> > # User Maxim Dounin <mdounin@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/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org