When you expect a function to return a particular value, don't put a comment
authormiod <miod@openbsd.org>
Thu, 7 Aug 2014 19:46:31 +0000 (19:46 +0000)
committermiod <miod@openbsd.org>
Thu, 7 Aug 2014 19:46:31 +0000 (19:46 +0000)
saying that you expect it to return that value and compare it against zero
because it is supposedly faster, for this leads to bugs (especially given the
high rate of sloppy cut'n'paste within ssl3 and dtls1 routines in this
library).

Instead, compare for the exact value it ought to return upon success.

ok deraadt@

15 files changed:
lib/libssl/d1_both.c
lib/libssl/d1_clnt.c
lib/libssl/d1_pkt.c
lib/libssl/s23_srvr.c
lib/libssl/s3_both.c
lib/libssl/s3_lib.c
lib/libssl/src/ssl/d1_both.c
lib/libssl/src/ssl/d1_clnt.c
lib/libssl/src/ssl/d1_pkt.c
lib/libssl/src/ssl/s23_srvr.c
lib/libssl/src/ssl/s3_both.c
lib/libssl/src/ssl/s3_enc.c
lib/libssl/src/ssl/s3_lib.c
lib/libssl/src/ssl/t1_enc.c
lib/libssl/t1_enc.c

index e25f69d..2391d52 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_both.c,v 1.24 2014/07/10 08:51:14 tedu Exp $ */
+/* $OpenBSD: d1_both.c,v 1.25 2014/08/07 19:46:31 miod Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -903,6 +903,7 @@ dtls1_send_finished(SSL *s, int a, int b, const char *sender, int slen)
 
                i = s->method->ssl3_enc->final_finish_mac(s, sender, slen,
                    s->s3->tmp.finish_md);
+               OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                s->s3->tmp.finish_md_len = i;
                memcpy(p, s->s3->tmp.finish_md, i);
                p += i;
@@ -913,12 +914,10 @@ dtls1_send_finished(SSL *s, int a, int b, const char *sender, int slen)
                 * renegotiation checks
                 */
                if (s->type == SSL_ST_CONNECT) {
-                       OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                        memcpy(s->s3->previous_client_finished,
                            s->s3->tmp.finish_md, i);
                        s->s3->previous_client_finished_len = i;
                } else {
-                       OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                        memcpy(s->s3->previous_server_finished,
                            s->s3->tmp.finish_md, i);
                        s->s3->previous_server_finished_len = i;
index 552667f..165f944 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_clnt.c,v 1.31 2014/07/12 22:33:39 jsing Exp $ */
+/* $OpenBSD: d1_clnt.c,v 1.32 2014/08/07 19:46:31 miod Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -778,8 +778,9 @@ dtls1_client_hello(SSL *s)
 
                /* if client_random is initialized, reuse it, we are
                 * required to use same upon reply to HelloVerify */
-               for (i = 0; p[i]=='\0' && i < sizeof(s->s3->client_random); i++)
-                       ;
+               for (i = 0; i < sizeof(s->s3->client_random); i++)
+                       if (p[i] != '\0')
+                               break;
                if (i == sizeof(s->s3->client_random))
                        RAND_pseudo_bytes(p, sizeof(s->s3->client_random));
 
@@ -1338,7 +1339,6 @@ dtls1_send_client_certificate(SSL *s)
                /* If we get an error, we need to
                 * ssl->rwstate=SSL_X509_LOOKUP; return(-1);
                 * We then get retied later */
-               i = 0;
                i = ssl_do_client_cert_cb(s, &x509, &pkey);
                if (i < 0) {
                        s->rwstate = SSL_X509_LOOKUP;
index c9ffab1..5be89f0 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_pkt.c,v 1.32 2014/07/10 08:51:14 tedu Exp $ */
+/* $OpenBSD: d1_pkt.c,v 1.33 2014/08/07 19:46:31 miod Exp $ */
 /* 
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.  
@@ -414,10 +414,12 @@ dtls1_process_record(SSL *s)
                }
 
                i = s->method->ssl3_enc->mac(s, md, 0 /* not send */);
-               if (i < 0 || mac == NULL || timingsafe_memcmp(md, mac, (size_t)mac_size) != 0)
+               if (i < 0 || mac == NULL ||
+                   timingsafe_memcmp(md, mac, (size_t)mac_size) != 0)
                        enc_err = -1;
                if (rr->length > SSL3_RT_MAX_COMPRESSED_LENGTH + mac_size)
                        enc_err = -1;
+               OPENSSL_cleanse(&md, sizeof md);
        }
 
        if (enc_err < 0) {
index ee97713..5f8ffa8 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: s23_srvr.c,v 1.32 2014/08/07 04:49:53 deraadt Exp $ */
+/* $OpenBSD: s23_srvr.c,v 1.33 2014/08/07 19:46:31 miod Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -448,11 +448,8 @@ ssl23_get_client_hello(SSL *s)
                }
 
                j = ssl23_read_bytes(s, n + 2);
-               /* We previously read 11 bytes, so if j > 0, we must have
-                * j == n+2 == s->packet_length. We have at least 11 valid
-                * packet bytes. */
-               if (j <= 0)
-                       return (j);
+               if (j != n + 2)
+                       return -1;
 
                ssl3_finish_mac(s, s->packet + 2, s->packet_length - 2);
                if (s->msg_callback)
index 500387e..afcaca3 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: s3_both.c,v 1.26 2014/07/10 08:51:14 tedu Exp $ */
+/* $OpenBSD: s3_both.c,v 1.27 2014/08/07 19:46:31 miod Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -161,7 +161,7 @@ ssl3_send_finished(SSL *s, int a, int b, const char *sender, int slen)
                p = &(d[4]);
 
                i = s->method->ssl3_enc->final_finish_mac(s,
-               sender, slen, s->s3->tmp.finish_md);
+                   sender, slen, s->s3->tmp.finish_md);
                if (i == 0)
                        return 0;
                s->s3->tmp.finish_md_len = i;
@@ -171,15 +171,14 @@ ssl3_send_finished(SSL *s, int a, int b, const char *sender, int slen)
 
                 /* Copy the finished so we can use it for
                    renegotiation checks */
+               OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                if (s->type == SSL_ST_CONNECT) {
-                       OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                        memcpy(s->s3->previous_client_finished,
-                       s->s3->tmp.finish_md, i);
+                           s->s3->tmp.finish_md, i);
                        s->s3->previous_client_finished_len = i;
                } else {
-                       OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                        memcpy(s->s3->previous_server_finished,
-                       s->s3->tmp.finish_md, i);
+                           s->s3->tmp.finish_md, i);
                        s->s3->previous_server_finished_len = i;
                }
 
