Maxim Dounin
September 16, 2022 05:04PM
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
Subject Author Views Posted

[PATCH 00 of 11] SSL session handling patches

Maxim Dounin 430 August 25, 2022 11:14PM

[PATCH 06 of 11] SSL: explicit clearing of expired sessions

Maxim Dounin 61 August 25, 2022 11:14PM

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

Maxim Dounin 60 August 25, 2022 11:14PM

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

Sergey Kandaurov 57 September 15, 2022 01:42AM

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

Maxim Dounin 59 September 16, 2022 05:04PM

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

Sergey Kandaurov 67 September 26, 2022 06:14AM

[PATCH 08 of 11] SSL: renamed session ticket key type

Maxim Dounin 56 August 25, 2022 11:14PM

[PATCH 10 of 11] SSL: shorter debug messages about session tickets

Maxim Dounin 55 August 25, 2022 11:14PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 74
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready