From: Benjamin Kaduk Date: Fri, 9 Jun 2017 17:31:11 +0000 (-0400) Subject: Fix memory leaks in CTLOG_new_from_base64 X-Git-Tag: OpenSSL_1_1_1-pre1~1343 X-Git-Url: https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff_plain;h=62b0a0dea612e3683c6bd4bef359fceda00238e8 Fix memory leaks in CTLOG_new_from_base64 Move the call to ct_base64_decode(), which allocates, until after the check for NULL output parameter. Also place a cap on the number of padding characters used to decrement the output length -- any more than two '='s is not permitted in a well-formed base64 text. Prior to this change, ct_base64_decode() would return a length of -1 along with allocated storage for an input of "====". Reviewed-by: Richard Levitte Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3379) --- diff --git a/crypto/ct/ct_b64.c b/crypto/ct/ct_b64.c index f0bf3aff29..109ffcdcf2 100644 --- a/crypto/ct/ct_b64.c +++ b/crypto/ct/ct_b64.c @@ -24,7 +24,7 @@ static int ct_base64_decode(const char *in, unsigned char **out) { size_t inlen = strlen(in); - int outlen; + int outlen, i; unsigned char *outbuf = NULL; if (inlen == 0) { @@ -45,9 +45,12 @@ static int ct_base64_decode(const char *in, unsigned char **out) goto err; } - /* Subtract padding bytes from |outlen| */ + /* Subtract padding bytes from |outlen|. Any more than 2 is malformed. */ + i = 0; while (in[--inlen] == '=') { --outlen; + if (++i > 2) + goto err; } *out = outbuf; @@ -132,7 +135,7 @@ SCT *SCT_new_from_base64(unsigned char version, const char *logid_base64, int CTLOG_new_from_base64(CTLOG **ct_log, const char *pkey_base64, const char *name) { unsigned char *pkey_der = NULL; - int pkey_der_len = ct_base64_decode(pkey_base64, &pkey_der); + int pkey_der_len; const unsigned char *p; EVP_PKEY *pkey = NULL; @@ -141,7 +144,8 @@ int CTLOG_new_from_base64(CTLOG **ct_log, const char *pkey_base64, const char *n return 0; } - if (pkey_der_len <= 0) { + pkey_der_len = ct_base64_decode(pkey_base64, &pkey_der); + if (pkey_der_len < 0) { CTerr(CT_F_CTLOG_NEW_FROM_BASE64, CT_R_LOG_CONF_INVALID_KEY); return 0; } diff --git a/test/ct_test.c b/test/ct_test.c index 6b36a43469..b551b85bc2 100644 --- a/test/ct_test.c +++ b/test/ct_test.c @@ -344,7 +344,7 @@ end: #define SETUP_CT_TEST_FIXTURE() SETUP_TEST_FIXTURE(CT_TEST_FIXTURE, set_up) #define EXECUTE_CT_TEST() EXECUTE_TEST(execute_cert_test, tear_down) -static int test_no_scts_in_certificate() +static int test_no_scts_in_certificate(void) { SETUP_CT_TEST_FIXTURE(); fixture.certs_dir = certs_dir; @@ -354,7 +354,7 @@ static int test_no_scts_in_certificate() EXECUTE_CT_TEST(); } -static int test_one_sct_in_certificate() +static int test_one_sct_in_certificate(void) { SETUP_CT_TEST_FIXTURE(); fixture.certs_dir = certs_dir; @@ -366,7 +366,7 @@ static int test_one_sct_in_certificate() EXECUTE_CT_TEST(); } -static int test_multiple_scts_in_certificate() +static int test_multiple_scts_in_certificate(void) { SETUP_CT_TEST_FIXTURE(); fixture.certs_dir = certs_dir; @@ -378,7 +378,7 @@ static int test_multiple_scts_in_certificate() EXECUTE_CT_TEST(); } -static int test_verify_one_sct() +static int test_verify_one_sct(void) { SETUP_CT_TEST_FIXTURE(); fixture.certs_dir = certs_dir; @@ -389,7 +389,7 @@ static int test_verify_one_sct() EXECUTE_CT_TEST(); } -static int test_verify_multiple_scts() +static int test_verify_multiple_scts(void) { SETUP_CT_TEST_FIXTURE(); fixture.certs_dir = certs_dir; @@ -400,7 +400,7 @@ static int test_verify_multiple_scts() EXECUTE_CT_TEST(); } -static int test_verify_fails_for_future_sct() +static int test_verify_fails_for_future_sct(void) { SETUP_CT_TEST_FIXTURE(); fixture.epoch_time_in_ms = 1365094800000; /* Apr 4 17:00:00 2013 GMT */ @@ -413,7 +413,7 @@ static int test_verify_fails_for_future_sct() EXECUTE_CT_TEST(); } -static int test_decode_tls_sct() +static int test_decode_tls_sct(void) { const unsigned char tls_sct_list[] = "\x00\x78" /* length of list */ "\x00\x76" @@ -441,7 +441,7 @@ static int test_decode_tls_sct() EXECUTE_CT_TEST(); } -static int test_encode_tls_sct() +static int test_encode_tls_sct(void) { const char log_id[] = "3xwuwRUAlFJHqWFoMl3cXHlZ6PfG04j8AC4LvT9012Q="; const uint64_t timestamp = 1; @@ -469,7 +469,7 @@ static int test_encode_tls_sct() * Tests that the CT_POLICY_EVAL_CTX default time is approximately now. * Allow +-10 minutes, as it may compensate for clock skew. */ -static int test_default_ct_policy_eval_ctx_time_is_now() +static int test_default_ct_policy_eval_ctx_time_is_now(void) { int success = 0; CT_POLICY_EVAL_CTX *ct_policy_ctx = CT_POLICY_EVAL_CTX_new(); @@ -486,6 +486,20 @@ end: return success; } +static int test_ctlog_from_base64(void) +{ + CTLOG *log = NULL; + const char notb64[] = "\01\02\03\04"; + const char pad[] = "===="; + const char name[] = "name"; + + /* We expect these to both fail! */ + if (!TEST_true(!CTLOG_new_from_base64(&log, notb64, name)) + || !TEST_true(!CTLOG_new_from_base64(&log, pad, name))) + return 0; + return 1; +} + int test_main(int argc, char *argv[]) { if ((ct_dir = getenv("CT_DIR")) == NULL) @@ -502,6 +516,7 @@ int test_main(int argc, char *argv[]) ADD_TEST(test_decode_tls_sct); ADD_TEST(test_encode_tls_sct); ADD_TEST(test_default_ct_policy_eval_ctx_time_is_now); + ADD_TEST(test_ctlog_from_base64); return run_tests(argv[0]); }