@@ -216,7 +215,7 @@ ssl3_take_mac(SSL *s)
        }
 
        s->s3->tmp.peer_finish_md_len = s->method->ssl3_enc->final_finish_mac(s,
-       sender, slen, s->s3->tmp.peer_finish_md);
+           sender, slen, s->s3->tmp.peer_finish_md);
 }
 #endif
 
@@ -250,7 +249,7 @@ ssl3_get_finished(SSL *s, int a, int b)
        p = (unsigned char *)s->init_msg;
        i = s->s3->tmp.peer_finish_md_len;
 
-       if (i != n) {
+       if (i != n || i > EVP_MAX_MD_SIZE) {
                al = SSL_AD_DECODE_ERROR;
                SSLerr(SSL_F_SSL3_GET_FINISHED, SSL_R_BAD_DIGEST_LENGTH);
                goto f_err;
@@ -265,14 +264,12 @@ ssl3_get_finished(SSL *s, int a, int b)
         /* Copy the finished so we can use it for
            renegotiation checks */
        if (s->type == SSL_ST_ACCEPT) {
-               OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                memcpy(s->s3->previous_client_finished,
-               s->s3->tmp.peer_finish_md, i);
+                   s->s3->tmp.peer_finish_md, i);
                s->s3->previous_client_finished_len = i;
        } else {
-               OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                memcpy(s->s3->previous_server_finished,
-               s->s3->tmp.peer_finish_md, i);
+                   s->s3->tmp.peer_finish_md, i);
                s->s3->previous_server_finished_len = i;
        }
 
