> On 17 Sep 2022, at 01:03, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> 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.

So that's for a rather theoretical and unlikely reason.
I agree though, such explicit and cheap guarantee won't hurt.

>
>>> 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.
>

Agree, with that in mind it won't make a difference.

>>> 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;
>>>
>

--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org