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

[Sergey Kandaurov]

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

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