From 4aed8756d86e2b934e83d916e57bee91c83c4b28 Mon Sep 17 00:00:00 2001 From: FdaSilvaYY Date: Mon, 27 Jun 2016 22:42:25 +0200 Subject: [PATCH] Improve some error management code in CT Separate invalid input case from any internal (malloc) failure Reviewed-by: Rich Salz Reviewed-by: Matt Caswell --- crypto/ct/ct_b64.c | 25 +++++++++++++++++-------- crypto/ct/ct_log.c | 34 ++++++++++++++++------------------ include/openssl/ct.h | 5 +++-- 3 files changed, 36 insertions(+), 28 deletions(-) diff --git a/crypto/ct/ct_b64.c b/crypto/ct/ct_b64.c index d6279d2298..9cf7c51be3 100644 --- a/crypto/ct/ct_b64.c +++ b/crypto/ct/ct_b64.c @@ -115,17 +115,26 @@ SCT *SCT_new_from_base64(unsigned char version, const char *logid_base64, return NULL; } -CTLOG *CTLOG_new_from_base64(const char *pkey_base64, const char *name) +/* + * This methods returns: 1 on Success, + * 0 on decoding failure, + * -1 on internal (malloc) failure, or invalid parameter if any. + */ +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); const unsigned char *p; EVP_PKEY *pkey = NULL; - CTLOG *log = NULL; + + if (ct_log == NULL) { + CTerr(CT_F_CTLOG_NEW_FROM_BASE64, ERR_R_PASSED_INVALID_ARGUMENT); + return 0; + } if (pkey_der_len <= 0) { CTerr(CT_F_CTLOG_NEW_FROM_BASE64, CT_R_LOG_CONF_INVALID_KEY); - return NULL; + return 0; } p = pkey_der; @@ -133,14 +142,14 @@ CTLOG *CTLOG_new_from_base64(const char *pkey_base64, const char *name) OPENSSL_free(pkey_der); if (pkey == NULL) { CTerr(CT_F_CTLOG_NEW_FROM_BASE64, CT_R_LOG_CONF_INVALID_KEY); - return NULL; + return 0; } - log = CTLOG_new(pkey, name); - if (log == NULL) { + *ct_log = CTLOG_new(pkey, name); + if (*ct_log == NULL) { EVP_PKEY_free(pkey); - return NULL; + return -1; } - return log; + return 1; } diff --git a/crypto/ct/ct_log.c b/crypto/ct/ct_log.c index b01bb5693e..1874d9121c 100644 --- a/crypto/ct/ct_log.c +++ b/crypto/ct/ct_log.c @@ -116,31 +116,23 @@ void CTLOG_STORE_free(CTLOG_STORE *store) } } -static CTLOG *ctlog_new_from_conf(const CONF *conf, const char *section) +static int ctlog_new_from_conf(CTLOG **ct_log, const CONF *conf, const char *section) { - CTLOG *ret = NULL; const char *description = NCONF_get_string(conf, section, "description"); char *pkey_base64; if (description == NULL) { CTerr(CT_F_CTLOG_NEW_FROM_CONF, CT_R_LOG_CONF_MISSING_DESCRIPTION); - goto end; + return 0; } pkey_base64 = NCONF_get_string(conf, section, "key"); if (pkey_base64 == NULL) { CTerr(CT_F_CTLOG_NEW_FROM_CONF, CT_R_LOG_CONF_MISSING_KEY); - goto end; - } - - ret = CTLOG_new_from_base64(pkey_base64, description); - if (ret == NULL) { - CTerr(CT_F_CTLOG_NEW_FROM_CONF, CT_R_LOG_CONF_INVALID); - goto end; + return 0; } -end: - return ret; + return CTLOG_new_from_base64(ct_log, pkey_base64, description); } int CTLOG_STORE_load_default_file(CTLOG_STORE *store) @@ -154,9 +146,10 @@ int CTLOG_STORE_load_default_file(CTLOG_STORE *store) } /* - * Called by CONF_parse_list, which stops if this returns <= 0, so don't unless - * something very bad happens. Otherwise, one bad log entry would stop loading - * of any of the following log entries. + * Called by CONF_parse_list, which stops if this returns <= 0, + * Otherwise, one bad log entry would stop loading of any of + * the following log entries. + * It may stop parsing and returns -1 on any internal (malloc) error. */ static int ctlog_store_load_log(const char *log_name, int log_name_len, void *arg) @@ -165,6 +158,7 @@ static int ctlog_store_load_log(const char *log_name, int log_name_len, CTLOG *ct_log = NULL; /* log_name may not be null-terminated, so fix that before using it */ char *tmp; + int ret = 0; /* log_name will be NULL for empty list entries */ if (log_name == NULL) @@ -174,10 +168,14 @@ static int ctlog_store_load_log(const char *log_name, int log_name_len, if (tmp == NULL) goto mem_err; - ct_log = ctlog_new_from_conf(load_ctx->conf, tmp); + ret = ctlog_new_from_conf(&ct_log, load_ctx->conf, tmp); OPENSSL_free(tmp); - if (ct_log == NULL) { - /* TODO: split invalid input case from internal failure */ + + if (ret < 0) { + /* Propagate any internal error */ + return ret; + } + if (ret == 0) { /* If we can't load this log, record that fact and skip it */ ++load_ctx->invalid_log_entries; return 1; diff --git a/include/openssl/ct.h b/include/openssl/ct.h index 4ef6063dd8..be7a953ff4 100644 --- a/include/openssl/ct.h +++ b/include/openssl/ct.h @@ -419,10 +419,11 @@ CTLOG *CTLOG_new(EVP_PKEY *public_key, const char *name); CTLOG *CTLOG_new_null(void); /* - * Creates a new CT log instance with the given base64 public_key and |name|. + * Creates a new CT |ct_log| instance with the given base64 public_key and |name|. * Should be deleted by the caller using CTLOG_free when no longer needed. */ -CTLOG *CTLOG_new_from_base64(const char *pkey_base64, const char *name); +int CTLOG_new_from_base64(CTLOG ** ct_log, + const char *pkey_base64, const char *name); /* * Deletes a CT log instance and its fields. -- 2.25.1