From 23bc08f8ba561b111db7061e6b2781cdaa1cabc9 Mon Sep 17 00:00:00 2001 From: claudio Date: Tue, 19 Apr 2022 09:52:29 +0000 Subject: [PATCH] Adjust on how CRL and MFT files are verified. Verify the CRL referenced from the mft against the mft's fileAndHash info. If the CRL matches then load it and use it to validate this mft. If the mft validated OK add the now also valid CRL to the auth store for later use. Before the newest CRL was always selected but that has negative consequences because it is common practice to revoke the previous MFT's EE cert and with that the cache is turned useless as soon as a new CRL is used. Also there was a possibility that the CRL used for validation of the MFT was not the one later used. Both RFC6486 and draft-ietf-sidrops-6486bis are unclear about this part of the validation process. We opted in favor of the chached MFT. With and OK tb@ --- usr.sbin/rpki-client/extern.h | 5 +- usr.sbin/rpki-client/main.c | 53 ++----- usr.sbin/rpki-client/mft.c | 40 +++++- usr.sbin/rpki-client/parser.c | 239 +++++++++++++++----------------- usr.sbin/rpki-client/validate.c | 23 ++- 5 files changed, 188 insertions(+), 172 deletions(-) diff --git a/usr.sbin/rpki-client/extern.h b/usr.sbin/rpki-client/extern.h index 13cfc262868..a887d320b48 100644 --- a/usr.sbin/rpki-client/extern.h +++ b/usr.sbin/rpki-client/extern.h @@ -1,4 +1,4 @@ -/* $OpenBSD: extern.h,v 1.126 2022/04/12 08:45:34 tb Exp $ */ +/* $OpenBSD: extern.h,v 1.127 2022/04/19 09:52:29 claudio Exp $ */ /* * Copyright (c) 2019 Kristaps Dzonsons * @@ -194,6 +194,8 @@ struct mft { char *aia; /* AIA */ char *aki; /* AKI */ char *ski; /* SKI */ + char *crl; /* CRL file name */ + unsigned char crlhash[SHA256_DIGEST_LENGTH]; time_t valid_from; time_t valid_until; size_t filesz; /* number of filenames */ @@ -463,6 +465,7 @@ int valid_ta(const char *, struct auth_tree *, int valid_cert(const char *, struct auth *, const struct cert *); int valid_roa(const char *, struct auth *, struct roa *); int valid_filehash(int, const char *, size_t); +int valid_hash(unsigned char *, size_t, const char *, size_t); int valid_uri(const char *, size_t, const char *); int valid_origin(const char *, const char *); diff --git a/usr.sbin/rpki-client/main.c b/usr.sbin/rpki-client/main.c index 8cbc30058c5..a9caa2bf215 100644 --- a/usr.sbin/rpki-client/main.c +++ b/usr.sbin/rpki-client/main.c @@ -1,4 +1,4 @@ -/* $OpenBSD: main.c,v 1.193 2022/04/11 18:59:23 claudio Exp $ */ +/* $OpenBSD: main.c,v 1.194 2022/04/19 09:52:29 claudio Exp $ */ /* * Copyright (c) 2021 Claudio Jeker * Copyright (c) 2019 Kristaps Dzonsons @@ -330,51 +330,21 @@ rrdp_http_done(unsigned int id, enum http_result res, const char *last_mod) * These are always relative to the directory in which "mft" sits. */ static void -queue_add_from_mft(const char *path, const struct mftfile *file, - struct repo *rp) -{ - char *nfile, *npath = NULL; - - if (path != NULL) - if ((npath = strdup(path)) == NULL) - err(1, NULL); - if ((nfile = strdup(file->file)) == NULL) - err(1, NULL); - - entityq_add(npath, nfile, file->type, file->location, rp, NULL, 0, -1); -} - -/* - * Loops over queue_add_from_mft() for all files. - * The order here is important: we want to parse the revocation - * list *before* we parse anything else. - */ -static void -queue_add_from_mft_set(const struct mft *mft, const char *name, struct repo *rp) +queue_add_from_mft(const struct mft *mft, struct repo *rp) { size_t i; const struct mftfile *f; + char *nfile, *npath = NULL; for (i = 0; i < mft->filesz; i++) { f = &mft->files[i]; - if (f->type != RTYPE_CRL) - continue; - queue_add_from_mft(mft->path, f, rp); - } - - for (i = 0; i < mft->filesz; i++) { - f = &mft->files[i]; - switch (f->type) { - case RTYPE_CER: - case RTYPE_ROA: - case RTYPE_GBR: - queue_add_from_mft(mft->path, f, rp); - break; - case RTYPE_CRL: - continue; - default: - warnx("%s: unsupported file: %s", name, f->file); - } + if (mft->path != NULL) + if ((npath = strdup(mft->path)) == NULL) + err(1, NULL); + if ((nfile = strdup(f->file)) == NULL) + err(1, NULL); + entityq_add(npath, nfile, f->type, f->location, rp, NULL, 0, + -1); } } @@ -553,8 +523,7 @@ entity_process(struct ibuf *b, struct stats *st, struct vrp_tree *tree, } mft = mft_read(b); if (!mft->stale) - queue_add_from_mft_set(mft, file, - repo_byid(mft->repoid)); + queue_add_from_mft(mft, repo_byid(mft->repoid)); else st->mfts_stale++; mft_free(mft); diff --git a/usr.sbin/rpki-client/mft.c b/usr.sbin/rpki-client/mft.c index d39d53688cc..2af058d8e0f 100644 --- a/usr.sbin/rpki-client/mft.c +++ b/usr.sbin/rpki-client/mft.c @@ -1,4 +1,4 @@ -/* $OpenBSD: mft.c,v 1.57 2022/04/11 10:03:12 claudio Exp $ */ +/* $OpenBSD: mft.c,v 1.58 2022/04/19 09:52:29 claudio Exp $ */ /* * Copyright (c) 2019 Kristaps Dzonsons * @@ -38,6 +38,7 @@ struct parse { const char *fn; /* manifest file name */ struct mft *res; /* result object */ + int found_crl; }; extern ASN1_OBJECT *mft_oid; @@ -180,6 +181,7 @@ mft_parse_filehash(struct parse *p, const ASN1_OCTET_STRING *os) size_t dsz = os->length; int rc = 0; struct mftfile *fent; + enum rtype type; if ((seq = d2i_ASN1_SEQUENCE_ANY(NULL, &d, dsz)) == NULL) { cryptowarnx("%s: RFC 6486 section 4.2.1: FileAndHash: " @@ -228,10 +230,17 @@ mft_parse_filehash(struct parse *p, const ASN1_OCTET_STRING *os) goto out; } + type = rtype_from_mftfile(fn); + /* remember the filehash for the CRL in struct mft */ + if (type == RTYPE_CRL && strcmp(fn, p->res->crl) == 0) { + memcpy(p->res->crlhash, hash->value.bit_string->data, + SHA256_DIGEST_LENGTH); + p->found_crl = 1; + } + /* Insert the filename and hash value. */ fent = &p->res->files[p->res->filesz++]; - - fent->type = rtype_from_mftfile(fn); + fent->type = type; fent->file = fn; fn = NULL; memcpy(fent->hash, hash->value.bit_string->data, SHA256_DIGEST_LENGTH); @@ -283,6 +292,11 @@ mft_parse_flist(struct parse *p, const ASN1_OCTET_STRING *os) goto out; } + if (!p->found_crl) { + warnx("%s: CRL not part of MFT fileList", p->fn); + goto out; + } + rc = 1; out: sk_ASN1_TYPE_pop_free(seq, ASN1_TYPE_free); @@ -432,6 +446,7 @@ mft_parse(X509 **x509, const char *fn, const unsigned char *der, size_t len) int rc = 0; size_t cmsz; unsigned char *cms; + char *crldp, *crlfile; memset(&p, 0, sizeof(struct parse)); p.fn = fn; @@ -456,6 +471,25 @@ mft_parse(X509 **x509, const char *fn, const unsigned char *der, size_t len) goto out; } + /* get CRL info for later */ + if (!x509_get_crl(*x509, fn, &crldp)) + goto out; + if (crldp == NULL) { + warnx("%s: RFC 6487 section 4.8.6: CRL: " + "missing CRL distribution point extension", fn); + goto out; + } + if ((crlfile = strrchr(crldp, '/')) == NULL || + !valid_filename(crlfile + 1, strlen(crlfile + 1)) || + rtype_from_file_extension(crlfile + 1) != RTYPE_CRL) { + warnx("%s: RFC 6487 section 4.8.6: CRL: " + "bad CRL distribution point extension", fn); + goto out; + } + if ((p.res->crl = strdup(crlfile + 1)) == NULL) + err(1, NULL); + free(crldp); + if (mft_parse_econtent(cms, cmsz, &p) == 0) goto out; diff --git a/usr.sbin/rpki-client/parser.c b/usr.sbin/rpki-client/parser.c index 8c243cf2c25..07b5bbd885d 100644 --- a/usr.sbin/rpki-client/parser.c +++ b/usr.sbin/rpki-client/parser.c @@ -1,4 +1,4 @@ -/* $OpenBSD: parser.c,v 1.67 2022/04/11 18:59:23 claudio Exp $ */ +/* $OpenBSD: parser.c,v 1.68 2022/04/19 09:52:29 claudio Exp $ */ /* * Copyright (c) 2019 Claudio Jeker * Copyright (c) 2019 Kristaps Dzonsons @@ -42,7 +42,6 @@ static void build_chain(const struct auth *, STACK_OF(X509) **); static struct crl *get_crl(const struct auth *); static void build_crls(const struct crl *, STACK_OF(X509_CRL) **); -static struct crl *parse_load_crl_from_mft(struct entity *, const char *); static X509_STORE_CTX *ctx; static struct auth_tree auths = RB_INITIALIZER(&auths); @@ -341,6 +340,45 @@ proc_parser_mft_check(const char *fn, struct mft *p) return rc; } +/* + * Load the correct CRL using the info from the MFT. + */ +static struct crl * +parse_load_crl_from_mft(struct entity *entp, struct mft *mft, enum location loc) +{ + struct crl *crl = NULL; + unsigned char *f = NULL; + char *fn = NULL; + size_t flen; + + while (1) { + fn = parse_filepath(entp->repoid, entp->path, mft->crl, loc); + if (fn == NULL) + goto next; + + f = load_file(fn, &flen); + if (f == NULL && errno != ENOENT) + warn("parse file %s", fn); + if (f == NULL) + goto next; + if (!valid_hash(f, flen, mft->crlhash, sizeof(mft->crlhash))) + goto next; + crl = crl_parse(fn, f, flen); +next: + free(f); + free(fn); + f = NULL; + fn = NULL; + + if (crl != NULL) + return crl; + if (loc == DIR_TEMP) + loc = DIR_VALID; + else + return NULL; + } +} + /* * Parse and validate a manifest file. Skip checking the fileandhash * this is done in the post check. After this step we know the mft is @@ -349,44 +387,28 @@ proc_parser_mft_check(const char *fn, struct mft *p) */ static struct mft * proc_parser_mft_pre(char *file, const unsigned char *der, size_t len, - struct entity *entp) + struct entity *entp, enum location loc, struct crl **crl) { struct mft *mft; X509 *x509; - struct crl *crl; struct auth *a; - char *c, *crlfile; + *crl = NULL; if ((mft = mft_parse(&x509, file, der, len)) == NULL) return NULL; + *crl = parse_load_crl_from_mft(entp, mft, loc); a = valid_ski_aki(file, &auths, mft->ski, mft->aki); - /* load CRL by hand, since it is referenced by the MFT itself */ - if (!x509_get_crl(x509, file, &c) || c == NULL) { - if (c == NULL) - warnx("%s: RFC 6487 section 4.8.6: CRL: " - "no CRL distribution point extension", file); - mft_free(mft); + if (!valid_x509(file, x509, a, *crl, 1)) { X509_free(x509); - return NULL; - } - crlfile = strrchr(c, '/'); - if (crlfile != NULL) - crlfile++; - else - crlfile = c; - crl = parse_load_crl_from_mft(entp, crlfile); - free(c); - - if (!valid_x509(file, x509, a, crl, 1)) { mft_free(mft); - crl_free(crl); - X509_free(x509); + crl_free(*crl); + *crl = NULL; return NULL; } - crl_free(crl); X509_free(x509); + mft->repoid = entp->repoid; return mft; } @@ -395,8 +417,7 @@ proc_parser_mft_pre(char *file, const unsigned char *der, size_t len, * Return the mft on success or NULL on failure. */ static struct mft * -proc_parser_mft_post(char *file, struct mft *mft, const char *path, - unsigned int repoid) +proc_parser_mft_post(char *file, struct mft *mft, const char *path) { /* check that now is not before from */ time_t now = time(NULL); @@ -419,7 +440,6 @@ proc_parser_mft_post(char *file, struct mft *mft, const char *path, mft->stale = 1; } - mft->repoid = repoid; if (path != NULL) if ((mft->path = strdup(path)) == NULL) err(1, NULL); @@ -433,6 +453,65 @@ proc_parser_mft_post(char *file, struct mft *mft, const char *path, return mft; } +/* + * Load the most recent MFT by opening both options and comparing the two. + */ +static char * +proc_parser_mft(struct entity *entp, struct mft **mp) +{ + struct mft *mft1 = NULL, *mft2 = NULL; + struct crl *crl, *crl1 = NULL, *crl2 = NULL; + char *f, *file, *file1, *file2; + size_t flen; + + *mp = NULL; + file1 = parse_filepath(entp->repoid, entp->path, entp->file, DIR_VALID); + file2 = parse_filepath(entp->repoid, entp->path, entp->file, DIR_TEMP); + + if (file1 != NULL) { + f = load_file(file1, &flen); + if (f == NULL && errno != ENOENT) + warn("parse file %s", file1); + mft1 = proc_parser_mft_pre(file1, f, flen, entp, DIR_VALID, + &crl1); + free(f); + } + if (file2 != NULL) { + f = load_file(file2, &flen); + if (f == NULL && errno != ENOENT) + warn("parse file %s", file2); + mft2 = proc_parser_mft_pre(file2, f, flen, entp, DIR_TEMP, + &crl2); + free(f); + } + + if (mft_compare(mft1, mft2) == 1) { + mft_free(mft2); + crl_free(crl2); + free(file2); + *mp = proc_parser_mft_post(file1, mft1, entp->path); + crl = crl1; + file = file1; + } else { + mft_free(mft1); + crl_free(crl1); + free(file1); + *mp = proc_parser_mft_post(file2, mft2, entp->path); + crl = crl2; + file = file2; + } + + if (*mp != NULL) { + if (RB_INSERT(crl_tree, &crlt, crl) != NULL) { + warnx("%s: duplicate AKI %s", file, crl->aki); + crl_free(crl); + } + } else { + crl_free(crl); + } + return file; +} + /* * Validate a certificate, if invalid free the resouces and return NULL. */ @@ -650,96 +729,6 @@ parse_load_file(struct entity *entp, unsigned char **f, size_t *flen) return file; } -/* - * Load the most recent MFT by opening both options and comparing the two. - */ -static char * -parse_load_mft(struct entity *entp, struct mft **mft) -{ - struct mft *mft1 = NULL, *mft2 = NULL; - char *f, *file1, *file2; - size_t flen; - - file1 = parse_filepath(entp->repoid, entp->path, entp->file, DIR_VALID); - file2 = parse_filepath(entp->repoid, entp->path, entp->file, DIR_TEMP); - - if (file1 != NULL) { - f = load_file(file1, &flen); - if (f == NULL && errno != ENOENT) - warn("parse file %s", file1); - mft1 = proc_parser_mft_pre(file1, f, flen, entp); - free(f); - } - - if (file2 != NULL) { - f = load_file(file2, &flen); - if (f == NULL && errno != ENOENT) - warn("parse file %s", file2); - mft2 = proc_parser_mft_pre(file2, f, flen, entp); - free(f); - } - - if (mft_compare(mft1, mft2) == 1) { - mft_free(mft2); - free(file2); - *mft = mft1; - return file1; - } else { - mft_free(mft1); - free(file1); - *mft = mft2; - return file2; - } -} - -/* - * Load the most recent CRL by opening both options and comparing the two. - */ -static struct crl * -parse_load_crl_from_mft(struct entity *entp, const char *file) -{ - struct crl *crl1 = NULL, *crl2 = NULL; - char *file1, *file2; - unsigned char *f; - size_t flen; - - if (file == NULL) - return NULL; - - file1 = parse_filepath(entp->repoid, entp->path, file, DIR_VALID); - file2 = parse_filepath(entp->repoid, entp->path, file, DIR_TEMP); - - if (file1 != NULL) { - f = load_file(file1, &flen); - if (f == NULL && errno != ENOENT) - warn("parse file %s", file1); - crl1 = crl_parse(file1, f, flen); - free(f); - } - - if (file2 != NULL) { - f = load_file(file2, &flen); - if (f == NULL && errno != ENOENT) - warn("parse file %s", file2); - crl2 = crl_parse(file2, f, flen); - free(f); - } - - free(file1); - free(file2); - - if (crl1 == NULL || crl2 == NULL) - return crl2 == NULL ? crl1 : crl2; - - if (crl1->issued >= crl2->issued) { - crl_free(crl2); - return crl1; - } else { - crl_free(crl1); - return crl2; - } -} - /* * Process an entity and responing to parent process. */ @@ -804,16 +793,16 @@ parse_entity(struct entityq *q, struct msgbuf *msgq) */ break; case RTYPE_CRL: - file = parse_load_file(entp, &f, &flen); + /* + * CRLs are already loaded with the MFT so nothing + * really needs to be done here. + */ + file = parse_filepath(entp->repoid, entp->path, + entp->file, entp->location); io_str_buffer(b, file); - proc_parser_crl(file, f, flen); break; case RTYPE_MFT: - file = parse_load_mft(entp, &mft); - - mft = proc_parser_mft_post(file, mft, - entp->path, entp->repoid); - + file = proc_parser_mft(entp, &mft); io_str_buffer(b, file); c = (mft != NULL); io_simple_buffer(b, &c, sizeof(int)); diff --git a/usr.sbin/rpki-client/validate.c b/usr.sbin/rpki-client/validate.c index adc200c7a8e..93eead135e2 100644 --- a/usr.sbin/rpki-client/validate.c +++ b/usr.sbin/rpki-client/validate.c @@ -1,4 +1,4 @@ -/* $OpenBSD: validate.c,v 1.29 2022/02/04 13:50:32 job Exp $ */ +/* $OpenBSD: validate.c,v 1.30 2022/04/19 09:52:29 claudio Exp $ */ /* * Copyright (c) 2019 Kristaps Dzonsons * @@ -247,7 +247,28 @@ valid_filehash(int fd, const char *hash, size_t hlen) if (memcmp(hash, filehash, sizeof(filehash)) != 0) return 0; + return 1; +} + +/* + * Same as above but with a buffer instead of a fd. + */ +int +valid_hash(unsigned char *buf, size_t len, const char *hash, size_t hlen) +{ + char filehash[SHA256_DIGEST_LENGTH]; + if (hlen != sizeof(filehash)) + errx(1, "bad hash size"); + + if (buf == NULL || len == 0) + return 0; + + if (!EVP_Digest(buf, len, filehash, NULL, EVP_sha256(), NULL)) + errx(1, "EVP_Digest failed"); + + if (memcmp(hash, filehash, sizeof(filehash)) != 0) + return 0; return 1; } -- 2.20.1