Normalize tls1_enc return values.

The distinction between publicly and non-publicly invalid is barely acted upon
and slightly silly now that the CBC padding check has been folded into
EVP_AEAD.

Change-Id: Idce4b9b8d29d624e3c95243a147265d071612127
Reviewed-on: https://boringssl-review.googlesource.com/2980
Reviewed-by: Adam Langley <[email protected]>

Author: davidben

ssl/d1_pkt.c

@@ -329,7 +329,6 @@ static int dtls1_process_buffered_records(SSL *s) {
 
 static int dtls1_process_record(SSL *s) {
   int al;
-  int enc_err;
   SSL3_RECORD *rr;
 
   rr = &(s->s3->rrec);
@@ -357,23 +356,12 @@ static int dtls1_process_record(SSL *s) {
   /* decrypt in place in 'rr->input' */
   rr->data = rr->input;
 
-  enc_err = s->enc_method->enc(s, 0);
-  /* enc_err is:
-   *    0: (in non-constant time) if the record is publically invalid.
-   *    1: if the padding is valid
-   *    -1: if the padding is invalid */
-  if (enc_err == 0) {
+  if (!s->enc_method->enc(s, 0)) {
     /* For DTLS we simply ignore bad packets. */
     rr->length = 0;
     s->packet_length = 0;
     goto err;
   }
-  if (enc_err < 0) {
-    /* decryption failed, silently discard message */
-    rr->length = 0;
-    s->packet_length = 0;
-    goto err;
-  }
 
   if (rr->length > SSL3_RT_MAX_PLAIN_LENGTH) {
     al = SSL_AD_RECORD_OVERFLOW;
@@ -1171,7 +1159,7 @@ static int do_dtls1_write(SSL *s, int type, const uint8_t *buf,
   wr->data = p;
   wr->length += eivlen;
 
-  if (s->enc_method->enc(s, 1) < 1) {
+  if (!s->enc_method->enc(s, 1)) {
     goto err;
   }
 

ssl/s3_pkt.c

@@ -268,7 +268,7 @@ int ssl3_read_n(SSL *s, int n, int max, int extend) {
 /* used only by ssl3_read_bytes */
 static int ssl3_get_record(SSL *s) {
   int ssl_major, ssl_minor, al;
-  int enc_err, n, i, ret = -1;
+  int n, i, ret = -1;
   SSL3_RECORD *rr;
   uint8_t *p;
   short version;
@@ -373,22 +373,7 @@ static int ssl3_get_record(SSL *s) {
   /* decrypt in place in 'rr->input' */
   rr->data = rr->input;
 
-  enc_err = s->enc_method->enc(s, 0);
-  /* enc_err is:
-   *    0: (in non-constant time) if the record is publically invalid.
-   *    1: if the padding is valid
-   *    -1: if the padding is invalid */
-  if (enc_err == 0) {
-    al = SSL_AD_DECRYPTION_FAILED;
-    OPENSSL_PUT_ERROR(SSL, ssl3_get_record, SSL_R_BLOCK_CIPHER_PAD_IS_WRONG);
-    goto f_err;
-  }
-  if (enc_err < 0) {
-    /* A separate 'decryption_failed' alert was introduced with TLS 1.0, SSL
-     * 3.0 only has 'bad_record_mac'.  But unless a decryption failure is
-     * directly visible from the ciphertext anyway, we should not reveal which
-     * kind of error occured – this might become visible to an attacker (e.g.
-     * via a logfile) */
+  if (!s->enc_method->enc(s, 0)) {
     al = SSL_AD_BAD_RECORD_MAC;
     OPENSSL_PUT_ERROR(SSL, ssl3_get_record,
                       SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC);
@@ -625,7 +610,7 @@ static int do_ssl3_write(SSL *s, int type, const uint8_t *buf, unsigned int len,
   wr->data = p;
   wr->length += eivlen;
 
-  if (s->enc_method->enc(s, 1) < 1) {
+  if (!s->enc_method->enc(s, 1)) {
     goto err;
   }
 

ssl/t1_enc.c

@@ -565,14 +565,7 @@ int tls1_setup_key_block(SSL *s) {
 }
 
 /* tls1_enc encrypts/decrypts the record in |s->wrec| / |s->rrec|,
- * respectively.
- *
- * Returns:
- *   0: (in non-constant time) if the record is publically invalid (i.e. too
- *       short etc).
- *   1: if the record's padding is valid / the encryption was successful.
- *   -1: if the record's padding/AEAD-authenticator is invalid or, if sending,
- *       an internal error occured. */
+ * respectively. It returns one on success and zero on failure. */
 int tls1_enc(SSL *s, int send) {
   SSL3_RECORD *rec;
   const SSL_AEAD_CTX *aead;
@@ -586,6 +579,7 @@ int tls1_enc(SSL *s, int send) {
   }
 
   if (s->session == NULL || aead == NULL) {
+    /* Handle the initial NULL cipher. */
     memmove(rec->data, rec->input, rec->length);
     rec->input = rec->data;
     return 1;
@@ -623,7 +617,7 @@ int tls1_enc(SSL *s, int send) {
 
   if (aead->fixed_nonce_len + aead->variable_nonce_len > sizeof(nonce)) {
     OPENSSL_PUT_ERROR(SSL, tls1_enc, ERR_R_INTERNAL_ERROR);
-    return -1; /* internal error - should never happen. */
+    return 0;
   }
 
   memcpy(nonce, aead->fixed_nonce, aead->fixed_nonce_len);
@@ -639,14 +633,14 @@ int tls1_enc(SSL *s, int send) {
     if (aead->random_variable_nonce) {
       assert(aead->variable_nonce_included_in_record);
       if (!RAND_bytes(nonce + nonce_used, aead->variable_nonce_len)) {
-        return -1;
+        return 0;
       }
     } else {
       /* When sending we use the sequence number as the variable part of the
        * nonce. */
       if (aead->variable_nonce_len != 8) {
         OPENSSL_PUT_ERROR(SSL, tls1_enc, ERR_R_INTERNAL_ERROR);
-        return -1;
+        return 0;
       }
       memcpy(nonce + nonce_used, ad, aead->variable_nonce_len);
     }
@@ -669,7 +663,7 @@ int tls1_enc(SSL *s, int send) {
 
     if (!EVP_AEAD_CTX_seal(&aead->ctx, out + eivlen, &n, len + aead->tag_len,
                            nonce, nonce_used, in + eivlen, len, ad, ad_len)) {
-      return -1;
+      return 0;
     }
 
     if (aead->variable_nonce_included_in_record) {
@@ -681,7 +675,7 @@ int tls1_enc(SSL *s, int send) {
 
     if (rec->data != rec->input) {
       OPENSSL_PUT_ERROR(SSL, tls1_enc, ERR_R_INTERNAL_ERROR);
-      return -1; /* internal error - should never happen. */
+      return 0;
     }
     out = in = rec->input;
 
@@ -711,7 +705,7 @@ int tls1_enc(SSL *s, int send) {
 
     if (!EVP_AEAD_CTX_open(&aead->ctx, out, &n, rec->length, nonce, nonce_used, in,
                            len, ad, ad_len)) {
-      return -1;
+      return 0;
     }
 
     rec->data = rec->input = out;