From f7c6723fdc5142e43edb79d4c5963acc26da7088 Mon Sep 17 00:00:00 2001
From: Greg Hudson <ghudson@mit.edu>
Date: Thu, 16 Jun 2016 16:38:07 -0400
Subject: [PATCH] Minimize timing leaks in PKINIT decryption

pkcs7_dataDecode() is derived from OpenSSL's PKCS7_datadecode() and is
used by PKINIT clients to decrypt ReplyKeyPack values in RSA mode.
The upstream function was changed for CVE-2012-0884 to minimize the
timing difference when RSA decryption results in the wrong padding.
Although the impact on Kerberos is negligible (because clients do not
ordinarily choose to use RSA mode, and cannot easily be induced to
make many thousands of requests with the same key), change
pkcs7_dataDecode() to match the upstream change, generating a random
symmetric key and using it when RSA decryption fails.  Also rename
"tmp" and "tmp_len" to "ek" and "eklen" to match the more descriptive
upstream variable names.

ticket: 8428 (new)
---
 .../preauth/pkinit/pkinit_crypto_openssl.c    | 57 +++++++++++--------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
index be936116a8..98a48a4b97 100644
--- a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
+++ b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
@@ -5814,9 +5814,9 @@ pkcs7_dataDecode(krb5_context context,
                  pkinit_identity_crypto_context id_cryptoctx,
                  PKCS7 *p7)
 {
-    unsigned int jj = 0, tmp_len = 0;
+    unsigned int eklen=0, tkeylen=0;
     BIO *out=NULL,*etmp=NULL,*bio=NULL;
-    unsigned char *tmp=NULL;
+    unsigned char *ek=NULL, *tkey=NULL;
     ASN1_OCTET_STRING *data_body=NULL;
     const EVP_CIPHER *evp_cipher=NULL;
     EVP_CIPHER_CTX *evp_ctx=NULL;
@@ -5851,15 +5851,10 @@ pkcs7_dataDecode(krb5_context context,
     }
 
     ri = sk_PKCS7_RECIP_INFO_value(rsk, 0);
-    jj = pkinit_decode_data(context, id_cryptoctx,
-                            M_ASN1_STRING_data(ri->enc_key),
-                            (unsigned int)M_ASN1_STRING_length(ri->enc_key),
-                            &tmp, &tmp_len);
-    if (jj || tmp_len <= 0) {
-        PKCS7err(PKCS7_F_PKCS7_DATADECODE, ERR_R_EVP_LIB);
-        goto cleanup;
-    }
-    jj = tmp_len;
+    (void)pkinit_decode_data(context, id_cryptoctx,
+                             M_ASN1_STRING_data(ri->enc_key),
+                             (unsigned int)M_ASN1_STRING_length(ri->enc_key),
+                             &ek, &eklen);
 
     evp_ctx=NULL;
     BIO_get_cipher_ctx(etmp,&evp_ctx);
@@ -5868,22 +5863,34 @@ pkcs7_dataDecode(krb5_context context,
     if (EVP_CIPHER_asn1_to_param(evp_ctx,enc_alg->parameter) < 0)
         goto cleanup;
 
-    if (jj != (unsigned) EVP_CIPHER_CTX_key_length(evp_ctx)) {
+    /* Generate a random symmetric key to avoid exposing timing data if RSA
+     * decryption fails the padding check. */
+    tkeylen = EVP_CIPHER_CTX_key_length(evp_ctx);
+    tkey = OPENSSL_malloc(tkeylen);
+    if (tkey == NULL)
+        goto cleanup;
+    if (EVP_CIPHER_CTX_rand_key(evp_ctx, tkey) <= 0)
+        goto cleanup;
+    if (ek == NULL) {
+        ek = tkey;
+        eklen = tkeylen;
+        tkey = NULL;
+    }
+
+    if (eklen != (unsigned)EVP_CIPHER_CTX_key_length(evp_ctx)) {
         /* Some S/MIME clients don't use the same key
          * and effective key length. The key length is
          * determined by the size of the decrypted RSA key.
          */
-        if(!EVP_CIPHER_CTX_set_key_length(evp_ctx, (int)jj)) {
-            PKCS7err(PKCS7_F_PKCS7_DATADECODE,
-                     PKCS7_R_DECRYPTED_KEY_IS_WRONG_LENGTH);
-            goto cleanup;
+        if (!EVP_CIPHER_CTX_set_key_length(evp_ctx, (int)eklen)) {
+            ek = tkey;
+            eklen = tkeylen;
+            tkey = NULL;
         }
     }
-    if (EVP_CipherInit_ex(evp_ctx,NULL,NULL,tmp,NULL,0) <= 0)
+    if (EVP_CipherInit_ex(evp_ctx,NULL,NULL,ek,NULL,0) <= 0)
         goto cleanup;
 
-    OPENSSL_cleanse(tmp,jj);
-
     if (out == NULL)
         out=etmp;
     else
@@ -5906,10 +5913,14 @@ pkcs7_dataDecode(krb5_context context,
         if (bio != NULL) BIO_free_all(bio);
         out=NULL;
     }
-
-    if (tmp != NULL)
-        free(tmp);
-
+    if (ek != NULL) {
+        OPENSSL_cleanse(ek, eklen);
+        OPENSSL_free(ek);
+    }
+    if (tkey != NULL) {
+        OPENSSL_cleanse(tkey, tkeylen);
+        OPENSSL_free(tkey);
+    }
     return(out);
 }
 
-- 
2.47.2