From 455b65dfab0de51c9f67b3c909311770f2b3f801 Mon Sep 17 00:00:00 2001 From: Emilia Kasper Date: Thu, 4 Sep 2014 13:04:42 +0200 Subject: [PATCH] RT3067: simplify patch (Original commit adb46dbc6dd7347750df2468c93e8c34bcb93a4b) Use the new constant-time methods consistently in s3_srvr.c Reviewed-by: Kurt Roeckx --- apps/openssl-vms.cnf | 4 +-- crypto/constant_time_locl.h | 15 ++++++++ crypto/constant_time_test.c | 39 ++++++++++++++++++++ ssl/Makefile | 45 +++++++++++------------ ssl/s3_srvr.c | 72 +++++++++++-------------------------- 5 files changed, 99 insertions(+), 76 deletions(-) diff --git a/apps/openssl-vms.cnf b/apps/openssl-vms.cnf index abea6d8255..c0ded4a5f1 100644 --- a/apps/openssl-vms.cnf +++ b/apps/openssl-vms.cnf @@ -44,7 +44,7 @@ certs = $dir.certs] # Where the issued certs are kept crl_dir = $dir.crl] # Where the issued crl are kept database = $dir]index.txt # database index file. #unique_subject = no # Set to 'no' to allow creation of - # several ctificates with same subject. + # several certs with same subject. new_certs_dir = $dir.newcerts] # default place for new certs. certificate = $dir]cacert.pem # The CA certificate @@ -103,7 +103,7 @@ emailAddress = optional #################################################################### [ req ] -default_bits = 1024 +default_bits = 2048 default_keyfile = privkey.pem distinguished_name = req_distinguished_name attributes = req_attributes diff --git a/crypto/constant_time_locl.h b/crypto/constant_time_locl.h index ccf7b62f5f..c0483939fe 100644 --- a/crypto/constant_time_locl.h +++ b/crypto/constant_time_locl.h @@ -106,6 +106,11 @@ static inline unsigned char constant_time_is_zero_8(unsigned int a); static inline unsigned int constant_time_eq(unsigned int a, unsigned int b); /* Convenience method for getting an 8-bit mask. */ static inline unsigned char constant_time_eq_8(unsigned int a, unsigned int b); +/* Signed integers. */ +static inline unsigned int constant_time_eq_int(int a, int b); +/* Convenience method for getting an 8-bit mask. */ +static inline unsigned char constant_time_eq_int_8(int a, int b); + /* * Returns (mask & a) | (~mask & b). @@ -177,6 +182,16 @@ static inline unsigned char constant_time_eq_8(unsigned int a, unsigned int b) return (unsigned char)(constant_time_eq(a, b)); } +static inline unsigned int constant_time_eq_int(int a, int b) + { + return constant_time_eq((unsigned)(a), (unsigned)(b)); + } + +static inline unsigned char constant_time_eq_int_8(int a, int b) + { + return constant_time_eq_8((unsigned)(a), (unsigned)(b)); + } + static inline unsigned int constant_time_select(unsigned int mask, unsigned int a, unsigned int b) { diff --git a/crypto/constant_time_test.c b/crypto/constant_time_test.c index 0e51892af5..1b4b18d199 100644 --- a/crypto/constant_time_test.c +++ b/crypto/constant_time_test.c @@ -196,6 +196,45 @@ static int test_select_int(int a, int b) return 0; } +static int test_eq_int(int a, int b) + { + unsigned int equal = constant_time_eq_int(a, b); + if (a == b && equal != CONSTTIME_TRUE) + { + fprintf(stderr, "Test failed for constant_time_select(%d, %d): " + "expected %du(TRUE), got %du\n", + a, b, CONSTTIME_TRUE, equal); + return 1; + } + else if (a != b && equal != CONSTTIME_FALSE) + { + fprintf(stderr, "Test failed for constant_time_select(%d, %d): " + "expected %du(FALSE), got %du\n", + a, b, CONSTTIME_FALSE, equal); + return 1; + } + return 0; + } + +static int test_eq_int_8(int a, int b) + { + unsigned char equal = constant_time_eq_int_8(a, b); + if (a == b && equal != CONSTTIME_TRUE_8) + { + fprintf(stderr, "Test failed for constant_time_select(%d, %d): " + "expected %u(TRUE), got %u\n", + a, b, CONSTTIME_TRUE_8, equal); + return 1; + } + else if (a != b && equal != CONSTTIME_FALSE_8) + { + fprintf(stderr, "Test failed for constant_time_select(%d, %d): " + "expected %u(FALSE), got %u\n", + a, b, CONSTTIME_FALSE_8, equal); + return 1; + } + return 0; + } static unsigned int test_values[] = {0, 1, 1024, 12345, 32000, UINT_MAX/2-1, UINT_MAX/2, UINT_MAX/2+1, UINT_MAX-1, diff --git a/ssl/Makefile b/ssl/Makefile index f39b88cc67..262d3081a0 100644 --- a/ssl/Makefile +++ b/ssl/Makefile @@ -671,28 +671,29 @@ s3_pkt.o: ../include/openssl/ssl23.h ../include/openssl/ssl3.h s3_pkt.o: ../include/openssl/stack.h ../include/openssl/symhacks.h s3_pkt.o: ../include/openssl/tls1.h ../include/openssl/x509.h s3_pkt.o: ../include/openssl/x509_vfy.h s3_pkt.c ssl_locl.h -s3_srvr.o: ../e_os.h ../include/openssl/asn1.h ../include/openssl/bio.h -s3_srvr.o: ../include/openssl/bn.h ../include/openssl/buffer.h -s3_srvr.o: ../include/openssl/comp.h ../include/openssl/crypto.h -s3_srvr.o: ../include/openssl/dh.h ../include/openssl/dsa.h -s3_srvr.o: ../include/openssl/dtls1.h ../include/openssl/e_os2.h -s3_srvr.o: ../include/openssl/ec.h ../include/openssl/ecdh.h -s3_srvr.o: ../include/openssl/ecdsa.h ../include/openssl/err.h -s3_srvr.o: ../include/openssl/evp.h ../include/openssl/hmac.h -s3_srvr.o: ../include/openssl/krb5_asn.h ../include/openssl/kssl.h -s3_srvr.o: ../include/openssl/lhash.h ../include/openssl/md5.h -s3_srvr.o: ../include/openssl/obj_mac.h ../include/openssl/objects.h -s3_srvr.o: ../include/openssl/opensslconf.h ../include/openssl/opensslv.h -s3_srvr.o: ../include/openssl/ossl_typ.h ../include/openssl/pem.h -s3_srvr.o: ../include/openssl/pem2.h ../include/openssl/pkcs7.h -s3_srvr.o: ../include/openssl/pqueue.h ../include/openssl/rand.h -s3_srvr.o: ../include/openssl/rsa.h ../include/openssl/safestack.h -s3_srvr.o: ../include/openssl/sha.h ../include/openssl/srtp.h -s3_srvr.o: ../include/openssl/ssl.h ../include/openssl/ssl2.h -s3_srvr.o: ../include/openssl/ssl23.h ../include/openssl/ssl3.h -s3_srvr.o: ../include/openssl/stack.h ../include/openssl/symhacks.h -s3_srvr.o: ../include/openssl/tls1.h ../include/openssl/x509.h -s3_srvr.o: ../include/openssl/x509_vfy.h kssl_lcl.h s3_srvr.c ssl_locl.h +s3_srvr.o: ../crypto/constant_time_locl.h ../e_os.h ../include/openssl/asn1.h +s3_srvr.o: ../include/openssl/bio.h ../include/openssl/bn.h +s3_srvr.o: ../include/openssl/buffer.h ../include/openssl/comp.h +s3_srvr.o: ../include/openssl/crypto.h ../include/openssl/dh.h +s3_srvr.o: ../include/openssl/dsa.h ../include/openssl/dtls1.h +s3_srvr.o: ../include/openssl/e_os2.h ../include/openssl/ec.h +s3_srvr.o: ../include/openssl/ecdh.h ../include/openssl/ecdsa.h +s3_srvr.o: ../include/openssl/err.h ../include/openssl/evp.h +s3_srvr.o: ../include/openssl/hmac.h ../include/openssl/krb5_asn.h +s3_srvr.o: ../include/openssl/kssl.h ../include/openssl/lhash.h +s3_srvr.o: ../include/openssl/md5.h ../include/openssl/obj_mac.h +s3_srvr.o: ../include/openssl/objects.h ../include/openssl/opensslconf.h +s3_srvr.o: ../include/openssl/opensslv.h ../include/openssl/ossl_typ.h +s3_srvr.o: ../include/openssl/pem.h ../include/openssl/pem2.h +s3_srvr.o: ../include/openssl/pkcs7.h ../include/openssl/pqueue.h +s3_srvr.o: ../include/openssl/rand.h ../include/openssl/rsa.h +s3_srvr.o: ../include/openssl/safestack.h ../include/openssl/sha.h +s3_srvr.o: ../include/openssl/srtp.h ../include/openssl/ssl.h +s3_srvr.o: ../include/openssl/ssl2.h ../include/openssl/ssl23.h +s3_srvr.o: ../include/openssl/ssl3.h ../include/openssl/stack.h +s3_srvr.o: ../include/openssl/symhacks.h ../include/openssl/tls1.h +s3_srvr.o: ../include/openssl/x509.h ../include/openssl/x509_vfy.h kssl_lcl.h +s3_srvr.o: s3_srvr.c ssl_locl.h ssl_algs.o: ../e_os.h ../include/openssl/asn1.h ../include/openssl/bio.h ssl_algs.o: ../include/openssl/buffer.h ../include/openssl/comp.h ssl_algs.o: ../include/openssl/crypto.h ../include/openssl/dsa.h diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 440fc13e4f..bef055ac05 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -154,6 +154,7 @@ #include #include "ssl_locl.h" #include "kssl_lcl.h" +#include "../crypto/constant_time_locl.h" #include #include #include @@ -2223,8 +2224,8 @@ int ssl3_get_client_key_exchange(SSL *s) if (alg_k & SSL_kRSA) { unsigned char rand_premaster_secret[SSL_MAX_MASTER_KEY_LENGTH]; - int decrypt_len, decrypt_good_mask; - unsigned char version_good; + int decrypt_len; + unsigned char decrypt_good, version_good; /* FIX THIS UP EAY EAY EAY EAY */ if (s->s3->tmp.use_rsa_tmp) @@ -2288,18 +2289,18 @@ int ssl3_get_client_key_exchange(SSL *s) ERR_clear_error(); /* decrypt_len should be SSL_MAX_MASTER_KEY_LENGTH. - * decrypt_good_mask will be zero if so and non-zero otherwise. */ - decrypt_good_mask = decrypt_len ^ SSL_MAX_MASTER_KEY_LENGTH; + * decrypt_good will be 0xff if so and zero otherwise. */ + decrypt_good = constant_time_eq_int_8(decrypt_len, SSL_MAX_MASTER_KEY_LENGTH); /* If the version in the decrypted pre-master secret is correct - * then version_good will be zero. The Klima-Pokorny-Rosa - * extension of Bleichenbacher's attack + * then version_good will be 0xff, otherwise it'll be zero. + * The Klima-Pokorny-Rosa extension of Bleichenbacher's attack * (http://eprint.iacr.org/2003/052/) exploits the version * number check as a "bad version oracle". Thus version checks * are done in constant time and are treated like any other * decryption error. */ - version_good = p[0] ^ (s->client_version>>8); - version_good |= p[1] ^ (s->client_version&0xff); + version_good = constant_time_eq_8(p[0], (unsigned)(s->client_version>>8)); + version_good &= constant_time_eq_8(p[1], (unsigned)(s->client_version&0xff)); /* The premaster secret must contain the same version number as * the ClientHello to detect version rollback attacks @@ -2310,55 +2311,22 @@ int ssl3_get_client_key_exchange(SSL *s) * SSL_OP_TLS_ROLLBACK_BUG is set, tolerate such clients. */ if (s->options & SSL_OP_TLS_ROLLBACK_BUG) { - unsigned char workaround_mask = version_good; - unsigned char workaround; - - /* workaround_mask will be 0xff if version_good is - * non-zero (i.e. the version match failed). Otherwise - * it'll be 0x00. */ - workaround_mask |= workaround_mask >> 4; - workaround_mask |= workaround_mask >> 2; - workaround_mask |= workaround_mask >> 1; - workaround_mask = ~((workaround_mask & 1) - 1); - - workaround = p[0] ^ (s->version>>8); - workaround |= p[1] ^ (s->version&0xff); - - /* If workaround_mask is 0xff (i.e. there was a version - * mismatch) then we copy the value of workaround over - * version_good. */ - version_good = (workaround & workaround_mask) | - (version_good & ~workaround_mask); - } - - /* If any bits in version_good are set then they'll poision - * decrypt_good_mask and cause rand_premaster_secret to be - * used. */ - decrypt_good_mask |= version_good; - - /* decrypt_good_mask will be zero iff decrypt_len == - * SSL_MAX_MASTER_KEY_LENGTH and the version check passed. We - * fold the bottom 32 bits of it with an OR so that the LSB - * will be zero iff everything is good. This assumes that we'll - * never decrypt a value > 2**31 bytes, which seems safe. */ - decrypt_good_mask |= decrypt_good_mask >> 16; - decrypt_good_mask |= decrypt_good_mask >> 8; - decrypt_good_mask |= decrypt_good_mask >> 4; - decrypt_good_mask |= decrypt_good_mask >> 2; - decrypt_good_mask |= decrypt_good_mask >> 1; - /* Now select only the LSB and subtract one. If decrypt_len == - * SSL_MAX_MASTER_KEY_LENGTH and the version check passed then - * decrypt_good_mask will be all ones. Otherwise it'll be all - * zeros. */ - decrypt_good_mask &= 1; - decrypt_good_mask--; + unsigned char workaround_good; + workaround_good = constant_time_eq_8(p[0], (unsigned)(s->version>>8)); + workaround_good &= constant_time_eq_8(p[1], (unsigned)(s->version&0xff)); + version_good |= workaround_good; + } + + /* Both decryption and version must be good for decrypt_good + * to remain non-zero (0xff). */ + decrypt_good &= version_good; /* Now copy rand_premaster_secret over p using * decrypt_good_mask. */ for (i = 0; i < (int) sizeof(rand_premaster_secret); i++) { - p[i] = (p[i] & decrypt_good_mask) | - (rand_premaster_secret[i] & ~decrypt_good_mask); + p[i] = constant_time_select_8(decrypt_good, p[i], + rand_premaster_secret[i]); } s->session->master_key_length= -- 2.34.1