From 6af080acaf57c74e3cd96642f2900fa602407d10 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Wed, 16 Apr 2014 07:21:26 -0400 Subject: [PATCH 1/5] Unit/regression test for TLS heartbeats. Regression test against CVE-2014-0160 (Heartbleed). More info: http://mike-bland.com/tags/heartbleed.html --- ssl/Makefile | 2 +- ssl/heartbeat_test.c | 424 +++++++++++++++++++++++++++++++++++++++++++ test/Makefile | 36 +++- 3 files changed, 457 insertions(+), 5 deletions(-) create mode 100644 ssl/heartbeat_test.c diff --git a/ssl/Makefile b/ssl/Makefile index 0bc71af90..1d74e9acc 100644 --- a/ssl/Makefile +++ b/ssl/Makefile @@ -15,7 +15,7 @@ KRB5_INCLUDES= CFLAGS= $(INCLUDES) $(CFLAG) GENERAL=Makefile README ssl-lib.com install.com -TEST=ssltest.c +TEST=ssltest.c heartbeat_test.c APPS= LIB=$(TOP)/libssl.a diff --git a/ssl/heartbeat_test.c b/ssl/heartbeat_test.c new file mode 100644 index 000000000..eb08ee6b0 --- /dev/null +++ b/ssl/heartbeat_test.c @@ -0,0 +1,424 @@ +/* test/heartbeat_test.c */ +/* + * Unit test for TLS heartbeats. + * + * Acts as a regression test against the Heartbleed bug (CVE-2014-0160). + * + * Author: Mike Bland (mbland@acm.org, http://mike-bland.com/) + * Date: 2014-04-12 + * License: Creative Commons Attribution 4.0 International (CC By 4.0) + * http://creativecommons.org/licenses/by/4.0/deed.en_US + * + * OUTPUT + * ------ + * The program returns zero on success. It will print a message with a count + * of the number of failed tests and return nonzero if any tests fail. + * + * It will print the contents of the request and response buffers for each + * failing test. In a "fixed" version, all the tests should pass and there + * should be no output. + * + * In a "bleeding" version, you'll see: + * + * test_dtls1_heartbleed failed: + * expected payload len: 0 + * received: 1024 + * sent 26 characters + * "HEARTBLEED " + * received 1024 characters + * "HEARTBLEED \xde\xad\xbe\xef..." + * ** test_dtls1_heartbleed failed ** + * + * The contents of the returned buffer in the failing test will depend on the + * contents of memory on your machine. + * + * MORE INFORMATION + * ---------------- + * http://mike-bland.com/2014/04/12/heartbleed.html + * http://mike-bland.com/tags/heartbleed.html + */ + +#include "../ssl/ssl_locl.h" +#include +#include +#include +#include + +/* As per https://tools.ietf.org/html/rfc6520#section-4 */ +static const int MIN_PADDING_SIZE = 16; + +/* Maximum number of payload characters to print as test output */ +static const int MAX_PRINTABLE_CHARACTERS = 1024; + +typedef struct heartbeat_test_fixture + { + SSL_CTX *ctx; + SSL *s; + const char* test_case_name; + int (*process_heartbeat)(SSL* s); + unsigned char* payload; + int sent_payload_len; + int expected_return_value; + int return_payload_offset; + int expected_payload_len; + const char* expected_return_payload; + } HEARTBEAT_TEST_FIXTURE; + +static HEARTBEAT_TEST_FIXTURE set_up(const char* const test_case_name, + const SSL_METHOD* meth) + { + HEARTBEAT_TEST_FIXTURE fixture; + memset(&fixture, 0, sizeof(fixture)); + fixture.test_case_name = test_case_name; + + fixture.ctx = SSL_CTX_new(meth); + if (!fixture.ctx) + { + fprintf(stderr, "Failed to allocate SSL_CTX for test: %s\n", + test_case_name); + goto fail; + } + + fixture.s = SSL_new(fixture.ctx); + if (!fixture.s) + { + fprintf(stderr, "Failed to allocate SSL for test: %s\n", test_case_name); + goto fail; + } + + ssl_init_wbio_buffer(fixture.s, 1); + ssl3_setup_buffers(fixture.s); + + fail: + if (!fixture.s) + { + ERR_print_errors_fp(stderr); + exit(EXIT_FAILURE); + } + return fixture; + } + +static HEARTBEAT_TEST_FIXTURE set_up_dtls(const char* const test_case_name) + { + HEARTBEAT_TEST_FIXTURE fixture = set_up(test_case_name, + DTLSv1_server_method()); + fixture.process_heartbeat = dtls1_process_heartbeat; + + /* As per dtls1_get_record(), skipping the following from the beginning of + * the returned heartbeat message: + * type-1 byte; version-2 bytes; sequence number-8 bytes; length-2 bytes + * + * And then skipping the 1-byte type encoded by process_heartbeat for + * a total of 14 bytes, at which point we can grab the length and the + * payload we seek. + */ + fixture.return_payload_offset = 14; + return fixture; + } + +/* Needed by ssl3_write_bytes() */ +static int dummy_handshake(SSL* s) + { + return 1; + } + +static HEARTBEAT_TEST_FIXTURE set_up_tls(const char* const test_case_name) + { + HEARTBEAT_TEST_FIXTURE fixture = set_up(test_case_name, + TLSv1_server_method()); + fixture.process_heartbeat = tls1_process_heartbeat; + fixture.s->handshake_func = dummy_handshake; + + /* As per do_ssl3_write(), skipping the following from the beginning of + * the returned heartbeat message: + * type-1 byte; version-2 bytes; length-2 bytes + * + * And then skipping the 1-byte type encoded by process_heartbeat for + * a total of 6 bytes, at which point we can grab the length and the payload + * we seek. + */ + fixture.return_payload_offset = 6; + return fixture; + } + +static void tear_down(HEARTBEAT_TEST_FIXTURE fixture) + { + ERR_print_errors_fp(stderr); + memset(fixture.s, 0, sizeof(*fixture.s)); + SSL_free(fixture.s); + memset(fixture.ctx, 0, sizeof(*fixture.ctx)); + SSL_CTX_free(fixture.ctx); + } + +static void print_payload(const char* const prefix, + const unsigned char *payload, const int n) + { + const int end = n < MAX_PRINTABLE_CHARACTERS ? n : MAX_PRINTABLE_CHARACTERS; + printf("%s %d character%s", prefix, n, n == 1 ? "" : "s"); + if (end != n) printf(" (first %d shown)", end); + printf("\n \""); + + int i = 0; + for (; i != end; ++i) + { + const unsigned char c = payload[i]; + if (isprint(c)) fputc(c, stdout); + else printf("\\x%02x", c); + } + printf("\"\n"); +} + +static int execute_heartbeat(HEARTBEAT_TEST_FIXTURE fixture) + { + int result = 0; + SSL* s = fixture.s; + unsigned char *payload = fixture.payload; + unsigned char sent_buf[MAX_PRINTABLE_CHARACTERS + 1]; + + s->s3->rrec.data = payload; + s->s3->rrec.length = strlen((const char*)payload); + *payload++ = TLS1_HB_REQUEST; + s2n(fixture.sent_payload_len, payload); + + /* Make a local copy of the request, since it gets overwritten at some + * point */ + memcpy((char *)sent_buf, (const char*)payload, sizeof(sent_buf)); + + int return_value = fixture.process_heartbeat(s); + + if (return_value != fixture.expected_return_value) + { + printf("%s failed: expected return value %d, received %d\n", + fixture.test_case_name, fixture.expected_return_value, + return_value); + result = 1; + } + + /* If there is any byte alignment, it will be stored in wbuf.offset. */ + unsigned const char *p = &(s->s3->wbuf.buf[ + fixture.return_payload_offset + s->s3->wbuf.offset]); + int actual_payload_len = 0; + n2s(p, actual_payload_len); + + if (actual_payload_len != fixture.expected_payload_len) + { + printf("%s failed:\n expected payload len: %d\n received: %d\n", + fixture.test_case_name, fixture.expected_payload_len, + actual_payload_len); + print_payload("sent", sent_buf, strlen((const char*)sent_buf)); + print_payload("received", p, actual_payload_len); + result = 1; + } + else + { + char* actual_payload = strndup((const char*)p, actual_payload_len); + if (strcmp(actual_payload, fixture.expected_return_payload) != 0) + { + printf("%s failed:\n expected payload: \"%s\"\n received: \"%s\"\n", + fixture.test_case_name, fixture.expected_return_payload, + actual_payload); + result = 1; + } + free(actual_payload); + } + + if (result != 0) + { + printf("** %s failed **\n--------\n", fixture.test_case_name); + } + return result; + } + +static int honest_payload_size(unsigned char payload_buf[]) + { + /* Omit three-byte pad at the beginning for type and payload length */ + return strlen((const char*)&payload_buf[3]) - MIN_PADDING_SIZE; + } + +#define SETUP_HEARTBEAT_TEST_FIXTURE(type)\ + HEARTBEAT_TEST_FIXTURE fixture = set_up_##type(__func__);\ + int result = 0 + +#define EXECUTE_HEARTBEAT_TEST()\ + if (execute_heartbeat(fixture) != 0) result = 1;\ + tear_down(fixture);\ + return result + +static int test_dtls1_not_bleeding() + { + SETUP_HEARTBEAT_TEST_FIXTURE(dtls); + /* Three-byte pad at the beginning for type and payload length */ + unsigned char payload_buf[] = " Not bleeding, sixteen spaces of padding" + " "; + const int payload_buf_len = honest_payload_size(payload_buf); + + fixture.payload = &payload_buf[0]; + fixture.sent_payload_len = payload_buf_len; + fixture.expected_return_value = 0; + fixture.expected_payload_len = payload_buf_len; + fixture.expected_return_payload = "Not bleeding, sixteen spaces of padding"; + EXECUTE_HEARTBEAT_TEST(); + } + +static int test_dtls1_not_bleeding_empty_payload() + { + SETUP_HEARTBEAT_TEST_FIXTURE(dtls); + /* Three-byte pad at the beginning for type and payload length, plus a NUL + * at the end */ + unsigned char payload_buf[4 + MIN_PADDING_SIZE]; + memset(payload_buf, ' ', sizeof(payload_buf)); + payload_buf[sizeof(payload_buf) - 1] = '\0'; + const int payload_buf_len = honest_payload_size(payload_buf); + + fixture.payload = &payload_buf[0]; + fixture.sent_payload_len = payload_buf_len; + fixture.expected_return_value = 0; + fixture.expected_payload_len = payload_buf_len; + fixture.expected_return_payload = ""; + EXECUTE_HEARTBEAT_TEST(); + } + +static int test_dtls1_heartbleed() + { + SETUP_HEARTBEAT_TEST_FIXTURE(dtls); + /* Three-byte pad at the beginning for type and payload length */ + unsigned char payload_buf[] = " HEARTBLEED "; + + fixture.payload = &payload_buf[0]; + fixture.sent_payload_len = MAX_PRINTABLE_CHARACTERS; + fixture.expected_return_value = 0; + fixture.expected_payload_len = 0; + fixture.expected_return_payload = ""; + EXECUTE_HEARTBEAT_TEST(); + } + +static int test_dtls1_heartbleed_empty_payload() + { + SETUP_HEARTBEAT_TEST_FIXTURE(dtls); + /* Excluding the NUL at the end, one byte short of type + payload length + + * minimum padding */ + unsigned char payload_buf[MIN_PADDING_SIZE + 3]; + memset(payload_buf, ' ', sizeof(payload_buf)); + payload_buf[sizeof(payload_buf) - 1] = '\0'; + + fixture.payload = &payload_buf[0]; + fixture.sent_payload_len = MAX_PRINTABLE_CHARACTERS; + fixture.expected_return_value = 0; + fixture.expected_payload_len = 0; + fixture.expected_return_payload = ""; + EXECUTE_HEARTBEAT_TEST(); + } + +static int test_dtls1_heartbleed_excessive_plaintext_length() + { + SETUP_HEARTBEAT_TEST_FIXTURE(dtls); + /* Excluding the NUL at the end, one byte in excess of maximum allowed + * heartbeat message length */ + unsigned char payload_buf[SSL3_RT_MAX_PLAIN_LENGTH + 2]; + memset(payload_buf, ' ', sizeof(payload_buf)); + payload_buf[sizeof(payload_buf) - 1] = '\0'; + + fixture.payload = &payload_buf[0]; + fixture.sent_payload_len = honest_payload_size(payload_buf); + fixture.expected_return_value = 0; + fixture.expected_payload_len = 0; + fixture.expected_return_payload = ""; + EXECUTE_HEARTBEAT_TEST(); + } + +static int test_tls1_not_bleeding() + { + SETUP_HEARTBEAT_TEST_FIXTURE(tls); + /* Three-byte pad at the beginning for type and payload length */ + unsigned char payload_buf[] = " Not bleeding, sixteen spaces of padding" + " "; + const int payload_buf_len = honest_payload_size(payload_buf); + + fixture.payload = &payload_buf[0]; + fixture.sent_payload_len = payload_buf_len; + fixture.expected_return_value = 0; + fixture.expected_payload_len = payload_buf_len; + fixture.expected_return_payload = "Not bleeding, sixteen spaces of padding"; + EXECUTE_HEARTBEAT_TEST(); + } + +static int test_tls1_not_bleeding_empty_payload() + { + SETUP_HEARTBEAT_TEST_FIXTURE(tls); + /* Three-byte pad at the beginning for type and payload length, plus a NUL + * at the end */ + unsigned char payload_buf[4 + MIN_PADDING_SIZE]; + memset(payload_buf, ' ', sizeof(payload_buf)); + payload_buf[sizeof(payload_buf) - 1] = '\0'; + const int payload_buf_len = honest_payload_size(payload_buf); + + fixture.payload = &payload_buf[0]; + fixture.sent_payload_len = payload_buf_len; + fixture.expected_return_value = 0; + fixture.expected_payload_len = payload_buf_len; + fixture.expected_return_payload = ""; + EXECUTE_HEARTBEAT_TEST(); + } + +static int test_tls1_heartbleed() + { + SETUP_HEARTBEAT_TEST_FIXTURE(tls); + /* Three-byte pad at the beginning for type and payload length */ + unsigned char payload_buf[] = " HEARTBLEED "; + + fixture.payload = &payload_buf[0]; + fixture.sent_payload_len = MAX_PRINTABLE_CHARACTERS; + fixture.expected_return_value = 0; + fixture.expected_payload_len = 0; + fixture.expected_return_payload = ""; + EXECUTE_HEARTBEAT_TEST(); + } + +static int test_tls1_heartbleed_empty_payload() + { + SETUP_HEARTBEAT_TEST_FIXTURE(tls); + /* Excluding the NUL at the end, one byte short of type + payload length + + * minimum padding */ + unsigned char payload_buf[MIN_PADDING_SIZE + 3]; + memset(payload_buf, ' ', sizeof(payload_buf)); + payload_buf[sizeof(payload_buf) - 1] = '\0'; + + fixture.payload = &payload_buf[0]; + fixture.sent_payload_len = MAX_PRINTABLE_CHARACTERS; + fixture.expected_return_value = 0; + fixture.expected_payload_len = 0; + fixture.expected_return_payload = ""; + EXECUTE_HEARTBEAT_TEST(); + } + +#undef EXECUTE_HEARTBEAT_TEST +#undef SETUP_HEARTBEAT_TEST_FIXTURE + +int main(int argc, char *argv[]) + { + SSL_library_init(); + SSL_load_error_strings(); + + const int num_failed = test_dtls1_not_bleeding() + + test_dtls1_not_bleeding_empty_payload() + + test_dtls1_heartbleed() + + test_dtls1_heartbleed_empty_payload() + + /* The following test causes an assertion failure at + * ssl/d1_pkt.c:dtls1_write_bytes() in versions prior to 1.0.1g: */ + (OPENSSL_VERSION_NUMBER >= 0x1000107fL ? + test_dtls1_heartbleed_excessive_plaintext_length() : 0) + + test_tls1_not_bleeding() + + test_tls1_not_bleeding_empty_payload() + + test_tls1_heartbleed() + + test_tls1_heartbleed_empty_payload() + + 0; + + ERR_print_errors_fp(stderr); + + if (num_failed != 0) + { + printf("%d test%s failed\n", num_failed, num_failed != 1 ? "s" : ""); + return EXIT_FAILURE; + } + return EXIT_SUCCESS; + } diff --git a/test/Makefile b/test/Makefile index d4b66992b..e016d71a9 100644 --- a/test/Makefile +++ b/test/Makefile @@ -85,6 +85,7 @@ FIPS_ECDSAVS= fips_ecdsavs FIPS_TEST_SUITE=fips_test_suite FIPS_CMACTEST= fips_cmactest FIPS_ALGVS= fips_algvs +HEARTBEATTEST = heartbeat_test TESTS= alltests @@ -98,7 +99,7 @@ EXE= $(BNTEST)$(EXE_EXT) $(ECTEST)$(EXE_EXT) $(ECDSATEST)$(EXE_EXT) $(ECDHTEST) $(BFTEST)$(EXE_EXT) $(CASTTEST)$(EXE_EXT) $(SSLTEST)$(EXE_EXT) \ $(EXPTEST)$(EXE_EXT) $(DSATEST)$(EXE_EXT) $(RSATEST)$(EXE_EXT) \ $(EVPTEST)$(EXE_EXT) $(IGETEST)$(EXE_EXT) $(JPAKETEST)$(EXE_EXT) $(SRPTEST)$(EXE_EXT) \ - $(V3NAMETEST)$(EXE_EXT) + $(V3NAMETEST)$(EXE_EXT) $(HEARTBEATTEST)$(EXE_EXT) FIPSEXE=$(FIPS_SHATEST)$(EXE_EXT) $(FIPS_DESTEST)$(EXE_EXT) \ $(FIPS_RANDTEST)$(EXE_EXT) $(FIPS_AESTEST)$(EXE_EXT) \ @@ -127,7 +128,7 @@ OBJ= $(BNTEST).o $(ECTEST).o $(ECDSATEST).o $(ECDHTEST).o $(IDEATEST).o \ $(FIPS_TEST_SUITE).o $(FIPS_DHVS).o $(FIPS_ECDSAVS).o \ $(FIPS_ECDHVS).o $(FIPS_CMACTEST).o $(FIPS_ALGVS).o \ $(EVPTEST).o $(IGETEST).o $(JPAKETEST).o $(V3NAMETEST).o \ - $(GOST2814789TEST).o + $(GOST2814789TEST).o $(HEARTBEATTEST).o SRC= $(BNTEST).c $(ECTEST).c $(ECDSATEST).c $(ECDHTEST).c $(IDEATEST).c \ $(MD2TEST).c $(MD4TEST).c $(MD5TEST).c \ $(HMACTEST).c $(WPTEST).c \ @@ -142,7 +143,7 @@ SRC= $(BNTEST).c $(ECTEST).c $(ECDSATEST).c $(ECDHTEST).c $(IDEATEST).c \ $(FIPS_TEST_SUITE).c $(FIPS_DHVS).c $(FIPS_ECDSAVS).c \ $(FIPS_ECDHVS).c $(FIPS_CMACTEST).c $(FIPS_ALGVS).c \ $(EVPTEST).c $(IGETEST).c $(JPAKETEST).c $(V3NAMETEST).c \ - $(GOST2814789TEST).c + $(GOST2814789TEST).c $(HEARTBEATTEST).c EXHEADER= HEADER= $(EXHEADER) @@ -190,7 +191,7 @@ alltests: \ test_gen test_req test_pkcs7 test_verify test_dh test_dsa \ test_ss test_ca test_engine test_evp test_ssl test_tsa test_ige \ test_jpake test_srp test_cms test_v3name test_ocsp \ - test_gost2814789 + test_gost2814789 test_heartbeat test_evp: $(EVPTEST)$(EXE_EXT) evptests.txt ../util/shlib_wrap.sh ./$(EVPTEST) evptests.txt @@ -377,6 +378,9 @@ test_ocsp: ../apps/openssl$(EXE_EXT) tocsp @echo "Test OCSP" @sh ./tocsp +test_heartbeat: $(HEARTBEATTEST)$(EXE_EXT) + ./$< + lint: lint -DLINT $(INCLUDES) $(SRC)>fluff @@ -605,6 +609,9 @@ $(SRPTEST)$(EXE_EXT): $(SRPTEST).o $(DLIBCRYPTO) $(V3NAMETEST)$(EXE_EXT): $(V3NAMETEST).o $(DLIBCRYPTO) @target=$(V3NAMETEST); $(BUILD_CMD) +$(HEARTBEATTEST)$(EXE_EXT): $(HEARTBEATTEST).o $(DLIBCRYPTO) + @target=$(HEARTBEATTEST); $(BUILD_CMD) + #$(AESTEST).o: $(AESTEST).c # $(CC) -c $(CFLAGS) -DINTERMEDIATE_VALUE_KAT -DTRACE_KAT_MCT $(AESTEST).c @@ -855,6 +862,27 @@ gost2814789t.o: ../include/openssl/pkcs7.h ../include/openssl/safestack.h gost2814789t.o: ../include/openssl/sha.h ../include/openssl/stack.h gost2814789t.o: ../include/openssl/symhacks.h ../include/openssl/x509.h gost2814789t.o: ../include/openssl/x509_vfy.h gost2814789t.c +heartbeat_test.o: ../e_os.h ../include/openssl/asn1.h ../include/openssl/bio.h +heartbeat_test.o: ../include/openssl/buffer.h ../include/openssl/comp.h +heartbeat_test.o: ../include/openssl/crypto.h ../include/openssl/dsa.h +heartbeat_test.o: ../include/openssl/dtls1.h ../include/openssl/e_os2.h +heartbeat_test.o: ../include/openssl/ec.h ../include/openssl/ecdh.h +heartbeat_test.o: ../include/openssl/ecdsa.h ../include/openssl/err.h +heartbeat_test.o: ../include/openssl/evp.h ../include/openssl/hmac.h +heartbeat_test.o: ../include/openssl/kssl.h ../include/openssl/lhash.h +heartbeat_test.o: ../include/openssl/obj_mac.h ../include/openssl/objects.h +heartbeat_test.o: ../include/openssl/opensslconf.h +heartbeat_test.o: ../include/openssl/opensslv.h ../include/openssl/ossl_typ.h +heartbeat_test.o: ../include/openssl/pem.h ../include/openssl/pem2.h +heartbeat_test.o: ../include/openssl/pkcs7.h ../include/openssl/pqueue.h +heartbeat_test.o: ../include/openssl/rsa.h ../include/openssl/safestack.h +heartbeat_test.o: ../include/openssl/sha.h ../include/openssl/srtp.h +heartbeat_test.o: ../include/openssl/ssl.h ../include/openssl/ssl2.h +heartbeat_test.o: ../include/openssl/ssl23.h ../include/openssl/ssl3.h +heartbeat_test.o: ../include/openssl/stack.h ../include/openssl/symhacks.h +heartbeat_test.o: ../include/openssl/tls1.h ../include/openssl/x509.h +heartbeat_test.o: ../include/openssl/x509_vfy.h ../ssl/ssl_locl.h +heartbeat_test.o: heartbeat_test.c hmactest.o: ../e_os.h ../include/openssl/asn1.h ../include/openssl/bio.h hmactest.o: ../include/openssl/crypto.h ../include/openssl/e_os2.h hmactest.o: ../include/openssl/evp.h ../include/openssl/hmac.h From f41231d62aaa25630e241a0e94780c6d9b43bc37 Mon Sep 17 00:00:00 2001 From: Ben Laurie Date: Tue, 29 Apr 2014 18:36:39 +0100 Subject: [PATCH 2/5] Make it build/run. --- ssl/heartbeat_test.c | 4 ++-- test/Makefile | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ssl/heartbeat_test.c b/ssl/heartbeat_test.c index eb08ee6b0..35985f836 100644 --- a/ssl/heartbeat_test.c +++ b/ssl/heartbeat_test.c @@ -45,10 +45,10 @@ #include /* As per https://tools.ietf.org/html/rfc6520#section-4 */ -static const int MIN_PADDING_SIZE = 16; +#define MIN_PADDING_SIZE 16 /* Maximum number of payload characters to print as test output */ -static const int MAX_PRINTABLE_CHARACTERS = 1024; +#define MAX_PRINTABLE_CHARACTERS 1024 typedef struct heartbeat_test_fixture { diff --git a/test/Makefile b/test/Makefile index e016d71a9..66ce1a1bb 100644 --- a/test/Makefile +++ b/test/Makefile @@ -85,7 +85,7 @@ FIPS_ECDSAVS= fips_ecdsavs FIPS_TEST_SUITE=fips_test_suite FIPS_CMACTEST= fips_cmactest FIPS_ALGVS= fips_algvs -HEARTBEATTEST = heartbeat_test +HEARTBEATTEST= heartbeat_test TESTS= alltests @@ -379,7 +379,7 @@ test_ocsp: ../apps/openssl$(EXE_EXT) tocsp @sh ./tocsp test_heartbeat: $(HEARTBEATTEST)$(EXE_EXT) - ./$< + ../util/shlib_wrap.sh ./$(HEARTBEATTEST) lint: lint -DLINT $(INCLUDES) $(SRC)>fluff From f5ad068b01a4adae1e1dd4103b5ce7e5e1442f6c Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Thu, 1 May 2014 10:08:18 -0400 Subject: [PATCH 3/5] More through error checks in set_up Checks the return values of ssl_init_wbio_buffer() and ssl3_setup_buffers(). --- ssl/heartbeat_test.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/ssl/heartbeat_test.c b/ssl/heartbeat_test.c index 35985f836..23ae0532d 100644 --- a/ssl/heartbeat_test.c +++ b/ssl/heartbeat_test.c @@ -68,6 +68,7 @@ static HEARTBEAT_TEST_FIXTURE set_up(const char* const test_case_name, const SSL_METHOD* meth) { HEARTBEAT_TEST_FIXTURE fixture; + int setup_ok = 1; memset(&fixture, 0, sizeof(fixture)); fixture.test_case_name = test_case_name; @@ -75,7 +76,8 @@ static HEARTBEAT_TEST_FIXTURE set_up(const char* const test_case_name, if (!fixture.ctx) { fprintf(stderr, "Failed to allocate SSL_CTX for test: %s\n", - test_case_name); + test_case_name); + setup_ok = 0; goto fail; } @@ -83,14 +85,28 @@ static HEARTBEAT_TEST_FIXTURE set_up(const char* const test_case_name, if (!fixture.s) { fprintf(stderr, "Failed to allocate SSL for test: %s\n", test_case_name); + setup_ok = 0; goto fail; } - ssl_init_wbio_buffer(fixture.s, 1); - ssl3_setup_buffers(fixture.s); + if (!ssl_init_wbio_buffer(fixture.s, 1)) + { + fprintf(stderr, "Failed to set up wbio buffer for test: %s\n", + test_case_name); + setup_ok = 0; + goto fail; + } + + if (!ssl3_setup_buffers(fixture.s)) + { + fprintf(stderr, "Failed to setup buffers for test: %s\n", + test_case_name); + setup_ok = 0; + goto fail; + } fail: - if (!fixture.s) + if (!setup_ok) { ERR_print_errors_fp(stderr); exit(EXIT_FAILURE); From 39dd6f4549cf4474f6f6eb4cf9983380215a1c21 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Thu, 1 May 2014 10:10:14 -0400 Subject: [PATCH 4/5] Zero-initialize heartbeat test write buffer The previous calls to memset() were added to tear_down() when I noticed the test spuriously failing in opt mode, with different results each time. This appeared to be because the allocator zeros out memory in debug mode, but not in opt mode. Since the heartbeat functions silently drop the request on error without modifying the contents of the write buffer, whatever random contents were in memory before being reallocated to the write buffer used in the test would cause nondeterministic test failures in the Heartbleed regression cases. Adding these calls allowed the test to pass in both debug and opt modes. Ben Laurie notified me offline that the test was aborting in debug-ben-debug-64-clang mode, configured with GitConfigure and built with GitMake. Looking into this, I realized the first memset() call was zeroing out a reference count used by SSL_free() that was checked in debug-ben-debug-64-clang mode but not in the normal debug mode. Removing the memset() calls from tear_down() and adding a memset() for the write buffer in set_up() addresses the issue and allows the test to successfully execute in debug, opt, and debug-ben-debug-64-clang modes. --- ssl/heartbeat_test.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/ssl/heartbeat_test.c b/ssl/heartbeat_test.c index 23ae0532d..f86ec5f15 100644 --- a/ssl/heartbeat_test.c +++ b/ssl/heartbeat_test.c @@ -105,6 +105,12 @@ static HEARTBEAT_TEST_FIXTURE set_up(const char* const test_case_name, goto fail; } + /* Clear the memory for the return buffer, since this isn't automatically + * zeroed in opt mode and will cause spurious test failures that will change + * with each execution. + */ + memset(fixture.s->s3->wbuf.buf, 0, fixture.s->s3->wbuf.len); + fail: if (!setup_ok) { @@ -160,9 +166,7 @@ static HEARTBEAT_TEST_FIXTURE set_up_tls(const char* const test_case_name) static void tear_down(HEARTBEAT_TEST_FIXTURE fixture) { ERR_print_errors_fp(stderr); - memset(fixture.s, 0, sizeof(*fixture.s)); SSL_free(fixture.s); - memset(fixture.ctx, 0, sizeof(*fixture.ctx)); SSL_CTX_free(fixture.ctx); } From 2ec52dc3a165d464161e28b68aa4332f307a16bf Mon Sep 17 00:00:00 2001 From: Ben Laurie Date: Mon, 19 May 2014 17:38:56 +0100 Subject: [PATCH 5/5] Fixup for ancient compilers. --- ssl/heartbeat_test.c | 51 +++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/ssl/heartbeat_test.c b/ssl/heartbeat_test.c index f86ec5f15..049c1de1b 100644 --- a/ssl/heartbeat_test.c +++ b/ssl/heartbeat_test.c @@ -173,12 +173,14 @@ static void tear_down(HEARTBEAT_TEST_FIXTURE fixture) static void print_payload(const char* const prefix, const unsigned char *payload, const int n) { - const int end = n < MAX_PRINTABLE_CHARACTERS ? n : MAX_PRINTABLE_CHARACTERS; + const int end = n < MAX_PRINTABLE_CHARACTERS ? n + : MAX_PRINTABLE_CHARACTERS; + int i = 0; + printf("%s %d character%s", prefix, n, n == 1 ? "" : "s"); if (end != n) printf(" (first %d shown)", end); printf("\n \""); - int i = 0; for (; i != end; ++i) { const unsigned char c = payload[i]; @@ -194,6 +196,9 @@ static int execute_heartbeat(HEARTBEAT_TEST_FIXTURE fixture) SSL* s = fixture.s; unsigned char *payload = fixture.payload; unsigned char sent_buf[MAX_PRINTABLE_CHARACTERS + 1]; + int return_value; + unsigned const char *p; + int actual_payload_len; s->s3->rrec.data = payload; s->s3->rrec.length = strlen((const char*)payload); @@ -204,7 +209,7 @@ static int execute_heartbeat(HEARTBEAT_TEST_FIXTURE fixture) * point */ memcpy((char *)sent_buf, (const char*)payload, sizeof(sent_buf)); - int return_value = fixture.process_heartbeat(s); + return_value = fixture.process_heartbeat(s); if (return_value != fixture.expected_return_value) { @@ -215,9 +220,9 @@ static int execute_heartbeat(HEARTBEAT_TEST_FIXTURE fixture) } /* If there is any byte alignment, it will be stored in wbuf.offset. */ - unsigned const char *p = &(s->s3->wbuf.buf[ + p = &(s->s3->wbuf.buf[ fixture.return_payload_offset + s->s3->wbuf.offset]); - int actual_payload_len = 0; + actual_payload_len = 0; n2s(p, actual_payload_len); if (actual_payload_len != fixture.expected_payload_len) @@ -282,13 +287,15 @@ static int test_dtls1_not_bleeding() static int test_dtls1_not_bleeding_empty_payload() { + int payload_buf_len; + SETUP_HEARTBEAT_TEST_FIXTURE(dtls); /* Three-byte pad at the beginning for type and payload length, plus a NUL * at the end */ unsigned char payload_buf[4 + MIN_PADDING_SIZE]; memset(payload_buf, ' ', sizeof(payload_buf)); payload_buf[sizeof(payload_buf) - 1] = '\0'; - const int payload_buf_len = honest_payload_size(payload_buf); + payload_buf_len = honest_payload_size(payload_buf); fixture.payload = &payload_buf[0]; fixture.sent_payload_len = payload_buf_len; @@ -364,13 +371,15 @@ static int test_tls1_not_bleeding() static int test_tls1_not_bleeding_empty_payload() { + int payload_buf_len; + SETUP_HEARTBEAT_TEST_FIXTURE(tls); /* Three-byte pad at the beginning for type and payload length, plus a NUL * at the end */ unsigned char payload_buf[4 + MIN_PADDING_SIZE]; memset(payload_buf, ' ', sizeof(payload_buf)); payload_buf[sizeof(payload_buf) - 1] = '\0'; - const int payload_buf_len = honest_payload_size(payload_buf); + payload_buf_len = honest_payload_size(payload_buf); fixture.payload = &payload_buf[0]; fixture.sent_payload_len = payload_buf_len; @@ -416,22 +425,24 @@ static int test_tls1_heartbleed_empty_payload() int main(int argc, char *argv[]) { + int num_failed; + SSL_library_init(); SSL_load_error_strings(); - const int num_failed = test_dtls1_not_bleeding() + - test_dtls1_not_bleeding_empty_payload() + - test_dtls1_heartbleed() + - test_dtls1_heartbleed_empty_payload() + - /* The following test causes an assertion failure at - * ssl/d1_pkt.c:dtls1_write_bytes() in versions prior to 1.0.1g: */ - (OPENSSL_VERSION_NUMBER >= 0x1000107fL ? - test_dtls1_heartbleed_excessive_plaintext_length() : 0) + - test_tls1_not_bleeding() + - test_tls1_not_bleeding_empty_payload() + - test_tls1_heartbleed() + - test_tls1_heartbleed_empty_payload() + - 0; + num_failed = test_dtls1_not_bleeding() + + test_dtls1_not_bleeding_empty_payload() + + test_dtls1_heartbleed() + + test_dtls1_heartbleed_empty_payload() + + /* The following test causes an assertion failure at + * ssl/d1_pkt.c:dtls1_write_bytes() in versions prior to 1.0.1g: */ + (OPENSSL_VERSION_NUMBER >= 0x1000107fL ? + test_dtls1_heartbleed_excessive_plaintext_length() : 0) + + test_tls1_not_bleeding() + + test_tls1_not_bleeding_empty_payload() + + test_tls1_heartbleed() + + test_tls1_heartbleed_empty_payload() + + 0; ERR_print_errors_fp(stderr);