[PATCH] Avoid using the result of i2d_SSL_SESSION when the session is invalid

[Bart Warmerdam]

According to the man-page of i2d_SSL_SESSION the result can be NULL or 
0, but case the actual result can also be -1 in case of a failed 
CRYPTO_malloc. The call trace for this function is:

Call chain:
     i2d_SSL_SESSION
     i2d_SSL_SESSION_ASN1
     ASN1_item_i2d
     asn1_item_flags_i2d


The preprocessor output generates the following code:

static int asn1_item_flags_i2d(ASN1_VALUE *val, unsigned char **out,
                                const ASN1_ITEM *it, int flags)
{
     if (out && !*out) {
         unsigned char *p, *buf;
         int len;
         len = ASN1_item_ex_i2d(&val,
# 60 "crypto/asn1/tasn_enc.c" 3 4
                                     ((void *)0)
# 60 "crypto/asn1/tasn_enc.c"
                                         , it, -1, flags);
         if (len <= 0)
             return len;
         buf = CRYPTO_malloc(len, "crypto/asn1/tasn_enc.c", 63);
         if (buf ==
# 64 "crypto/asn1/tasn_enc.c" 3 4
                   ((void *)0)
# 64 "crypto/asn1/tasn_enc.c"
                       )
             return -1;
         p = buf;
         ASN1_item_ex_i2d(&val, &p, it, -1, flags);
         *out = buf;
         return len;
     }

     return ASN1_item_ex_i2d(&val, out, it, -1, flags);
}


As you can see around line 65 the -1 is returned once the allocation 
fails, which is then directly returned in all intermediate calls up to 
i2d_SSL_SESSION. So the -1 case is missed once the current "if 
statement" is kept without the "len < 1" or-statement.

Regards,

B.



On 2017-06-19 12:10, Ruslan Ermilov wrote:
> On Mon, Jun 19, 2017 at 08:08:38AM +0200, Bart Warmerdam wrote:
>> # HG changeset patch
>> # User Bart Warmerdam <bartw at xs4all.nl>
>> # Date 1497852211 -7200
>> #      Mon Jun 19 08:03:31 2017 +0200
>> # Branch i2d_ssl_session_length
>> # Node ID 079afb2cb4be3ef06d07e96d1a54cc359b971631
>> # Parent  d1816a2696de8c2faa1cd913a151e5f62a8620f3
>> Make sure to also take into account the 'return 0' response of
>> i2d_SSL_SESSION, which is possible when the session is not valid
> 
> A case of invalid session is already caught by checking the return
> value of SSL_get0_session() just prior to calling i2d_SSL_SESSION()
> in ngx_http_upstream_save_round_robin_peer_session() and
> ngx_stream_upstream_save_round_robin_peer_session().
> 
> The ngx_ssl_new_session() function is passed as an argument to
> SSL_CTX_sess_set_new_cb() that "sets the callback function, which
> is automatically called whenever a new session was negotiated."
> 
> Do you think that it's possible for a session to be invalid in
> either of these cases?
> 
>> diff -r d1816a2696de -r 079afb2cb4be src/event/ngx_event_openssl.c
>> --- a/src/event/ngx_event_openssl.c     Fri Jun 16 18:15:58 2017 +0300
>> +++ b/src/event/ngx_event_openssl.c     Mon Jun 19 08:03:31 2017 +0200
>> @@ -2458,9 +2458,9 @@
>> 
>>       len = i2d_SSL_SESSION(sess, NULL);
>> 
>> -    /* do not cache too big session */
>> -
>> -    if (len > (int) NGX_SSL_MAX_SESSION_SIZE) {
>> +    /* do not cache too big or invalid session */
>> +
>> +    if (len > (int) NGX_SSL_MAX_SESSION_SIZE || len < 1) {
>>           return 0;
>>       }
>> 
>> diff -r d1816a2696de -r 079afb2cb4be
>> src/http/ngx_http_upstream_round_robin.c
>> --- a/src/http/ngx_http_upstream_round_robin.c  Fri Jun 16 18:15:58 
>> 2017
>> +0300
>> +++ b/src/http/ngx_http_upstream_round_robin.c  Mon Jun 19 08:03:31 
>> 2017
>> +0200
>> @@ -755,9 +755,9 @@
>> 
>>           len = i2d_SSL_SESSION(ssl_session, NULL);
>> 
>> -        /* do not cache too big session */
>> +        /* do not cache too big or invalid session */
>> 
>> -        if (len > NGX_SSL_MAX_SESSION_SIZE) {
>> +         if (len > NGX_SSL_MAX_SESSION_SIZE || len < 1) {
>>               return;
>>           }
>> 
>> diff -r d1816a2696de -r 079afb2cb4be
>> src/stream/ngx_stream_upstream_round_robin.c
>> --- a/src/stream/ngx_stream_upstream_round_robin.c      Fri Jun 16
>> 18:15:58 2017 +0300
>> +++ b/src/stream/ngx_stream_upstream_round_robin.c      Mon Jun 19
>> 08:03:31 2017 +0200
>> @@ -787,9 +787,9 @@
>> 
>>           len = i2d_SSL_SESSION(ssl_session, NULL);
>> 
>> -        /* do not cache too big session */
>> +        /* do not cache too big or invalid session */
>> 
>> -        if (len > NGX_SSL_MAX_SESSION_SIZE) {
>> +        if (len > NGX_SSL_MAX_SESSION_SIZE || len < 1) {
>>               return;
>>           }
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel at nginx.org
>> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>>