RT3066: rewrite RSA padding checks to be slightly more constant time.

Also tweak s3_cbc.c to use new constant-time methods.
Also fix memory leaks from internal errors in RSA_padding_check_PKCS1_OAEP_mgf1

This patch is based on the original RT submission by Adam Langley <agl@chromium.org>,
as well as code from BoringSSL and OpenSSL.

Reviewed-by: Kurt Roeckx <kurt@openssl.org>

Conflicts:
	crypto/rsa/rsa_oaep.c
	crypto/rsa/rsa_pk1.c
	ssl/s3_cbc.c

5 files changed, 168 insertions, 95 deletions

diff --git a/crypto/rsa/Makefile b/crypto/rsa/Makefile
index b61e9e9deb..af487b6004 100644
--- a/crypto/rsa/Makefile
+++ b/crypto/rsa/Makefile
@@ -212,7 +212,7 @@ rsa_oaep.o: ../../include/openssl/opensslv.h ../../include/openssl/ossl_typ.h
 rsa_oaep.o: ../../include/openssl/rand.h ../../include/openssl/rsa.h
 rsa_oaep.o: ../../include/openssl/safestack.h ../../include/openssl/sha.h
 rsa_oaep.o: ../../include/openssl/stack.h ../../include/openssl/symhacks.h
-rsa_oaep.o: ../cryptlib.h rsa_oaep.c
+rsa_oaep.o: ../constant_time_locl.h ../cryptlib.h rsa_oaep.c
 rsa_pk1.o: ../../e_os.h ../../include/openssl/asn1.h
 rsa_pk1.o: ../../include/openssl/bio.h ../../include/openssl/bn.h
 rsa_pk1.o: ../../include/openssl/buffer.h ../../include/openssl/crypto.h
@@ -221,7 +221,8 @@ rsa_pk1.o: ../../include/openssl/lhash.h ../../include/openssl/opensslconf.h
 rsa_pk1.o: ../../include/openssl/opensslv.h ../../include/openssl/ossl_typ.h
 rsa_pk1.o: ../../include/openssl/rand.h ../../include/openssl/rsa.h
 rsa_pk1.o: ../../include/openssl/safestack.h ../../include/openssl/stack.h
-rsa_pk1.o: ../../include/openssl/symhacks.h ../cryptlib.h rsa_pk1.c
+rsa_pk1.o: ../../include/openssl/symhacks.h ../constant_time_locl.h
+rsa_pk1.o: ../cryptlib.h rsa_pk1.c
 rsa_pmeth.o: ../../e_os.h ../../include/openssl/asn1.h
 rsa_pmeth.o: ../../include/openssl/asn1t.h ../../include/openssl/bio.h
 rsa_pmeth.o: ../../include/openssl/bn.h ../../include/openssl/buffer.h
diff --git a/crypto/rsa/rsa.h b/crypto/rsa/rsa.h
index 2d57c2f386..f8e0cd04d5 100644
--- a/crypto/rsa/rsa.h
+++ b/crypto/rsa/rsa.h
@@ -576,7 +576,7 @@ void ERR_load_RSA_strings(void);
 #define RSA_R_DATA_TOO_LARGE_FOR_MODULUS		 132
 #define RSA_R_DATA_TOO_SMALL				 111
 #define RSA_R_DATA_TOO_SMALL_FOR_KEY_SIZE		 122
-#define RSA_R_DIGEST_DOES_NOT_MATCH			 159
+#define RSA_R_DIGEST_DOES_NOT_MATCH			 166
 #define RSA_R_DIGEST_TOO_BIG_FOR_RSA_KEY		 112
 #define RSA_R_DMP1_NOT_CONGRUENT_TO_D			 124
 #define RSA_R_DMQ1_NOT_CONGRUENT_TO_D			 125
@@ -610,6 +610,7 @@ void ERR_load_RSA_strings(void);
 #define RSA_R_OPERATION_NOT_ALLOWED_IN_FIPS_MODE	 158
 #define RSA_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE	 148
 #define RSA_R_PADDING_CHECK_FAILED			 114
+#define RSA_R_PKCS_DECODING_ERROR			 159
 #define RSA_R_P_NOT_PRIME				 128
 #define RSA_R_Q_NOT_PRIME				 129
 #define RSA_R_RSA_OPERATIONS_NOT_SUPPORTED		 130