index 8a40b75..aa091f5 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: s3_lib.c,v 1.71 2014/07/13 16:03:10 beck Exp $ */
+/* $OpenBSD: s3_lib.c,v 1.72 2014/08/07 19:46:31 miod Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -2333,7 +2333,7 @@ ssl3_new(SSL *s)
        SSL3_STATE      *s3;
 
        if ((s3 = calloc(1, sizeof *s3)) == NULL)
-               goto err;
+               return 0;
        memset(s3->rrec.seq_num, 0, sizeof(s3->rrec.seq_num));
        memset(s3->wrec.seq_num, 0, sizeof(s3->wrec.seq_num));
 
@@ -2341,8 +2341,6 @@ ssl3_new(SSL *s)
 
        s->method->ssl_clear(s);
        return (1);
-err:
-       return (0);
 }
 
 void
index e25f69d..2391d52 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_both.c,v 1.24 2014/07/10 08:51:14 tedu Exp $ */
+/* $OpenBSD: d1_both.c,v 1.25 2014/08/07 19:46:31 miod Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -903,6 +903,7 @@ dtls1_send_finished(SSL *s, int a, int b, const char *sender, int slen)
 
                i = s->method->ssl3_enc->final_finish_mac(s, sender, slen,
                    s->s3->tmp.finish_md);
+               OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                s->s3->tmp.finish_md_len = i;
                memcpy(p, s->s3->tmp.finish_md, i);
                p += i;
@@ -913,12 +914,10 @@ dtls1_send_finished(SSL *s, int a, int b, const char *sender, int slen)
                 * renegotiation checks
                 */
                if (s->type == SSL_ST_CONNECT) {
-                       OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                        memcpy(s->s3->previous_client_finished,
                            s->s3->tmp.finish_md, i);
                        s->s3->previous_client_finished_len = i;
                } else {
-                       OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                        memcpy(s->s3->previous_server_finished,
                            s->s3->tmp.finish_md, i);
                        s->s3->previous_server_finished_len = i;
index 552667f..165f944 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_clnt.c,v 1.31 2014/07/12 22:33:39 jsing Exp $ */
+/* $OpenBSD: d1_clnt.c,v 1.32 2014/08/07 19:46:31 miod Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -778,8 +778,9 @@ dtls1_client_hello(SSL *s)
 
                /* if client_random is initialized, reuse it, we are
                 * required to use same upon reply to HelloVerify */
-               for (i = 0; p[i]=='\0' && i < sizeof(s->s3->client_random); i++)
-                       ;
+               for (i = 0; i < sizeof(s->s3->client_random); i++)
+                       if (p[i] != '\0')
+                               break;
                if (i == sizeof(s->s3->client_random))
                        RAND_pseudo_bytes(p, sizeof(s->s3->client_random));
 
@@ -1338,7 +1339,6 @@ dtls1_send_client_certificate(SSL *s)
                /* If we get an error, we need to
                 * ssl->rwstate=SSL_X509_LOOKUP; return(-1);
                 * We then get retied later */
-               i = 0;
                i = ssl_do_client_cert_cb(s, &x509, &pkey);
                if (i < 0) {
                        s->rwstate = SSL_X509_LOOKUP;
index c9ffab1..5be89f0 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_pkt.c,v 1.32 2014/07/10 08:51:14 tedu Exp $ */
+/* $OpenBSD: d1_pkt.c,v 1.33 2014/08/07 19:46:31 miod Exp $ */
 /* 
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.  
@@ -414,10 +414,12 @@ dtls1_process_record(SSL *s)
                }
 
                i = s->method->ssl3_enc->mac(s, md, 0 /* not send */);
-               if (i < 0 || mac == NULL || timingsafe_memcmp(md, mac, (size_t)mac_size) != 0)
+               if (i < 0 || mac == NULL ||
+                   timingsafe_memcmp(md, mac, (size_t)mac_size) != 0)
                        enc_err = -1;
                if (rr->length > SSL3_RT_MAX_COMPRESSED_LENGTH + mac_size)
                        enc_err = -1;
+               OPENSSL_cleanse(&md, sizeof md);
        }
 
        if (enc_err < 0) {
index ee97713..5f8ffa8 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: s23_srvr.c,v 1.32 2014/08/07 04:49:53 deraadt Exp $ */
+/* $OpenBSD: s23_srvr.c,v 1.33 2014/08/07 19:46:31 miod Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -448,11 +448,8 @@ ssl23_get_client_hello(SSL *s)
                }
 
                j = ssl23_read_bytes(s, n + 2);
-               /* We previously read 11 bytes, so if j > 0, we must have
-                * j == n+2 == s->packet_length. We have at least 11 valid
-                * packet bytes. */
-               if (j <= 0)
-                       return (j);
+               if (j != n + 2)
+                       return -1;
 
                ssl3_finish_mac(s, s->packet + 2, s->packet_length - 2);
                if (s->msg_callback)
index 500387e..afcaca3 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: s3_both.c,v 1.26 2014/07/10 08:51:14 tedu Exp $ */
+/* $OpenBSD: s3_both.c,v 1.27 2014/08/07 19:46:31 miod Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -161,7 +161,7 @@ ssl3_send_finished(SSL *s, int a, int b, const char *sender, int slen)
                p = &(d[4]);
 
                i = s->method->ssl3_enc->final_finish_mac(s,
-               sender, slen, s->s3->tmp.finish_md);
+                   sender, slen, s->s3->tmp.finish_md);
                if (i == 0)
                        return 0;
                s->s3->tmp.finish_md_len = i;
@@ -171,15 +171,14 @@ ssl3_send_finished(SSL *s, int a, int b, const char *sender, int slen)
 
                 /* Copy the finished so we can use it for
                    renegotiation checks */
+               OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                if (s->type == SSL_ST_CONNECT) {
-                       OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                        memcpy(s->s3->previous_client_finished,
-                       s->s3->tmp.finish_md, i);
+                           s->s3->tmp.finish_md, i);
                        s->s3->previous_client_finished_len = i;
                } else {
-                       OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                        memcpy(s->s3->previous_server_finished,
-                       s->s3->tmp.finish_md, i);
+                           s->s3->tmp.finish_md, i);
                        s->s3->previous_server_finished_len = i;
                }
 
