Fix up EVP_tls_cbc_remove_padding's calling convention.

The old one was rather confusing. Switch to returning 1/0 for whether
the padding is publicly invalid and then add an output argument which
returns a constant_time_eq-style boolean.

Change-Id: Ieba89d352faf80e9bcea993b716f4b2df5439d4b
Reviewed-on: https://boringssl-review.googlesource.com/10222
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/cipher/e_tls.c

diff --git a/crypto/cipher/e_tls.c b/crypto/cipher/e_tls.c
index b87b0d6..b562a53 100644
--- a/crypto/cipher/e_tls.c
+++ b/crypto/cipher/e_tls.c
@@ -264,20 +264,18 @@
 
   /* Remove CBC padding. Code from here on is timing-sensitive with respect to
    * |padding_ok| and |data_plus_mac_len| for CBC ciphers. */
-  int padding_ok;
-  unsigned data_plus_mac_len, data_len;
+  unsigned padding_ok, data_plus_mac_len, data_len;
   if (EVP_CIPHER_CTX_mode(&tls_ctx->cipher_ctx) == EVP_CIPH_CBC_MODE) {
-    padding_ok = EVP_tls_cbc_remove_padding(
-        &data_plus_mac_len, out, total,
-        EVP_CIPHER_CTX_block_size(&tls_ctx->cipher_ctx),
-        (unsigned)HMAC_size(&tls_ctx->hmac_ctx));
-    /* Publicly invalid. This can be rejected in non-constant time. */
-    if (padding_ok == 0) {
+    if (!EVP_tls_cbc_remove_padding(
+            &padding_ok, &data_plus_mac_len, out, total,
+            EVP_CIPHER_CTX_block_size(&tls_ctx->cipher_ctx),
+            (unsigned)HMAC_size(&tls_ctx->hmac_ctx))) {
+      /* Publicly invalid. This can be rejected in non-constant time. */
       OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BAD_DECRYPT);
       return 0;
     }
   } else {
-    padding_ok = 1;
+    padding_ok = ~0u;
     data_plus_mac_len = total;
     /* |data_plus_mac_len| = |total| = |in_len| at this point. |in_len| has
      * already been checked against the MAC size at the top of the function. */
@@ -285,9 +283,9 @@
   }
   data_len = data_plus_mac_len - HMAC_size(&tls_ctx->hmac_ctx);
 
-  /* At this point, |padding_ok| is 1 or -1. If 1, the padding is valid and the
-   * first |data_plus_mac_size| bytes after |out| are the plaintext and
-   * MAC. Either way, |data_plus_mac_size| is large enough to extract a MAC. */
+  /* At this point, if the padding is valid, the first |data_plus_mac_len| bytes
+   * after |out| are the plaintext and MAC. Otherwise, |data_plus_mac_len| is
+   * still large enough to extract a MAC, but it will be irrelevant. */
 
   /* To allow for CBC mode which changes cipher length, |ad| doesn't include the
    * length for legacy ciphers. */
@@ -338,7 +336,7 @@
    * EVP_tls_cbc_remove_padding. */
   unsigned good = constant_time_eq_int(CRYPTO_memcmp(record_mac, mac, mac_len),
                                        0);
-  good &= constant_time_eq_int(padding_ok, 1);
+  good &= padding_ok;
   if (!good) {
     OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BAD_DECRYPT);
     return 0;

crypto/cipher/internal.h

diff --git a/crypto/cipher/internal.h b/crypto/cipher/internal.h
index 72ac118..dba3912 100644
--- a/crypto/cipher/internal.h
+++ b/crypto/cipher/internal.h
@@ -104,15 +104,15 @@
 
 /* EVP_tls_cbc_get_padding determines the padding from the decrypted, TLS, CBC
  * record in |in|. This decrypted record should not include any "decrypted"
- * explicit IV. It sets |*out_len| to the length with the padding removed or
- * |in_len| if invalid.
+ * explicit IV. If the record is publicly invalid, it returns zero. Otherwise,
+ * it returns one and sets |*out_padding_ok| to all ones (0xfff..f) if the
+ * padding is valid and zero otherwise. It then sets |*out_len| to the length
+ * with the padding removed or |in_len| if invalid.
  *
- * block_size: the block size of the cipher used to encrypt the record.
- * returns:
- *   0: (in non-constant time) if the record is publicly invalid.
- *   1: if the padding was valid
- *  -1: otherwise. */
-int EVP_tls_cbc_remove_padding(unsigned *out_len,
+ * If the function returns one, it runs in time independent of the contents of
+ * |in|. It is also guaranteed that |*out_len| >= |mac_size|, satisfying
+ * |EVP_tls_cbc_copy_mac|'s precondition. */
+int EVP_tls_cbc_remove_padding(unsigned *out_padding_ok, unsigned *out_len,
                                const uint8_t *in, unsigned in_len,
                                unsigned block_size, unsigned mac_size);
 

crypto/cipher/tls_cbc.c

diff --git a/crypto/cipher/tls_cbc.c b/crypto/cipher/tls_cbc.c
index fbc93fa..a0dc509 100644
--- a/crypto/cipher/tls_cbc.c
+++ b/crypto/cipher/tls_cbc.c
@@ -73,7 +73,7 @@
  * supported by TLS.) */
 #define MAX_HASH_BLOCK_SIZE 128
 
-int EVP_tls_cbc_remove_padding(unsigned *out_len,
+int EVP_tls_cbc_remove_padding(unsigned *out_padding_ok, unsigned *out_len,
                                const uint8_t *in, unsigned in_len,
                                unsigned block_size, unsigned mac_size) {
   unsigned padding_length, good, to_check, i;
@@ -119,8 +119,8 @@
    * bad padding would give POODLE's padding oracle. */
   padding_length = good & (padding_length + 1);
   *out_len = in_len - padding_length;
-
-  return constant_time_select_int(good, 1, -1);
+  *out_padding_ok = good;
+  return 1;
 }
 
 /* If CBC_MAC_ROTATE_IN_PLACE is defined then EVP_tls_cbc_copy_mac is performed