diff --git a/crypto/rsa/rsa_err.c b/crypto/rsa/rsa_err.c
index 3a5c580209..ae905c6cb1 100644
--- a/crypto/rsa/rsa_err.c
+++ b/crypto/rsa/rsa_err.c
@@ -1,6 +1,6 @@
 /* crypto/rsa/rsa_err.c */
 /* ====================================================================
- * Copyright (c) 1999-2013 The OpenSSL Project.  All rights reserved.
+ * Copyright (c) 1999-2014 The OpenSSL Project.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -185,6 +185,7 @@ static ERR_STRING_DATA RSA_str_reasons[]=
 {ERR_REASON(RSA_R_OPERATION_NOT_ALLOWED_IN_FIPS_MODE),"operation not allowed in fips mode"},
 {ERR_REASON(RSA_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE),"operation not supported for this keytype"},
 {ERR_REASON(RSA_R_PADDING_CHECK_FAILED)  ,"padding check failed"},
+{ERR_REASON(RSA_R_PKCS_DECODING_ERROR)   ,"pkcs decoding error"},
 {ERR_REASON(RSA_R_P_NOT_PRIME)           ,"p not prime"},
 {ERR_REASON(RSA_R_Q_NOT_PRIME)           ,"q not prime"},
 {ERR_REASON(RSA_R_RSA_OPERATIONS_NOT_SUPPORTED),"rsa operations not supported"},
diff --git a/crypto/rsa/rsa_oaep.c b/crypto/rsa/rsa_oaep.c
index 5446d4db92..1a7a0c7301 100644
--- a/crypto/rsa/rsa_oaep.c
+++ b/crypto/rsa/rsa_oaep.c
@@ -18,6 +18,7 @@
  * an equivalent notion.
  */
 
+#include "../constant_time_locl.h"
 
 #if !defined(OPENSSL_NO_SHA) && !defined(OPENSSL_NO_SHA1)
 #include <stdio.h>
