From: claudio Date: Tue, 18 Jan 2022 13:46:07 +0000 (+0000) Subject: Unify the various X509_verify_cert() calls and the boiler plate code around X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=0e31088cd62dd2ade2468b503ff2eb792b6e8c88;p=openbsd Unify the various X509_verify_cert() calls and the boiler plate code around it into its own function valid_x509(). Simplifies the code substantially. This may report a few more errors for .roa and .gbr files but IMO that special case was a left-over from long time ago. OK tb@ --- diff --git a/usr.sbin/rpki-client/parser.c b/usr.sbin/rpki-client/parser.c index bf3e25d27a1..88fda210e7a 100644 --- a/usr.sbin/rpki-client/parser.c +++ b/usr.sbin/rpki-client/parser.c @@ -1,4 +1,4 @@ -/* $OpenBSD: parser.c,v 1.38 2022/01/18 13:06:43 claudio Exp $ */ +/* $OpenBSD: parser.c,v 1.39 2022/01/18 13:46:07 claudio Exp $ */ /* * Copyright (c) 2019 Claudio Jeker * Copyright (c) 2019 Kristaps Dzonsons @@ -200,52 +200,74 @@ verify_cb(int ok, X509_STORE_CTX *store_ctx) } /* - * Parse and validate a ROA. - * This is standard stuff. - * Returns the roa on success, NULL on failure. + * Validate the X509 certificate. If crl is NULL don't check CRL. + * Returns 1 for valid certificates, returns 0 if there is a verify error */ -static struct roa * -proc_parser_roa(char *file, const unsigned char *der, size_t len) +static int +valid_x509(char *file, X509 *x509, struct auth *a, struct crl *crl) { - struct roa *roa; - X509 *x509; - int c; - struct auth *a; STACK_OF(X509) *chain; - STACK_OF(X509_CRL) *crls; - struct crl *crl; - - if ((roa = roa_parse(&x509, file, der, len)) == NULL) - return NULL; + STACK_OF(X509_CRL) *crls = NULL; + int c; - a = valid_ski_aki(file, &auths, roa->ski, roa->aki); build_chain(a, &chain); - crl = get_crl(a); - build_crls(crl, &crls); + if (crl != NULL) + build_crls(crl, &crls); assert(x509 != NULL); if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL)) cryptoerrx("X509_STORE_CTX_init"); + X509_STORE_CTX_set_verify_cb(ctx, verify_cb); if (!X509_STORE_CTX_set_app_data(ctx, file)) cryptoerrx("X509_STORE_CTX_set_app_data"); - X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK); + if (crl != NULL) + X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK); X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH); X509_STORE_CTX_set0_trusted_stack(ctx, chain); - X509_STORE_CTX_set0_crls(ctx, crls); + if (crl != NULL) + X509_STORE_CTX_set0_crls(ctx, crls); if (X509_verify_cert(ctx) <= 0) { c = X509_STORE_CTX_get_error(ctx); + warnx("%s: %s", file, X509_verify_cert_error_string(c)); X509_STORE_CTX_cleanup(ctx); - if (verbose > 0 || c != X509_V_ERR_UNABLE_TO_GET_CRL) - warnx("%s: %s", file, X509_verify_cert_error_string(c)); - X509_free(x509); - roa_free(roa); sk_X509_free(chain); sk_X509_CRL_free(crls); - return NULL; + return 0; } + X509_STORE_CTX_cleanup(ctx); + sk_X509_free(chain); + sk_X509_CRL_free(crls); + return 1; +} + +/* + * Parse and validate a ROA. + * This is standard stuff. + * Returns the roa on success, NULL on failure. + */ +static struct roa * +proc_parser_roa(char *file, const unsigned char *der, size_t len) +{ + struct roa *roa; + struct crl *crl; + struct auth *a; + X509 *x509; + + if ((roa = roa_parse(&x509, file, der, len)) == NULL) + return NULL; + + a = valid_ski_aki(file, &auths, roa->ski, roa->aki); + crl = get_crl(a); + + if (!valid_x509(file, x509, a, crl)) { + X509_free(x509); + roa_free(roa); + return NULL; + } + X509_free(x509); /* * Check CRL to figure out the soonest transitive expiry moment @@ -270,10 +292,6 @@ proc_parser_roa(char *file, const unsigned char *der, size_t len) if (valid_roa(file, &auths, roa)) roa->valid = 1; - sk_X509_free(chain); - sk_X509_CRL_free(crls); - X509_free(x509); - return roa; } @@ -336,38 +354,18 @@ proc_parser_mft(char *file, const unsigned char *der, size_t len, { struct mft *mft; X509 *x509; - int c; struct auth *a; - STACK_OF(X509) *chain; if ((mft = mft_parse(&x509, file, der, len)) == NULL) return NULL; a = valid_ski_aki(file, &auths, mft->ski, mft->aki); - build_chain(a, &chain); - - if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL)) - cryptoerrx("X509_STORE_CTX_init"); - - /* CRL checks disabled here because CRL is referenced from mft */ - X509_STORE_CTX_set_verify_cb(ctx, verify_cb); - if (!X509_STORE_CTX_set_app_data(ctx, file)) - cryptoerrx("X509_STORE_CTX_set_app_data"); - X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH); - X509_STORE_CTX_set0_trusted_stack(ctx, chain); - if (X509_verify_cert(ctx) <= 0) { - c = X509_STORE_CTX_get_error(ctx); - X509_STORE_CTX_cleanup(ctx); - warnx("%s: %s", file, X509_verify_cert_error_string(c)); + if (!valid_x509(file, x509, a, NULL)) { mft_free(mft); X509_free(x509); - sk_X509_free(chain); return NULL; } - - X509_STORE_CTX_cleanup(ctx); - sk_X509_free(chain); X509_free(x509); mft->repoid = repoid; @@ -396,10 +394,8 @@ proc_parser_cert(char *file, const unsigned char *der, size_t len) { struct cert *cert; X509 *x509; - int c; - struct auth *a = NULL; - STACK_OF(X509) *chain; - STACK_OF(X509_CRL) *crls; + struct auth *a; + struct crl *crl; /* Extract certificate data and X509. */ @@ -408,35 +404,13 @@ proc_parser_cert(char *file, const unsigned char *der, size_t len) return NULL; a = valid_ski_aki(file, &auths, cert->ski, cert->aki); - build_chain(a, &chain); - build_crls(get_crl(a), &crls); - - assert(x509 != NULL); - if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL)) - cryptoerrx("X509_STORE_CTX_init"); - - X509_STORE_CTX_set_verify_cb(ctx, verify_cb); - if (!X509_STORE_CTX_set_app_data(ctx, file)) - cryptoerrx("X509_STORE_CTX_set_app_data"); - X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK); - X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH); - X509_STORE_CTX_set0_trusted_stack(ctx, chain); - X509_STORE_CTX_set0_crls(ctx, crls); + crl = get_crl(a); - if (X509_verify_cert(ctx) <= 0) { - c = X509_STORE_CTX_get_error(ctx); - warnx("%s: %s", file, X509_verify_cert_error_string(c)); - X509_STORE_CTX_cleanup(ctx); + if (!valid_x509(file, x509, a, crl)) { cert_free(cert); - sk_X509_free(chain); - sk_X509_CRL_free(crls); X509_free(x509); return NULL; } - - X509_STORE_CTX_cleanup(ctx); - sk_X509_free(chain); - sk_X509_CRL_free(crls); X509_free(x509); cert->talid = a->cert->talid; @@ -597,39 +571,18 @@ proc_parser_gbr(char *file, const unsigned char *der, size_t len) { struct gbr *gbr; X509 *x509; - int c; struct auth *a; - STACK_OF(X509) *chain; - STACK_OF(X509_CRL) *crls; + struct crl *crl; if ((gbr = gbr_parse(&x509, file, der, len)) == NULL) return; a = valid_ski_aki(file, &auths, gbr->ski, gbr->aki); + crl = get_crl(a); - build_chain(a, &chain); - build_crls(get_crl(a), &crls); - - assert(x509 != NULL); - if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL)) - cryptoerrx("X509_STORE_CTX_init"); - X509_STORE_CTX_set_verify_cb(ctx, verify_cb); - if (!X509_STORE_CTX_set_app_data(ctx, file)) - cryptoerrx("X509_STORE_CTX_set_app_data"); - X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK); - X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH); - X509_STORE_CTX_set0_trusted_stack(ctx, chain); - X509_STORE_CTX_set0_crls(ctx, crls); - - if (X509_verify_cert(ctx) <= 0) { - c = X509_STORE_CTX_get_error(ctx); - if (verbose > 0 || c != X509_V_ERR_UNABLE_TO_GET_CRL) - warnx("%s: %s", file, X509_verify_cert_error_string(c)); - } + /* return value can be ignored since nothing happens here */ + valid_x509(file, x509, a, crl); - X509_STORE_CTX_cleanup(ctx); - sk_X509_free(chain); - sk_X509_CRL_free(crls); X509_free(x509); gbr_free(gbr); }