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

[Ruslan Ermilov]

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
> 

-- 
Ruslan Ermilov

Join us at nginx.conf, Sep. 6-8, Portland, OR
https://www.nginx.com/nginxconf