@@ -119,12 +120,13 @@ int RSA_padding_check_PKCS1_OAEP_mgf1(unsigned char *to, int tlen,
 	const unsigned char *param, int plen,
 	const EVP_MD *md, const EVP_MD *mgf1md)
 	{
-	int i, dblen, mlen = -1;
-	const unsigned char *maskeddb;
-	int lzero;
-	unsigned char *db = NULL, seed[EVP_MAX_MD_SIZE], phash[EVP_MAX_MD_SIZE];
-	unsigned char *padded_from;
-	int bad = 0;
+	int i, dblen, mlen = -1, one_index = 0, msg_index;
+	unsigned int good, found_one_byte;
+	const unsigned char *maskedseed, *maskeddb;
+	/* |em| is the encoded message, zero-padded to exactly |num| bytes:
+	 * em = Y || maskedSeed || maskedDB */
+	unsigned char *db = NULL, *em = NULL, seed[EVP_MAX_MD_SIZE],
+		phash[EVP_MAX_MD_SIZE];
 	int mdlen;
 
 	if (md == NULL)
@@ -134,85 +136,108 @@ int RSA_padding_check_PKCS1_OAEP_mgf1(unsigned char *to, int tlen,
 
 	mdlen = EVP_MD_size(md);
 
-	if (--num < 2 * mdlen + 1)
-		/* 'num' is the length of the modulus, i.e. does not depend on the
-		 * particular ciphertext. */
+        if (tlen <= 0 || flen <= 0)
+		return -1;
+	/*
+	 * |num| is the length of the modulus; |flen| is the length of the
+	 * encoded message. Therefore, for any |from| that was obtained by
+	 * decrypting a ciphertext, we must have |flen| <= |num|. Similarly,
+	 * num < 2 * mdlen + 2 must hold for the modulus irrespective of
+	 * the ciphertext, see PKCS #1 v2.2, section 7.1.2.
+	 * This does not leak any side-channel information.
+	 */
+	if (num < flen || num < 2 * mdlen + 2)
 		goto decoding_err;
 
-	lzero = num - flen;
-	if (lzero < 0)
-		{
-		/* signalling this error immediately after detection might allow
-		 * for side-channel attacks (e.g. timing if 'plen' is huge
-		 * -- cf. James H. Manger, "A Chosen Ciphertext Attack on RSA Optimal
-		 * Asymmetric Encryption Padding (OAEP) [...]", CRYPTO 2001),
-		 * so we use a 'bad' flag */
-		bad = 1;
-		lzero = 0;
-		flen = num; /* don't overflow the memcpy to padded_from */
-		}
-
-	dblen = num - mdlen;
-	db = OPENSSL_malloc(dblen + num);
-	if (db == NULL)
+	dblen = num - mdlen - 1;
+	db = OPENSSL_malloc(dblen);
+	em = OPENSSL_malloc(num);
+	if (db == NULL || em == NULL)
 		{
 		RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_OAEP_MGF1, ERR_R_MALLOC_FAILURE);
-		return -1;
+		goto cleanup;
 		}
 
-	/* Always do this zero-padding copy (even when lzero == 0)
-	 * to avoid leaking timing info about the value of lzero. */
-	padded_from = db + dblen;
-	memset(padded_from, 0, lzero);
-	memcpy(padded_from + lzero, from, flen);
-
-	maskeddb = padded_from + mdlen;
+	/*
+	 * Always do this zero-padding copy (even when num == flen) to avoid
+	 * leaking that information. The copy still leaks some side-channel
+	 * information, but it's impossible to have a fixed  memory access
+	 * pattern since we can't read out of the bounds of |from|.
+	 *
+	 * TODO(emilia): Consider porting BN_bn2bin_padded from BoringSSL.
+	 */
+	memset(em, 0, num);
+	memcpy(em + num - flen, from, flen);
+
+	/*
+	 * The first byte must be zero, however we must not leak if this is
+	 * true. See James H. Manger, "A Chosen Ciphertext  Attack on RSA
+	 * Optimal Asymmetric Encryption Padding (OAEP) [...]", CRYPTO 2001).
+	 */
+	good = constant_time_is_zero(em[0]);
+
+	maskedseed = em + 1;
+	maskeddb = em + 1 + mdlen;
 
 	if (PKCS1_MGF1(seed, mdlen, maskeddb, dblen, mgf1md))
-		return -1;
+		goto cleanup;
 	for (i = 0; i < mdlen; i++)
-		seed[i] ^= padded_from[i];
-  
+		seed[i] ^= maskedseed[i];
+
 	if (PKCS1_MGF1(db, dblen, seed, mdlen, mgf1md))
-		return -1;
+		goto cleanup;
 	for (i = 0; i < dblen; i++)
 		db[i] ^= maskeddb[i];
 
 	if (!EVP_Digest((void *)param, plen, phash, NULL, md, NULL))
-		return -1;
+		goto cleanup;
+
+	good &= constant_time_is_zero(CRYPTO_memcmp(db, phash, mdlen));
+
+	found_one_byte = 0;
+	for (i = mdlen; i < dblen; i++)
+		{
+		/* Padding consists of a number of 0-bytes, followed by a 1. */
+		unsigned int equals1 = constant_time_eq(db[i], 1);
+		unsigned int equals0 = constant_time_is_zero(db[i]);
+		one_index = constant_time_select_int(~found_one_byte & equals1,
+			i, one_index);
+		found_one_byte |= equals1;
+		good &= (found_one_byte | equals0);
+		}
+
+	good &= found_one_byte;
 
-	if (CRYPTO_memcmp(db, phash, mdlen) != 0 || bad)
+	/*
+	 * At this point |good| is zero unless the plaintext was valid,
+	 * so plaintext-awareness ensures timing side-channels are no longer a
+	 * concern.
+	 */
+	if (!good)
 		goto decoding_err;
+
+	msg_index = one_index + 1;
+	mlen = dblen - msg_index;
+
+	if (tlen < mlen)
+		{
+		RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_OAEP_MGF1, RSA_R_DATA_TOO_LARGE);
+		mlen = -1;
+		}
 	else
 		{
-		for (i = mdlen; i < dblen; i++)
-			if (db[i] != 0x00)
-				break;
-		if (i == dblen || db[i] != 0x01)
-			goto decoding_err;
-		else
-			{
-			/* everything looks OK */
-
-			mlen = dblen - ++i;
-			if (tlen < mlen)
-				{
-				RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_OAEP_MGF1, RSA_R_DATA_TOO_LARGE);
-				mlen = -1;
-				}
-			else
-				memcpy(to, db + i, mlen);
-			}
+		memcpy(to, db + msg_index, mlen);
+		goto cleanup;
 		}
-	OPENSSL_free(db);
-	return mlen;
 
 decoding_err:
-	/* to avoid chosen ciphertext attacks, the error message should not reveal
-	 * which kind of decoding error happened */
+	/* To avoid chosen ciphertext attacks, the error message should not reveal
+	 * which kind of decoding error happened. */
 	RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_OAEP_MGF1, RSA_R_OAEP_DECODING_ERROR);
+cleanup:
 	if (db != NULL) OPENSSL_free(db);