@@ -216,7 +215,7 @@ ssl3_take_mac(SSL *s)
        }
 
        s->s3->tmp.peer_finish_md_len = s->method->ssl3_enc->final_finish_mac(s,
-       sender, slen, s->s3->tmp.peer_finish_md);
+           sender, slen, s->s3->tmp.peer_finish_md);
 }
 #endif
 
@@ -250,7 +249,7 @@ ssl3_get_finished(SSL *s, int a, int b)
        p = (unsigned char *)s->init_msg;
        i = s->s3->tmp.peer_finish_md_len;
 
-       if (i != n) {
+       if (i != n || i > EVP_MAX_MD_SIZE) {
                al = SSL_AD_DECODE_ERROR;
                SSLerr(SSL_F_SSL3_GET_FINISHED, SSL_R_BAD_DIGEST_LENGTH);
                goto f_err;
@@ -265,14 +264,12 @@ ssl3_get_finished(SSL *s, int a, int b)
         /* Copy the finished so we can use it for
            renegotiation checks */
        if (s->type == SSL_ST_ACCEPT) {
-               OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                memcpy(s->s3->previous_client_finished,
-               s->s3->tmp.peer_finish_md, i);
+                   s->s3->tmp.peer_finish_md, i);
                s->s3->previous_client_finished_len = i;
        } else {
-               OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                memcpy(s->s3->previous_server_finished,
-               s->s3->tmp.peer_finish_md, i);
+                   s->s3->tmp.peer_finish_md, i);
                s->s3->previous_server_finished_len = i;
        }
 
