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

[Maxim Dounin]

Hello!

On Mon, Jun 19, 2017 at 04:09:43PM +0200, Bart Warmerdam wrote:

> 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) {

This condition cannot be true, as nginx uses preallocated buffer 
for i2d_SSL_SESSION().

(Moreover, using a preallocated buffer is this is the only 
approach documented in the i2d_SSL_SESSION() manual page, and the 
only one actually available before OpenSSL 1.1.0.)

[...]

-- 
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [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(,
# 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(, , it, -1, flags);
*out = buf;
return len;
}

return ASN1_item_ex_i2d(, 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 
# 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@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel


___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [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 
> # 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@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
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel