Addresses review comments from richsalz

Reviewed-by: NBen Laurie <ben@openssl.org>
Reviewed-by: NRich Salz <rsalz@openssl.org>

crypto/ct/ct_b64.c

@@ -77,18 +77,12 @@
  * A new string will be malloc'd and assigned to |out|. This will be owned by
  * the caller. Do not provide a pre-allocated string in |out|.
  */
-static int CT_base64_decode(const char *in, unsigned char **out)
+static int ct_base64_decode(const char *in, unsigned char **out)
 {
-    size_t inlen;
+    size_t inlen = strlen(in);
     int outlen;
     unsigned char *outbuf = NULL;
 
-    if (in == NULL || out == NULL) {
-        CTerr(CT_F_CT_BASE64_DECODE, ERR_R_PASSED_NULL_PARAMETER);
-        goto err;
-    }
-
-    inlen = strlen(in);
     if (inlen == 0) {
         *out = NULL;
         return 0;
@@ -119,18 +113,10 @@ SCT *SCT_new_from_base64(unsigned char version, const char *logid_base64,
                          const char *extensions_base64,
                          const char *signature_base64)
 {
-    SCT *sct;
+    SCT *sct = SCT_new();
     unsigned char *dec = NULL;
     int declen;
 
-    if (logid_base64 == NULL ||
-        extensions_base64 == NULL ||
-        signature_base64 == NULL) {
-        CTerr(CT_F_SCT_NEW_FROM_BASE64, ERR_R_PASSED_NULL_PARAMETER);
-        return NULL;
-    }
-
-    sct = SCT_new();
     if (sct == NULL) {
         CTerr(CT_F_SCT_NEW_FROM_BASE64, ERR_R_MALLOC_FAILURE);
         return NULL;
@@ -145,7 +131,7 @@ SCT *SCT_new_from_base64(unsigned char version, const char *logid_base64,
         goto err;
     }
 
-    declen = CT_base64_decode(logid_base64, &dec);
+    declen = ct_base64_decode(logid_base64, &dec);
     if (declen < 0) {
         CTerr(CT_F_SCT_NEW_FROM_BASE64, X509_R_BASE64_DECODE_ERROR);
         goto err;
@@ -154,7 +140,7 @@ SCT *SCT_new_from_base64(unsigned char version, const char *logid_base64,
         goto err;
     dec = NULL;
 
-    declen = CT_base64_decode(extensions_base64, &dec);
+    declen = ct_base64_decode(extensions_base64, &dec);
     if (declen < 0) {
         CTerr(CT_F_SCT_NEW_FROM_BASE64, X509_R_BASE64_DECODE_ERROR);
         goto err;
@@ -162,7 +148,7 @@ SCT *SCT_new_from_base64(unsigned char version, const char *logid_base64,
     SCT_set0_extensions(sct, dec, declen);
     dec = NULL;
 
-    declen = CT_base64_decode(signature_base64, &dec);
+    declen = ct_base64_decode(signature_base64, &dec);
     if (declen < 0) {
         CTerr(CT_F_SCT_NEW_FROM_BASE64, X509_R_BASE64_DECODE_ERROR);
         goto err;
@@ -188,12 +174,11 @@ SCT *SCT_new_from_base64(unsigned char version, const char *logid_base64,
 CTLOG *CTLOG_new_from_base64(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;
-    int pkey_der_len;
     EVP_PKEY *pkey = NULL;
     CTLOG *log = NULL;
 
-    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 NULL;

crypto/ct/ct_log.c

@@ -53,6 +53,9 @@
  *
  */
 
+#include <stdlib.h>
+#include <string.h>
+
 #include <openssl/conf.h>
 #include <openssl/ct.h>
 #include <openssl/err.h>
@@ -88,48 +91,47 @@ typedef struct ctlog_store_load_ctx_st {
  * Creates an empty context for loading a CT log store.
  * It should be populated before use.
  */
-static CTLOG_STORE_LOAD_CTX *CTLOG_STORE_LOAD_CTX_new();
+static CTLOG_STORE_LOAD_CTX *ctlog_store_load_ctx_new();
 
 /*
  * Deletes a CT log store load context.
  * Does not delete any of the fields.
  */
-static void CTLOG_STORE_LOAD_CTX_free(CTLOG_STORE_LOAD_CTX* ctx);
+static void ctlog_store_load_ctx_free(CTLOG_STORE_LOAD_CTX* ctx);
 
-static CTLOG_STORE_LOAD_CTX *CTLOG_STORE_LOAD_CTX_new()
+static CTLOG_STORE_LOAD_CTX *ctlog_store_load_ctx_new()
 {
-    CTLOG_STORE_LOAD_CTX *ctx = OPENSSL_zalloc(sizeof(CTLOG_STORE_LOAD_CTX));
+    CTLOG_STORE_LOAD_CTX *ctx = OPENSSL_zalloc(sizeof(*ctx));
+
     if (ctx == NULL) {
         CTerr(CT_F_CTLOG_STORE_LOAD_CTX_NEW, ERR_R_MALLOC_FAILURE);
         goto err;
     }
 
     return ctx;
-
 err:
-    CTLOG_STORE_LOAD_CTX_free(ctx);
+    ctlog_store_load_ctx_free(ctx);
     return NULL;
 }
 
-static void CTLOG_STORE_LOAD_CTX_free(CTLOG_STORE_LOAD_CTX* ctx)
+static void ctlog_store_load_ctx_free(CTLOG_STORE_LOAD_CTX* ctx)
 {
-    if (ctx == NULL)
-        return;
-
     OPENSSL_free(ctx);
 }
 
 /* Converts a log's public key into a SHA256 log ID */
-static int CT_v1_log_id_from_pkey(EVP_PKEY *pkey,
+static int ct_v1_log_id_from_pkey(EVP_PKEY *pkey,
                                   unsigned char log_id[CT_V1_HASHLEN])
 {
     int ret = 0;
     unsigned char *pkey_der = NULL;
     int pkey_der_len = i2d_PUBKEY(pkey, &pkey_der);
+
     if (pkey_der_len <= 0) {
         CTerr(CT_F_CT_V1_LOG_ID_FROM_PKEY, CT_R_LOG_KEY_INVALID);
         goto err;
     }
+
     SHA256(pkey_der, pkey_der_len, log_id);
     ret = 1;
 err:
@@ -139,12 +141,15 @@ err:
 
 CTLOG_STORE *CTLOG_STORE_new(void)
 {
-    CTLOG_STORE *ret = OPENSSL_malloc(sizeof(CTLOG_STORE));
+    CTLOG_STORE *ret = OPENSSL_zalloc(sizeof(*ret));
+
     if (ret == NULL)
         goto err;
+
     ret->logs = sk_CTLOG_new_null();
     if (ret->logs == NULL)
         goto err;
+
     return ret;
 err:
     CTLOG_STORE_free(ret);
@@ -159,17 +164,11 @@ void CTLOG_STORE_free(CTLOG_STORE *store)
     }
 }
 
-static CTLOG *CTLOG_new_from_conf(const CONF *conf, const char *section)
+static CTLOG *ctlog_new_from_conf(const CONF *conf, const char *section)
 {
     CTLOG *ret = NULL;
-    char *description;
+    char *description = NCONF_get_string(conf, section, "description");
     char *pkey_base64;
-    if (conf == NULL || section == NULL) {
-        CTerr(CT_F_CTLOG_NEW_FROM_CONF, ERR_R_PASSED_NULL_PARAMETER);
-        goto end;
-    }
-
-    description = NCONF_get_string(conf, section, "description");
 
     if (description == NULL) {
         CTerr(CT_F_CTLOG_NEW_FROM_CONF, CT_R_LOG_CONF_MISSING_DESCRIPTION);
@@ -177,7 +176,6 @@ static CTLOG *CTLOG_new_from_conf(const CONF *conf, const char *section)
     }
 
     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;
@@ -195,20 +193,22 @@ end:
 
 int CTLOG_STORE_load_default_file(CTLOG_STORE *store)
 {
-    char *fpath = (char *)getenv(CTLOG_FILE_EVP);
+    const char *fpath = getenv(CTLOG_FILE_EVP);
+
     if (fpath == NULL)
       fpath = CTLOG_FILE;
+
     return CTLOG_STORE_load_file(store, fpath);
 }
 
-static int CTLOG_STORE_load_log(const char *log_name, int log_name_len, void *arg)
+static int ctlog_store_load_log(const char *log_name, int log_name_len, void *arg)
 {
     CTLOG_STORE_LOAD_CTX *load_ctx = arg;
     CTLOG *ct_log;
-
     /* log_name may not be null-terminated, so fix that before using it */
     char *tmp = OPENSSL_strndup(log_name, log_name_len);
-    ct_log = CTLOG_new_from_conf(load_ctx->conf, tmp);
+
+    ct_log = ctlog_new_from_conf(load_ctx->conf, tmp);
     OPENSSL_free(tmp);
     if (ct_log == NULL)
         return 0;
@@ -221,7 +221,8 @@ int CTLOG_STORE_load_file(CTLOG_STORE *store, const char *file)
 {
     int ret = -1;
     char *enabled_logs;
-    CTLOG_STORE_LOAD_CTX* load_ctx = CTLOG_STORE_LOAD_CTX_new();
+    CTLOG_STORE_LOAD_CTX* load_ctx = ctlog_store_load_ctx_new();
+
     load_ctx->log_store = store;
     load_ctx->conf = NCONF_new(NULL);
     if (load_ctx->conf == NULL)
@@ -234,11 +235,11 @@ int CTLOG_STORE_load_file(CTLOG_STORE *store, const char *file)
     }
 
     enabled_logs = NCONF_get_string(load_ctx->conf, NULL, "enabled_logs");
-    CONF_parse_list(enabled_logs, ',', 1, CTLOG_STORE_load_log, load_ctx);
+    CONF_parse_list(enabled_logs, ',', 1, ctlog_store_load_log, load_ctx);
 
 end:
     NCONF_free(load_ctx->conf);
-    CTLOG_STORE_LOAD_CTX_free(load_ctx);
+    ctlog_store_load_ctx_free(load_ctx);
     return ret;
 }
 
@@ -249,20 +250,19 @@ end:
  */
 CTLOG *CTLOG_new(EVP_PKEY *public_key, const char *name)
 {
-    CTLOG *ret = NULL;
-    if (public_key == NULL || name == NULL) {
-        CTerr(CT_F_CTLOG_NEW, ERR_R_PASSED_NULL_PARAMETER);
-        goto err;
-    }
-    ret = CTLOG_new_null();
+    CTLOG *ret = CTLOG_new_null();
+
     if (ret == NULL)
         goto err;
+
     ret->name = OPENSSL_strdup(name);
     if (ret->name == NULL)
         goto err;
+
     ret->public_key = public_key;
-    if (CT_v1_log_id_from_pkey(public_key, ret->log_id) != 1)
+    if (ct_v1_log_id_from_pkey(public_key, ret->log_id) != 1)
         goto err;
+
     return ret;
 err:
     CTLOG_free(ret);
@@ -271,9 +271,11 @@ err:
 
 CTLOG *CTLOG_new_null(void)
 {
-    CTLOG *ret = OPENSSL_zalloc(sizeof(CTLOG));
+    CTLOG *ret = OPENSSL_zalloc(sizeof(*ret));
+
     if (ret == NULL)
         CTerr(CT_F_CTLOG_NEW_NULL, ERR_R_MALLOC_FAILURE);
+
     return ret;
 }
 
@@ -282,11 +284,7 @@ void CTLOG_free(CTLOG *log)
 {
     if (log != NULL) {
         OPENSSL_free(log->name);
-        log->name = NULL;
-
         EVP_PKEY_free(log->public_key);
-        log->public_key = NULL;
-
         OPENSSL_free(log);
     }
 }
@@ -316,15 +314,12 @@ CTLOG *CTLOG_STORE_get0_log_by_id(const CTLOG_STORE *store,
                                   size_t log_id_len)
 {
     int i;
-    if (store == NULL) {
-        CTerr(CT_F_CTLOG_STORE_GET0_LOG_BY_ID, ERR_R_PASSED_NULL_PARAMETER);
-        goto end;
-    }
+
     for (i = 0; i < sk_CTLOG_num(store->logs); ++i) {
         CTLOG *log = sk_CTLOG_value(store->logs, i);
         if (memcmp(log->log_id, log_id, log_id_len) == 0)
             return log;
     }
-end:
+
     return NULL;
 }

crypto/ct/ct_oct.c

@@ -70,30 +70,6 @@
 
 #include "ct_locl.h"
 
-#define n2s(c,s)        ((s=(((unsigned int)((c)[0]))<< 8)| \
-                            (((unsigned int)((c)[1]))    )),c+=2)
-
-#define s2n(s,c)        ((c[0]=(unsigned char)(((s)>> 8)&0xff), \
-                          c[1]=(unsigned char)(((s)    )&0xff)),c+=2)
-
-#define n2l8(c,l)       (l =((uint64_t)(*((c)++)))<<56, \
-                         l|=((uint64_t)(*((c)++)))<<48, \
-                         l|=((uint64_t)(*((c)++)))<<40, \
-                         l|=((uint64_t)(*((c)++)))<<32, \
-                         l|=((uint64_t)(*((c)++)))<<24, \
-                         l|=((uint64_t)(*((c)++)))<<16, \
-                         l|=((uint64_t)(*((c)++)))<< 8, \
-                         l|=((uint64_t)(*((c)++))))
-
-#define l2n8(l,c)       (*((c)++)=(unsigned char)(((l)>>56)&0xff), \
-                         *((c)++)=(unsigned char)(((l)>>48)&0xff), \
-                         *((c)++)=(unsigned char)(((l)>>40)&0xff), \
-                         *((c)++)=(unsigned char)(((l)>>32)&0xff), \
-                         *((c)++)=(unsigned char)(((l)>>24)&0xff), \
-                         *((c)++)=(unsigned char)(((l)>>16)&0xff), \
-                         *((c)++)=(unsigned char)(((l)>> 8)&0xff), \
-                         *((c)++)=(unsigned char)(((l)    )&0xff))
-
 int o2i_SCT_signature(SCT *sct, const unsigned char **in, size_t len)
 {
     size_t siglen;

crypto/ct/ct_sct.c

@@ -70,7 +70,7 @@
 
 SCT *SCT_new(void)
 {
-    SCT *sct = OPENSSL_zalloc(sizeof(SCT));
+    SCT *sct = OPENSSL_zalloc(sizeof(*sct));
 
     if (sct == NULL) {
         CTerr(CT_F_SCT_NEW, ERR_R_MALLOC_FAILURE);
@@ -335,8 +335,7 @@ CTLOG *SCT_get0_log(const SCT *sct)
 
 int SCT_set0_log(SCT *sct, const CTLOG_STORE *ct_logs)
 {
-    sct->log =
-            CTLOG_STORE_get0_log_by_id(ct_logs, sct->log_id, sct->log_id_len);
+    sct->log = CTLOG_STORE_get0_log_by_id(ct_logs, sct->log_id, sct->log_id_len);
 
     return sct->log != NULL;
 }

crypto/ct/ct_sct_ctx.c

@@ -71,9 +71,11 @@
 
 SCT_CTX *SCT_CTX_new(void)
 {
-    SCT_CTX *sctx = OPENSSL_zalloc(sizeof(SCT_CTX));
+    SCT_CTX *sctx = OPENSSL_zalloc(sizeof(*sctx));
+
     if (sctx == NULL)
         CTerr(CT_F_SCT_CTX_NEW, ERR_R_MALLOC_FAILURE);
+
     return sctx;
 }
 
@@ -89,28 +91,44 @@ void SCT_CTX_free(SCT_CTX *sctx)
     OPENSSL_free(sctx);
 }
 
-/* retrieve extension index checking for duplicates */
-static int sct_get_ext(X509 *cert, int nid)
+/*
+ * Finds the index of the first extension with the given NID in cert.
+ * If there is more than one extension with that NID, *is_duplicated is set to
+ * 1, otherwise 0 (unless it is NULL).
+ */
+static int ct_x509_get_ext(X509 *cert, int nid, int *is_duplicated)
 {
-    int rv = X509_get_ext_by_NID(cert, nid, -1);
-    if (rv >= 0 && X509_get_ext_by_NID(cert, nid, rv) >= 0)
-        return -2;
-    return rv;
+    int ret = X509_get_ext_by_NID(cert, nid, -1);
+
+    if (is_duplicated != NULL)
+        *is_duplicated = ret >= 0 && X509_get_ext_by_NID(cert, nid, ret) >= 0;
+
+    return ret;
 }
 
 /*
- * modify certificate by deleting extensions, copying issuer
- * and AKID if necessary.
+ * Modifies a certificate by deleting extensions and copying the issuer and
+ * AKID from the presigner certificate, if necessary.
+ * Returns 1 on success, 0 otherwise.
  */
-static int sct_cert_fixup(X509 *cert, X509 *presigner)
+static int ct_x509_cert_fixup(X509 *cert, X509 *presigner)
 {
     int preidx, certidx;
+    int pre_akid_ext_is_dup, cert_akid_ext_is_dup;
+
     if (presigner == NULL)
         return 1;
-    preidx = sct_get_ext(presigner, NID_authority_key_identifier);
-    certidx = sct_get_ext(cert, NID_authority_key_identifier);
-    /* Invalid certificate if duplicate */
-    if (preidx == -2 || certidx == -2)
+
+    preidx = ct_x509_get_ext(presigner, NID_authority_key_identifier,
+                             &pre_akid_ext_is_dup);
+    certidx = ct_x509_get_ext(cert, NID_authority_key_identifier,
+                              &cert_akid_ext_is_dup);
+
+    /* An error occurred whilst searching for the extension */
+    if (preidx < -1 || certidx < -1)
+        return 0;
+    /* Invalid certificate if they contain duplicate extensions */
+    if (pre_akid_ext_is_dup || cert_akid_ext_is_dup)
         return 0;
     /* AKID must be present in both certificate or absent in both */
     if (preidx >= 0 && certidx == -1)
@@ -125,11 +143,13 @@ static int sct_cert_fixup(X509 *cert, X509 *presigner)
         X509_EXTENSION *preext = X509_get_ext(presigner, preidx);
         X509_EXTENSION *certext = X509_get_ext(cert, certidx);
         ASN1_OCTET_STRING *preextdata;
+
         /* Should never happen */
         if (preext == NULL || certext == NULL)
             return 0;
         preextdata = X509_EXTENSION_get_data(preext);
-        if (preextdata == NULL || !X509_EXTENSION_set_data(certext, preextdata))
+        if (preextdata == NULL ||
+            !X509_EXTENSION_set_data(certext, preextdata))
             return 0;
     }
     return 1;
@@ -140,42 +160,51 @@ int SCT_CTX_set1_cert(SCT_CTX *sctx, X509 *cert, X509 *presigner)
     unsigned char *certder = NULL, *preder = NULL;
     X509 *pretmp = NULL;
     int certderlen = 0, prederlen = 0;
-    int idx = -1, idxp = -1;
-    idxp = sct_get_ext(cert, NID_ct_precert_poison);
+    int idx = -1;
+    int poison_ext_is_dup, sct_ext_is_dup;
+    int poison_idx = ct_x509_get_ext(cert, NID_ct_precert_poison, &poison_ext_is_dup);
+
     /* Duplicate poison */
-    if (idxp == -2)
+    if (poison_ext_is_dup)
         goto err;
-    /* If no poison store encoding */
-    if (idxp == -1) {
-        /* If presigner must have poison */
-        if (presigner)
+
+    /* If no poison extension, store encoding */
+    if (poison_idx == -1) {
+        /* presigner must have poison */
+        if (presigner != NULL)
             goto err;
+
         certderlen = i2d_X509(cert, &certder);
         if (certderlen < 0)
             goto err;
     }
+
     /* See if have precert scts extension */
-    idx = X509_get_ext_by_NID(cert, NID_ct_precert_scts, -1);
+    idx = ct_x509_get_ext(cert, NID_ct_precert_scts, &sct_ext_is_dup);
     /* Duplicate scts */
-    if (idx == -2)
+    if (sct_ext_is_dup)
         goto err;
+
     if (idx >= 0) {
         /* Can't have both poison and scts */
-        if (idxp >= 0)
+        if (poison_idx >= 0)
             goto err;
-    } else
-        idx = idxp;
+    } else {
+        idx = poison_idx;
+    }
+
     if (idx >= 0) {
         X509_EXTENSION *ext;
-        /*
-         * Take a copy of certificate so we don't modify passed version
-         */
+
+        /* Take a copy of certificate so we don't modify passed version */
         pretmp = X509_dup(cert);
         if (pretmp == NULL)
             goto err;
+
         ext = X509_delete_ext(pretmp, idx);
         X509_EXTENSION_free(ext);
-        if (!sct_cert_fixup(pretmp, presigner))
+
+        if (!ct_x509_cert_fixup(pretmp, presigner))
             goto err;
 
         prederlen = i2d_re_X509_tbs(pretmp, &preder);
@@ -194,43 +223,44 @@ int SCT_CTX_set1_cert(SCT_CTX *sctx, X509 *cert, X509 *presigner)
     sctx->prederlen = prederlen;
 
     return 1;
-
- err:
+err:
     OPENSSL_free(certder);
     OPENSSL_free(preder);
     X509_free(pretmp);
     return 0;
 }
 
-static int CT_public_key_hash(X509_PUBKEY *pkey, unsigned char **hash,
+static int ct_public_key_hash(X509_PUBKEY *pkey, unsigned char **hash,
                               size_t *hash_len)
 {
     int ret = -1;
     unsigned char *md = NULL, *der = NULL;
     int der_len;
     unsigned int md_len;
-    if (pkey == NULL)
-        goto err;
+
     /* Reuse buffer if possible */
     if (*hash != NULL && *hash_len >= SHA256_DIGEST_LENGTH) {
         md = *hash;
     } else {
         md = OPENSSL_malloc(SHA256_DIGEST_LENGTH);
         if (md == NULL)
-          goto err;
+            goto err;
     }
 
     /* Calculate key hash */
     der_len = i2d_X509_PUBKEY(pkey, &der);
     if (der_len <= 0)
         goto err;
+
     if (!EVP_Digest(der, der_len, md, &md_len, EVP_sha256(), NULL))
         goto err;
+
     if (md != *hash) {
         OPENSSL_free(*hash);
         *hash = md;
         *hash_len = SHA256_DIGEST_LENGTH;
     }
+
     md = NULL;
     ret = 1;
  err:
@@ -241,22 +271,23 @@ static int CT_public_key_hash(X509_PUBKEY *pkey, unsigned char **hash,
 
 int SCT_CTX_set1_issuer(SCT_CTX *sctx, const X509 *issuer)
 {
-    return CT_public_key_hash(X509_get_X509_PUBKEY(issuer), &sctx->ihash,
+    return ct_public_key_hash(X509_get_X509_PUBKEY(issuer), &sctx->ihash,
                               &sctx->ihashlen);
 }
 
 int SCT_CTX_set1_issuer_pubkey(SCT_CTX *sctx, X509_PUBKEY *pubkey)
 {
-    return CT_public_key_hash(pubkey, &sctx->ihash, &sctx->ihashlen);
+    return ct_public_key_hash(pubkey, &sctx->ihash, &sctx->ihashlen);
 }
 
 int SCT_CTX_set1_pubkey(SCT_CTX *sctx, X509_PUBKEY *pubkey)
 {
     EVP_PKEY *pkey = X509_PUBKEY_get(pubkey);
+
     if (pkey == NULL)
         return 0;
 
-    if (!CT_public_key_hash(pubkey, &sctx->pkeyhash, &sctx->pkeyhashlen)) {
+    if (!ct_public_key_hash(pubkey, &sctx->pkeyhash, &sctx->pkeyhashlen)) {
         EVP_PKEY_free(pkey);
         return 0;
     }

crypto/ct/ct_vfy.c

@@ -108,23 +108,25 @@ static int sct_ctx_update(EVP_MD_CTX *ctx, const SCT_CTX *sctx, const SCT *sct)
     unsigned char tmpbuf[12];
     unsigned char *p, *der;
     size_t derlen;
-    /*
-     * digitally-signed struct { (1 byte) Version sct_version; (1 byte)
-     * SignatureType signature_type = certificate_timestamp; (8 bytes) uint64
-     * timestamp; (2 bytes) LogEntryType entry_type; (? bytes)
-     * select(entry_type) { case x509_entry: ASN.1Cert; case precert_entry:
-     * PreCert; } signed_entry; (2 bytes + sct->ext_len) CtExtensions
-     * extensions;
+    /*+
+     * digitally-signed struct {
+     *   (1 byte) Version sct_version;
+     *   (1 byte) SignatureType signature_type = certificate_timestamp;
+     *   (8 bytes) uint64 timestamp;
+     *   (2 bytes) LogEntryType entry_type;
+     *   (? bytes) select(entry_type) {
+     *     case x509_entry: ASN.1Cert;
+     *     case precert_entry: PreCert;
+     *   } signed_entry;
+     *   (2 bytes + sct->ext_len) CtExtensions extensions;
+     * }
      */
-
     if (sct->entry_type == CT_LOG_ENTRY_TYPE_NOT_SET)
         return 0;
-
     if (sct->entry_type == CT_LOG_ENTRY_TYPE_PRECERT && sctx->ihash == NULL)
         return 0;
 
     p = tmpbuf;
-
     *p++ = sct->version;
     *p++ = SIGNATURE_TYPE_CERT_TIMESTAMP;
     l2n8(sct->timestamp, p);
@@ -172,6 +174,7 @@ int SCT_verify(const SCT_CTX *sctx, const SCT *sct)
 {
     EVP_MD_CTX *ctx = NULL;
     int ret = -1;
+
     if (!SCT_is_complete(sct) || sctx->pkey == NULL ||
         sct->entry_type == CT_LOG_ENTRY_TYPE_NOT_SET ||
         (sct->entry_type == CT_LOG_ENTRY_TYPE_PRECERT && sctx->ihash == NULL)) {
@@ -187,6 +190,7 @@ int SCT_verify(const SCT_CTX *sctx, const SCT *sct)
         CTerr(CT_F_SCT_VERIFY, CT_R_SCT_LOG_ID_MISMATCH);
         return 0;
     }
+
     ctx = EVP_MD_CTX_new();
     if (ctx == NULL)
         goto end;
@@ -203,7 +207,7 @@ int SCT_verify(const SCT_CTX *sctx, const SCT *sct)
     if (ret == 0)
         CTerr(CT_F_SCT_VERIFY, CT_R_SCT_INVALID_SIGNATURE);
 
- end:
+end:
     EVP_MD_CTX_free(ctx);
     return ret;
 }
@@ -214,14 +218,12 @@ int SCT_verify_v1(SCT *sct, X509 *cert, X509 *preissuer,
     int ret = 0;
     SCT_CTX *sctx = NULL;
 
-    if (sct == NULL || cert == NULL || log_pubkey == NULL ||
-        (sct->entry_type == CT_LOG_ENTRY_TYPE_PRECERT && issuer_cert == NULL)) {
-        CTerr(CT_F_SCT_VERIFY_V1, ERR_R_PASSED_NULL_PARAMETER);
-        return -1;
-    } else if (!SCT_is_complete(sct)) {
+    if (!SCT_is_complete(sct)) {
         CTerr(CT_F_SCT_VERIFY_V1, CT_R_SCT_NOT_SET);
         return -1;
-    } else if (sct->version != 0) {
+    }
+
+    if (sct->version != 0) {
         CTerr(CT_F_SCT_VERIFY_V1, CT_R_SCT_UNSUPPORTED_VERSION);
         return 0;
     }
@@ -231,12 +233,10 @@ int SCT_verify_v1(SCT *sct, X509 *cert, X509 *preissuer,
         goto done;
 
     ret = SCT_CTX_set1_pubkey(sctx, log_pubkey);
-
     if (ret <= 0)
         goto done;
 
     ret = SCT_CTX_set1_cert(sctx, cert, preissuer);
-
     if (ret <= 0)
         goto done;
 
@@ -248,8 +248,7 @@ int SCT_verify_v1(SCT *sct, X509 *cert, X509 *preissuer,
 
     ret = SCT_verify(sctx, sct);
 
- done:
-    if (sctx != NULL)
-        SCT_CTX_free(sctx);
+done:
+    SCT_CTX_free(sctx);
     return ret;
 }