Improved the client version number check.

If the client sends a wrong version number within the encrypted
Pre-Master-Secret, and a version number check is required, the decoded
Pre-Master-Secret is replaced with a random value in constant time and memory.

3 files changed, 51 insertions, 34 deletions

diff --git a/core/src/main/java/org/bouncycastle/crypto/tls/DefaultTlsEncryptionCredentials.java b/core/src/main/java/org/bouncycastle/crypto/tls/DefaultTlsEncryptionCredentials.java
index 7170455a..ea7bea76 100644
--- a/core/src/main/java/org/bouncycastle/crypto/tls/DefaultTlsEncryptionCredentials.java
+++ b/core/src/main/java/org/bouncycastle/crypto/tls/DefaultTlsEncryptionCredentials.java
@@ -55,11 +55,11 @@ public class DefaultTlsEncryptionCredentials
         return certificate;
     }
 
-    public byte[] decryptPreMasterSecret(byte[] encryptedPreMasterSecret)
+    public byte[] decryptPreMasterSecret(byte[] encryptedPreMasterSecret, byte[] fallback)
         throws IOException
     {
 
-        PKCS1Encoding encoding = new PKCS1Encoding(new RSABlindedEngine(), 48);
+        PKCS1Encoding encoding = new PKCS1Encoding(new RSABlindedEngine(), fallback);
         encoding.init(false, new ParametersWithRandom(this.privateKey, context.getSecureRandom()));
 
         try
@@ -69,6 +69,9 @@ public class DefaultTlsEncryptionCredentials
         }
         catch (InvalidCipherTextException e)
         {
+        	/*
+        	 * This should never happen, the decryption should always succeed, or return a random value.
+        	 */
             throw new TlsFatalAlert(AlertDescription.illegal_parameter);
         }
     }
diff --git a/core/src/main/java/org/bouncycastle/crypto/tls/TlsEncryptionCredentials.java b/core/src/main/java/org/bouncycastle/crypto/tls/TlsEncryptionCredentials.java
index eddf6847..292d4dbb 100644
--- a/core/src/main/java/org/bouncycastle/crypto/tls/TlsEncryptionCredentials.java
+++ b/core/src/main/java/org/bouncycastle/crypto/tls/TlsEncryptionCredentials.java
@@ -5,6 +5,6 @@ import java.io.IOException;
 public interface TlsEncryptionCredentials
     extends TlsCredentials
 {
-    byte[] decryptPreMasterSecret(byte[] encryptedPreMasterSecret)
+    byte[] decryptPreMasterSecret(byte[] encryptedPreMasterSecret, byte[] fallback)
         throws IOException;
 }
diff --git a/core/src/main/java/org/bouncycastle/crypto/tls/TlsRSAUtils.java b/core/src/main/java/org/bouncycastle/crypto/tls/TlsRSAUtils.java
index e3856bdc..0a464593 100644
--- a/core/src/main/java/org/bouncycastle/crypto/tls/TlsRSAUtils.java
+++ b/core/src/main/java/org/bouncycastle/crypto/tls/TlsRSAUtils.java
@@ -55,27 +55,35 @@ public class TlsRSAUtils
         /*
          * RFC 5246 7.4.7.1.
          */
-
         ProtocolVersion clientVersion = context.getClientVersion();
 
         // TODO Provide as configuration option?
         boolean versionNumberCheckDisabled = false;
 
         /*
-         * See notes regarding Bleichenbacher/Klima attack. The code here implements the first
-         * construction proposed there, which is RECOMMENDED.
+         * Bleichenbacher side channel countermeasures have been implemented in
+         * decryptPreMasterSecret, so we con't need to do something here.
          */
-        byte[] R = new byte[48];
-        context.getSecureRandom().nextBytes(R);
-
         byte[] M = TlsUtils.EMPTY_BYTES;
+        
+        
+		/*
+		 * Generate 48 random bytes we can use as a Pre-Master-Secret, if the
+		 * PKCS1 padding check should fail.
+		 */
+        byte[] fallback = new byte[48];
+    	context.getSecureRandom().nextBytes(fallback);
+
         try
         {
-            M = encryptionCredentials.decryptPreMasterSecret(encryptedPreMasterSecret);
+            M = encryptionCredentials.decryptPreMasterSecret(encryptedPreMasterSecret, fallback);
         }
         catch (Exception e)
         {
             /*
+             * This should never happen since the decryption should never throw an exception
+             * and return a random value instead.
+             * 
              * In any case, a TLS server MUST NOT generate an alert if processing an
              * RSA-encrypted premaster secret message fails, or the version number is not as
              * expected. Instead, it MUST continue the handshake with a randomly generated
@@ -83,34 +91,40 @@ public class TlsRSAUtils
              */
         }
 
-        if (M.length != 48)
-        {
-            TlsUtils.writeVersion(clientVersion, R, 0);
-            return R;
-        }
-
         /*
-         * If ClientHello.client_version is TLS 1.1 or higher, server implementations MUST
-         * check the version number [..].
-         */
+-         * If ClientHello.client_version is TLS 1.1 or higher, server implementations MUST
+-         * check the version number [..].
+          */
         if (versionNumberCheckDisabled && clientVersion.isEqualOrEarlierVersionOf(ProtocolVersion.TLSv10))
         {
-            /*
-             * If the version number is TLS 1.0 or earlier, server implementations SHOULD
-             * check the version number, but MAY have a configuration option to disable the
-             * check.
-             */
+			/*
+			 * If the version number is TLS 1.0 or earlier, server
+			 * implementations SHOULD check the version number, but MAY have a
+			 * configuration option to disable the check.
+			 * 
+			 * So there is nothing to do here.
+			 */
+        } else {
+			/*
+			 * OK, we need to compare the version number in the decrypted
+			 * Pre-Master-Secret with the clientVersion received during the
+			 * handshake. If they don't match, we replace the decrypted
+			 * Pre-Master-Secret with a random one.
+			 */
+        	int correct = (clientVersion.getMajorVersion() ^ (M[0]&0xff)) | (clientVersion.getMinorVersion() ^ (M[1]&0xff));
+        	correct |= correct>>1;
+        	correct |= correct>>2;
+        	correct |= correct>>4;
+        	int mask = ~((correct & 1) - 1);
+        	
+        	/*
+        	 * mask will be all bits set to 0xff if the version number differed.
+        	 */
+        	
+        	for (int i = 0; i < 48; i++) {
+        		M[i] = (byte)((M[i]&(~mask))|(fallback[i]&mask));
+        	}
         }
-        else
-        {
-            /*
-             * Note that explicitly constructing the pre_master_secret with the
-             * ClientHello.client_version produces an invalid master_secret if the client
-             * has sent the wrong version in the original pre_master_secret.
-             */
-            TlsUtils.writeVersion(clientVersion, M, 0);
-        }
-
         return M;
     }
 }