-	return -1;
+	if (em != NULL) OPENSSL_free(em);
+	return mlen;
 	}
 
 int PKCS1_MGF1(unsigned char *mask, long len,
diff --git a/crypto/rsa/rsa_pk1.c b/crypto/rsa/rsa_pk1.c
index 8560755f1d..e028ff7189 100644
--- a/crypto/rsa/rsa_pk1.c
+++ b/crypto/rsa/rsa_pk1.c
@@ -56,6 +56,8 @@
  * [including the GNU Public Licence.]
  */
 
+#include "../constant_time_locl.h"
+
 #include <stdio.h>
 #include "cryptlib.h"
 #include <openssl/bn.h>
@@ -181,44 +183,87 @@ int RSA_padding_add_PKCS1_type_2(unsigned char *to, int tlen,
 int RSA_padding_check_PKCS1_type_2(unsigned char *to, int tlen,
 	     const unsigned char *from, int flen, int num)
 	{
-	int i,j;
-	const unsigned char *p;
+	int i;
+	/* |em| is the encoded message, zero-padded to exactly |num| bytes */
+	unsigned char *em = NULL;
+	unsigned int good, found_zero_byte;
+	int zero_index = 0, msg_index, mlen = -1;
 
-	p=from;
-	if ((num != (flen+1)) || (*(p++) != 02))
-		{
-		RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2,RSA_R_BLOCK_TYPE_IS_NOT_02);
-		return(-1);
-		}
-#ifdef PKCS1_CHECK
-	return(num-11);
-#endif
+        if (tlen < 0 || flen < 0)
+		return -1;
 
-	/* scan over padding data */
-	j=flen-1; /* one for type. */
-	for (i=0; i<j; i++)
-		if (*(p++) == 0) break;
+	/* PKCS#1 v1.5 decryption. See "PKCS #1 v2.2: RSA Cryptography
+	 * Standard", section 7.2.2. */
 
-	if (i == j)
+	if (flen > num)
+		goto err;
+
+	if (num < 11)
+		goto err;
+
+	em = OPENSSL_malloc(num);
+	if (em == NULL)
 		{
-		RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2,RSA_R_NULL_BEFORE_BLOCK_MISSING);
-		return(-1);
+		RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2, ERR_R_MALLOC_FAILURE);
+		return -1;
 		}
+	memset(em, 0, num);
+	/*
+	 * Always do this zero-padding copy (even when num == flen) to avoid
+	 * leaking that information. The copy still leaks some side-channel
+	 * information, but it's impossible to have a fixed  memory access
+	 * pattern since we can't read out of the bounds of |from|.
+	 *
+	 * TODO(emilia): Consider porting BN_bn2bin_padded from BoringSSL.
+	 */
+	memcpy(em + num - flen, from, flen);
 
-	if (i < 8)
+	good = constant_time_is_zero(em[0]);
+	good &= constant_time_eq(em[1], 2);
+
+	found_zero_byte = 0;
+	for (i = 2; i < num; i++)
 		{
-		RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2,RSA_R_BAD_PAD_BYTE_COUNT);
-		return(-1);
+		unsigned int equals0 = constant_time_is_zero(em[i]);
+		zero_index = constant_time_select_int(~found_zero_byte & equals0, i, zero_index);
+		found_zero_byte |= equals0;
 		}
-	i++; /* Skip over the '\0' */
-	j-=i;
-	if (j > tlen)
+
+	/*
+	 * PS must be at least 8 bytes long, and it starts two bytes into |em|.
+         * If we never found a 0-byte, then |zero_index| is 0 and the check
+	 * also fails.
+	 */
+	good &= constant_time_ge((unsigned int)(zero_index), 2 + 8);
+
+	/* Skip the zero byte. This is incorrect if we never found a zero-byte
+	 * but in this case we also do not copy the message out. */
+	msg_index = zero_index + 1;
+	mlen = num - msg_index;
+
+	/* For good measure, do this check in constant time as well; it could
+	 * leak something if |tlen| was assuming valid padding. */
+	good &= constant_time_ge((unsigned int)(tlen), (unsigned int)(mlen));
+
+	/*
+	 * We can't continue in constant-time because we need to copy the result
+	 * and we cannot fake its length. This unavoidably leaks timing
+	 * information at the API boundary.
+	 * TODO(emilia): this could be addressed at the call site,
+	 * see BoringSSL commit 0aa0767340baf925bda4804882aab0cb974b2d26.
+	 */
+	if (!good)
 		{
-		RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2,RSA_R_DATA_TOO_LARGE);
-		return(-1);
+		mlen = -1;
+		goto err;
 		}
-	memcpy(to,p,(unsigned int)j);
 
-	return(j);
-	}
+	memcpy(to, em + msg_index, mlen);
 
+err:
+	if (em != NULL)
+		OPENSSL_free(em);
+	if (mlen == -1)
+		RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2, RSA_R_PKCS_DECODING_ERROR);
+	return mlen;
+	}