From 2ce35f3ca5242cdb7eeb33345fa58e1268e76464 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 15 Jun 2016 01:37:43 -0400 Subject: [PATCH] delete signature validation state code The purpose of this code appears to be verification acceleration: avoid performing expensive crypto work when the object has already been verified. (The purpose isn't entirely clear due to lack of explanatory comments.) However: * It's unclear that this code actually improves performance in any perceptable way. First, it's unclear how often an object is added multiple times. Second, it's trading crypto work for database operations. * This code permits evil twin ROAs: A malicious CA can generate an evil ROA with an EE certificate containing the same SKI as a valid EE certificate. The malicious CA won't be able to produce a valid signature for the evil ROA, but that doesn't matter because this code shortcuts the signature check. Fortunately there's another bug preventing this from being exploitable (issue #55). --- bin/rpki/upgrade.in | 4 + lib/rpki/db_constants.h | 16 ---- lib/rpki/scmmain.h | 2 - lib/rpki/sqhl.c | 193 +--------------------------------------- 4 files changed, 6 insertions(+), 209 deletions(-) diff --git a/bin/rpki/upgrade.in b/bin/rpki/upgrade.in index a65b25d3..5f36b941 100644 --- a/bin/rpki/upgrade.in +++ b/bin/rpki/upgrade.in @@ -84,6 +84,10 @@ created. If you want to start sending data to routers again, run } upgrade_from_0_12 () { + mysql_cmd <<\EOF || fatal "failed to update database records" +ALTER TABLE `rpki_cert` DROP COLUMN `sigval`; +ALTER TABLE `rpki_roa` DROP COLUMN `sigval`; +EOF } upgrade_from_0_11 () { diff --git a/lib/rpki/db_constants.h b/lib/rpki/db_constants.h index 616878a8..3ebf134d 100644 --- a/lib/rpki/db_constants.h +++ b/lib/rpki/db_constants.h @@ -2,22 +2,6 @@ #define LIB_RPKI_DB_CONSTANTS_H -/** - * @brief - * Signature validation states - */ -typedef enum { - // Use a negative value to force sigval_state to be a signed type. - // This prevents ancient versions of gcc from complaining with - // "comparison of unsigned expression < 0 is always false" - SIGVAL_silence_bogus_gcc_warning = -1, - - SIGVAL_UNKNOWN = 0, - SIGVAL_NOTPRESENT, - SIGVAL_VALID, - SIGVAL_INVALID, -} sigval_state; - /* * Flags */ diff --git a/lib/rpki/scmmain.h b/lib/rpki/scmmain.h index 905c368d..ae064f0d 100644 --- a/lib/rpki/scmmain.h +++ b/lib/rpki/scmmain.h @@ -61,7 +61,6 @@ static scmtab scmtabbuilder[] = { "hash VARCHAR(256)," "valfrom DATETIME NOT NULL," "valto DATETIME NOT NULL," - "sigval INT UNSIGNED DEFAULT 0," "ipblen INT UNSIGNED DEFAULT 0," "ipb BLOB," "ts_mod TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP," @@ -126,7 +125,6 @@ static scmtab scmtabbuilder[] = { "dir_id INT UNSIGNED NOT NULL DEFAULT 1," "ski VARCHAR(128) NOT NULL," "sig VARCHAR(520) NOT NULL," - "sigval INT UNSIGNED DEFAULT 0," "hash VARCHAR(256)," "asn INT UNSIGNED NOT NULL," "flags INT UNSIGNED DEFAULT 0," diff --git a/lib/rpki/sqhl.c b/lib/rpki/sqhl.c index 42051bb2..16517f79 100644 --- a/lib/rpki/sqhl.c +++ b/lib/rpki/sqhl.c @@ -532,117 +532,6 @@ add_crl_internal( return (sta); } -/** - * @brief - * This function gets the sigval parameter from a table based on - * the type. - * - * @return - * One of the SIGVAL_ constants indicating what happened. - */ -static sigval_state -get_sigval( - scmcon *conp, - object_type typ, - const char *ski, - const char *subj) -{ - scmtab *table; - unsigned int val; - scmsrch search_cols[] = { - { - .colno = 1, - .sqltype = SQL_C_ULONG, - .colname = "sigval", - .valptr = &val, - .valsize = sizeof(val), - }, - }; - char where[WHERESTR_SIZE]; - if (OT_CER == typ) - { - xsnprintf(where, sizeof(where), - "ski=\"%s\" and subject=\"%s\"", ski, subj); - table = theCertTable; - } - else if (OT_ROA == typ) - { - xsnprintf(where, sizeof(where), "ski=\"%s\"", ski); - table = theROATable; - } - else - { - return SIGVAL_UNKNOWN; - } - scmsrcha sigsrch = { - .vec = search_cols, - .ntot = ELTS(search_cols), - .nused = ELTS(search_cols), - .wherestr = where, - }; - err_code sta = 0; - - if (theSCMP != NULL) - initTables(theSCMP); - sta = searchscm(conp, table, &sigsrch, NULL, &ok, - SCM_SRCH_DOVALUE_ALWAYS, NULL); - if (sta < 0) - return SIGVAL_UNKNOWN; - sigval_state sval = val; - if (sval < SIGVAL_UNKNOWN || sval > SIGVAL_INVALID) - return SIGVAL_UNKNOWN; - return sval; -} - -/** - * @brief - * This function attempts to set the sigval parameter in a table - * based on the type. - */ -static err_code -set_sigval( - scmcon *conp, - object_type typ, - const char *ski, - const char *subj, - sigval_state valu) -{ - /** @bug magic number */ - char stmt[520]; - err_code sta; - scmtab *table; - char where[sizeof(stmt)]; - - if (theSCMP != NULL) - initTables(theSCMP); - switch (typ) - { - case OT_CER: - { - table = theCertTable; - size_t subj_len = strlen(subj); - char escaped_subj[2 * subj_len + 1]; - mysql_escape_string(escaped_subj, subj, subj_len); - xsnprintf(where, sizeof(where), "ski=\"%s\" and subject=\"%s\"", - ski, escaped_subj); - break; - } - case OT_ROA: - table = theROATable; - xsnprintf(where, sizeof(where), "ski=\"%s\"", ski); - break; - default: - // other cases not handled yet - return ERR_SCM_UNSPECIFIED; - } - if (table == NULL) - return ERR_SCM_NOSUCHTAB; - xsnprintf(stmt, sizeof(stmt), "update %s set sigval=%d where %s;", - table->tabname, valu, where); - sta = statementscm_no_data(conp, stmt); - return sta; -} - /* * A verification function type */ @@ -658,67 +547,6 @@ vfunc( static vfunc *old_vfunc = NULL; static scmcon *thecon = NULL; -/* - * Our replacement for X509_verify. Consults the database first to see if the - * certificate is already valid, otherwise calls X509_verify and then sets the - * state in the db based on that. It returns 1 on success and 0 on failure. - */ - -static int local_verify( - X509 *cert, - EVP_PKEY *pkey) -{ - int x509sta = 0; - err_code sta = 0; - sigval_state sigval = SIGVAL_UNKNOWN; - int mok; - char *subj = NULL; - char *ski = NULL; - - // first, get the subject and the SKI - /** @bug ignores error code without explanation */ - subj = X509_to_subject(cert, &sta, &x509sta); - if (subj != NULL) - { - /** @bug ignores error code without explanation */ - ski = X509_to_ski(cert, &sta, &x509sta); - if (ski != NULL) - { - sigval = get_sigval(thecon, OT_CER, ski, subj); - } - } - switch (sigval) - { - case SIGVAL_VALID: /* already validated */ - if (subj != NULL) - free((void *)subj); - if (ski != NULL) - free((void *)ski); - return 1; - case SIGVAL_INVALID: /* already invalidated */ - if (subj != NULL) - free((void *)subj); - if (ski != NULL) - free((void *)ski); - return 0; - case SIGVAL_UNKNOWN: - case SIGVAL_NOTPRESENT: - default: - break; /* compute validity, then set in db */ - } - mok = X509_verify(cert, pkey); - if (mok) - { - /** @bug ignores error code without explanation */ - set_sigval(thecon, OT_CER, ski, subj, SIGVAL_VALID); - } - if (subj != NULL) - free((void *)subj); - if (ski != NULL) - free((void *)ski); - return mok; -} - /* * Our own internal verifier, replacing the internal_verify function in * openSSL (x509_vfy.c). It returns 1 on success and 0 on failure. @@ -774,7 +602,7 @@ our_verify( if (!mok) goto end; } - else if (local_verify(xsubject, pkey) <= 0) + else if (X509_verify(xsubject, pkey) <= 0) { ctx->error = X509_V_ERR_CERT_SIGNATURE_FAILURE; ctx->current_cert = xsubject; @@ -2030,17 +1858,8 @@ verify_roa( err_code sta = 0; X509 *cert; - sigval_state sigval; - // first, see if the ROA is already validated and in the DB - sigval = get_sigval(conp, OT_ROA, ski, NULL); - if (sigval == SIGVAL_VALID) - { - LOG(LOG_DEBUG, "ROA already verified; skipping checks"); - *chainOK = 1; - goto done; - } - // next call the syntactic verification + // call the syntactic verification sta = roaValidate(r); if (sta) { @@ -2069,14 +1888,6 @@ verify_roa( *chainOK = 1; sta = roaValidate2(r); X509_free(cert); - if (sta >= 0) - { - sta = set_sigval(conp, OT_ROA, ski, NULL, SIGVAL_VALID); - if (sta < 0) - LOG(LOG_ERR, - "could not set ROA sigval: conp->mystat.errmsg = %s", - conp->mystat.errmsg); - } sta = (sta < 0) ? sta : 0; done: