From 2e333375708f01bed5db51a3d89613e2fdd30daf Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Tue, 24 Apr 2018 13:22:10 +0200
Subject: [PATCH] Fix invalid data being accepted in RSA-decryption-based
 ciphersuites

In the refactoring of ssl_parse_encrypted_pms, I advertently broke the
case when decryption signalled an error, with the variable ret getting
overwritten before calculating diff. Move the calculation of diff
immediately after getting the return code to make the connection more
obvious. Also move the calculation of mask immediately after the
calculation of diff, which doesn't change the behavior, because I find
the code clearer that way.
---
 library/ssl_srv.c | 48 +++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/library/ssl_srv.c b/library/ssl_srv.c
index de8056426..e1dc5a8e9 100644
--- a/library/ssl_srv.c
+++ b/library/ssl_srv.c
@@ -3508,6 +3508,30 @@ static int ssl_parse_encrypted_pms( mbedtls_ssl_context *ssl,
         return( ret );
 #endif /* MBEDTLS_SSL_ASYNC_PRIVATE */
 
+    mbedtls_ssl_write_version( ssl->handshake->max_major_ver,
+                               ssl->handshake->max_minor_ver,
+                               ssl->conf->transport, ver );
+
+    /* Avoid data-dependent branches while checking for invalid
+     * padding, to protect against timing-based Bleichenbacher-type
+     * attacks. */
+    diff  = (unsigned int) ret;
+    diff |= peer_pmslen ^ 48;
+    diff |= peer_pms[0] ^ ver[0];
+    diff |= peer_pms[1] ^ ver[1];
+
+    /* mask = diff ? 0xff : 0x00 using bit operations to avoid branches */
+    /* MSVC has a warning about unary minus on unsigned, but this is
+     * well-defined and precisely what we want to do here */
+#if defined(_MSC_VER)
+#pragma warning( push )
+#pragma warning( disable : 4146 )
+#endif
+    mask = - ( ( diff | - diff ) >> ( sizeof( unsigned int ) * 8 - 1 ) );
+#if defined(_MSC_VER)
+#pragma warning( pop )
+#endif
+
     /*
      * Protection against Bleichenbacher's attack: invalid PKCS#1 v1.5 padding
      * must not cause the connection to end immediately; instead, send a
@@ -3524,18 +3548,6 @@ static int ssl_parse_encrypted_pms( mbedtls_ssl_context *ssl,
         return( ret );
     }
 
-    mbedtls_ssl_write_version( ssl->handshake->max_major_ver,
-                               ssl->handshake->max_minor_ver,
-                               ssl->conf->transport, ver );
-
-    /* Avoid data-dependent branches while checking for invalid
-     * padding, to protect against timing-based Bleichenbacher-type
-     * attacks. */
-    diff  = (unsigned int) ret;
-    diff |= peer_pmslen ^ 48;
-    diff |= peer_pms[0] ^ ver[0];
-    diff |= peer_pms[1] ^ ver[1];
-
 #if defined(MBEDTLS_SSL_DEBUG_ALL)
     if( diff != 0 )
         MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client key exchange message" ) );
@@ -3549,18 +3561,6 @@ static int ssl_parse_encrypted_pms( mbedtls_ssl_context *ssl,
     }
     ssl->handshake->pmslen = 48;
 
-    /* mask = diff ? 0xff : 0x00 using bit operations to avoid branches */
-    /* MSVC has a warning about unary minus on unsigned, but this is
-     * well-defined and precisely what we want to do here */
-#if defined(_MSC_VER)
-#pragma warning( push )
-#pragma warning( disable : 4146 )
-#endif
-    mask = - ( ( diff | - diff ) >> ( sizeof( unsigned int ) * 8 - 1 ) );
-#if defined(_MSC_VER)
-#pragma warning( pop )
-#endif
-
     /* Set pms to either the true or the fake PMS, without
      * data-dependent branches. */
     for( i = 0; i < ssl->handshake->pmslen; i++ )