index d9fedfb..913a256 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: s3_enc.c,v 1.52 2014/07/10 08:51:14 tedu Exp $ */
+/* $OpenBSD: s3_enc.c,v 1.53 2014/08/07 19:46:31 miod Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -607,7 +607,7 @@ ssl3_handshake_mac(SSL *s, int md_nid, const char *sender, int len,
        if (!EVP_MD_CTX_copy_ex(&ctx, d))
                return 0;
        n = EVP_MD_CTX_size(&ctx);
-       if (n < 0)
+       if (n <= 0)
                return 0;
 
        npad = (48 / n) * n;
@@ -655,7 +655,7 @@ n_ssl3_mac(SSL *ssl, unsigned char *md, int send)
        }
 
        t = EVP_MD_CTX_size(hash);
-       if (t < 0)
+       if (t <= 0)
                return -1;
        md_size = t;
        npad = (48 / md_size) * md_size;
index 8a40b75..aa091f5 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: s3_lib.c,v 1.71 2014/07/13 16:03:10 beck Exp $ */
+/* $OpenBSD: s3_lib.c,v 1.72 2014/08/07 19:46:31 miod Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -2333,7 +2333,7 @@ ssl3_new(SSL *s)
        SSL3_STATE      *s3;
 
        if ((s3 = calloc(1, sizeof *s3)) == NULL)
-               goto err;
+               return 0;
        memset(s3->rrec.seq_num, 0, sizeof(s3->rrec.seq_num));
        memset(s3->wrec.seq_num, 0, sizeof(s3->wrec.seq_num));
 
@@ -2341,8 +2341,6 @@ ssl3_new(SSL *s)
 
        s->method->ssl_clear(s);
        return (1);
-err:
-       return (0);
 }
 
 void
index e4b5469..bec8328 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: t1_enc.c,v 1.67 2014/07/10 10:09:54 jsing Exp $ */
+/* $OpenBSD: t1_enc.c,v 1.68 2014/08/07 19:46:31 miod Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -319,7 +319,7 @@ tls1_aead_ctx_init(SSL_AEAD_CTX **aead_ctx)
 
 static int
 tls1_change_cipher_state_aead(SSL *s, char is_read, const unsigned char *key,
-    unsigned key_len, const unsigned char *iv, unsigned iv_len)
+    unsigned int key_len, const unsigned char *iv, unsigned int iv_len)
 {
        const EVP_AEAD *aead = s->s3->tmp.new_aead;
        SSL_AEAD_CTX *aead_ctx;
@@ -856,6 +856,7 @@ tls1_enc(SSL *s, int send)
                                rec->length += pad;
                        }
                } else if ((bs != 1) && send) {
+                       /* XXX divide by zero if bs == 0 (should not happen) */
                        i = bs - ((int)l % bs);
 
                        /* Add weird padding of upto 256 bytes */
@@ -1120,7 +1121,7 @@ tls1_export_keying_material(SSL *s, unsigned char *out, size_t olen,
                currentvalpos++;
                val[currentvalpos] = contextlen & 0xff;
                currentvalpos++;
-               if ((contextlen > 0) || (context != NULL)) {
+               if (contextlen != 0 && context != NULL) {
                        memcpy(val + currentvalpos, context, contextlen);
                }
        }
index e4b5469..bec8328 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: t1_enc.c,v 1.67 2014/07/10 10:09:54 jsing Exp $ */
+/* $OpenBSD: t1_enc.c,v 1.68 2014/08/07 19:46:31 miod Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -319,7 +319,7 @@ tls1_aead_ctx_init(SSL_AEAD_CTX **aead_ctx)
 
 static int
 tls1_change_cipher_state_aead(SSL *s, char is_read, const unsigned char *key,
-    unsigned key_len, const unsigned char *iv, unsigned iv_len)
+    unsigned int key_len, const unsigned char *iv, unsigned int iv_len)
 {
        const EVP_AEAD *aead = s->s3->tmp.new_aead;
        SSL_AEAD_CTX *aead_ctx;
@@ -856,6 +856,7 @@ tls1_enc(SSL *s, int send)
                                rec->length += pad;
                        }
                } else if ((bs != 1) && send) {
+                       /* XXX divide by zero if bs == 0 (should not happen) */
                        i = bs - ((int)l % bs);
 
                        /* Add weird padding of upto 256 bytes */
@@ -1120,7 +1121,7 @@ tls1_export_keying_material(SSL *s, unsigned char *out, size_t olen,
                currentvalpos++;
                val[currentvalpos] = contextlen & 0xff;
                currentvalpos++;
-               if ((contextlen > 0) || (context != NULL)) {
+               if (contextlen != 0 && context != NULL) {
                        memcpy(val + currentvalpos, context, contextlen);
                }
        }