RT3425: constant-time evp_enc

Do the final padding check in EVP_DecryptFinal_ex in constant time to avoid a timing leak from padding failure. Reviewed-by: Rich Salz <rsalz@openssl.org>
openssl · Sep 24, 2014 · 4aac102 · 4aac102
1 parent 455b65d
commit 4aac102
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 26 deletions.
diff --git a/crypto/evp/Makefile b/crypto/evp/Makefile
@@ -406,7 +406,7 @@ evp_enc.o: ../../include/openssl/pkcs7.h ../../include/openssl/rand.h
 evp_enc.o: ../../include/openssl/safestack.h ../../include/openssl/sha.h
 evp_enc.o: ../../include/openssl/stack.h ../../include/openssl/symhacks.h
 evp_enc.o: ../../include/openssl/x509.h ../../include/openssl/x509_vfy.h
-evp_enc.o: ../cryptlib.h evp_enc.c evp_locl.h
+evp_enc.o: ../constant_time_locl.h ../cryptlib.h evp_enc.c evp_locl.h
 evp_err.o: ../../include/openssl/asn1.h ../../include/openssl/bio.h
 evp_err.o: ../../include/openssl/crypto.h ../../include/openssl/e_os2.h
 evp_err.o: ../../include/openssl/err.h ../../include/openssl/evp.h

diff --git a/crypto/evp/evp_enc.c b/crypto/evp/evp_enc.c
@@ -64,6 +64,7 @@
 #ifndef OPENSSL_NO_ENGINE
 #include <openssl/engine.h>
 #endif
+#include "../constant_time_locl.h"
 #include "evp_locl.h"
 
 const char EVP_version[]="EVP" OPENSSL_VERSION_PTEXT;
@@ -491,21 +492,21 @@ int EVP_DecryptFinal(EVP_CIPHER_CTX *ctx, unsigned char *out, int *outl)
 
 int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *out, int *outl)
 	{
-	int i,n;
-	unsigned int b;
+	unsigned int i, b;
+        unsigned char pad, padding_good;
 	*outl=0;
 
 	if (ctx->cipher->flags & EVP_CIPH_FLAG_CUSTOM_CIPHER)
 		{
-		i = ctx->cipher->do_cipher(ctx, out, NULL, 0);
-		if (i < 0)
+		int ret = ctx->cipher->do_cipher(ctx, out, NULL, 0);
+		if (ret < 0)
 			return 0;
 		else
-			*outl = i;
+			*outl = ret;
 		return 1;
 		}
 
-	b=ctx->cipher->block_size;
+	b=(unsigned int)(ctx->cipher->block_size);
 	if (ctx->flags & EVP_CIPH_NO_PADDING)
 		{
 		if(ctx->buf_len)
@@ -524,28 +525,34 @@ int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *out, int *outl)
 			return(0);
 			}
 		OPENSSL_assert(b <= sizeof ctx->final);
-		n=ctx->final[b-1];
-		if (n == 0 || n > (int)b)
-			{
-			EVPerr(EVP_F_EVP_DECRYPTFINAL_EX,EVP_R_BAD_DECRYPT);
-			return(0);
-			}
-		for (i=0; i<n; i++)
+		pad=ctx->final[b-1];
+
+		padding_good = (unsigned char)(~constant_time_is_zero_8(pad));
+		padding_good &= constant_time_ge_8(b, pad);
+
+                for (i = 1; i < b; ++i)
 			{
-			if (ctx->final[--b] != n)
-				{
-				EVPerr(EVP_F_EVP_DECRYPTFINAL_EX,EVP_R_BAD_DECRYPT);
-				return(0);
-				}
+			unsigned char is_pad_index = constant_time_lt_8(i, pad);
+			unsigned char pad_byte_good = constant_time_eq_8(ctx->final[b-i-1], pad);
+			padding_good &= constant_time_select_8(is_pad_index, pad_byte_good, 0xff);
 			}
-		n=ctx->cipher->block_size-n;
-		for (i=0; i<n; i++)
-			out[i]=ctx->final[i];
-		*outl=n;
+
+		/*
+		 * At least 1 byte is always padding, so we always write b - 1
+		 * bytes to avoid a timing leak. The caller is required to have |b|
+		 * bytes space in |out| by the API contract.
+		 */
+		for (i = 0; i < b - 1; ++i)
+			out[i] = ctx->final[i] & padding_good;
+		/* Safe cast: for a good padding, EVP_MAX_IV_LENGTH >= b >= pad */
+		*outl = padding_good & ((unsigned char)(b - pad));
+		return padding_good & 1;
 		}
 	else
-		*outl=0;
-	return(1);
+		{
+		*outl = 0;
+		return 1;
+		}
 	}
 
 void EVP_CIPHER_CTX_free(EVP_CIPHER_CTX *ctx)
@@ -664,4 +671,3 @@ int EVP_CIPHER_CTX_copy(EVP_CIPHER_CTX *out, const EVP_CIPHER_CTX *in)
 		return in->cipher->ctrl((EVP_CIPHER_CTX *)in, EVP_CTRL_COPY, 0, out);
 	return 1;
 	}
-