From: Theo Buehler Subject: Re: rpki-client: check that the purported "new" manifest's thisUpdate is recent enough To: Job Snijders Cc: tech@openbsd.org Date: Tue, 30 Jan 2024 11:42:55 +0100 On Thu, Jan 18, 2024 at 11:19:15PM +0000, Job Snijders wrote: > RFC 9286 section 4.2.1 says: > > """ > Each RP MUST verify that this field value is greater (more recent) > than the most recent manifest it has validated. If this field in a > purported "new" manifest is smaller (less recent) than previously > validated manifests, the RP SHOULD use locally cached versions of > objects, as described in Section 6.6. > """ > > The below changeset adds a thisUpdate recency check to rpki-client. While this diff is correct, there are various things I don't like about it. Here's a step towards refactoring the complicated madness that is proc_parser_mft(). I can split out the issuance handling into a separate diff if that is preferred. The point is that the current handling attempts to treat the two manifests in a symmetric fashion, while there is no real symmetry. Pulling various checks into proc_parser_mft_pre() allows us to distinguish the manifest from the cache and the manifest pulled from the network. This also removes the gotchas with NULL filenames and risk of use after free. Apart from a hack for noop in proc_parser_mft_pre() (which I will clean up in a subsequent step), this adds a comparison function for the issuance time and turns mft_compare() (which was always a bit weird) into a normal comparison functions. The bulk of the patch adds (perhaps overly) detailed error reporting. There is a bug fix for the overloading of an error which went in the wrong direction after the meaning of mft1 and mft2 were changed in a recent commit. Index: extern.h =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v diff -u -p -r1.199 extern.h --- extern.h 18 Jan 2024 14:34:26 -0000 1.199 +++ extern.h 30 Jan 2024 10:19:26 -0000 @@ -629,7 +629,8 @@ void mft_free(struct mft *); struct mft *mft_parse(X509 **, const char *, int, const unsigned char *, size_t); struct mft *mft_read(struct ibuf *); -int mft_compare(const struct mft *, const struct mft *); +int mft_compare_issued(const struct mft *, const struct mft *); +int mft_compare_seqnum(const struct mft *, const struct mft *); void roa_buffer(struct ibuf *, const struct roa *); void roa_free(struct roa *); Index: mft.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v diff -u -p -r1.100 mft.c --- mft.c 11 Dec 2023 15:50:23 -0000 1.100 +++ mft.c 30 Jan 2024 10:19:26 -0000 @@ -544,20 +544,25 @@ mft_read(struct ibuf *b) return p; } +int +mft_compare_issued(const struct mft *a, const struct mft *b) +{ + if (a->thisupdate > b->thisupdate) + return 1; + if (a->thisupdate < b->thisupdate) + return -1; + return 0; +} + /* * Compare the manifestNumber of two MFT files. * Returns 1 if first MFT should be used, 0 if both are equal, and -1 if the * second MFT should be used. */ int -mft_compare(const struct mft *a, const struct mft *b) +mft_compare_seqnum(const struct mft *a, const struct mft *b) { int r; - - if (b == NULL) - return 1; - if (a == NULL) - return -1; r = strlen(a->seqnum) - strlen(b->seqnum); if (r > 0) /* seqnum in a is longer -> higher */ Index: parser.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v diff -u -p -r1.108 parser.c --- parser.c 18 Jan 2024 14:34:26 -0000 1.108 +++ parser.c 30 Jan 2024 10:19:26 -0000 @@ -258,22 +258,28 @@ parse_load_crl_from_mft(struct entity *e */ static struct mft * proc_parser_mft_pre(struct entity *entp, enum location loc, char **file, - struct crl **crl, char **crlfile, const char **errstr) + struct crl **crl, char **crlfile, struct mft *cached_mft, + const char **errstr) { struct mft *mft; X509 *x509; struct auth *a; unsigned char *der; size_t len; + int issued_cmp, seqnum_cmp; *crl = NULL; *crlfile = NULL; *errstr = NULL; + /* XXX - pull this into proc_parser_mft. */ *file = parse_filepath(entp->repoid, entp->path, entp->file, loc); if (*file == NULL) return NULL; + if (noop && loc == DIR_TEMP) + return NULL; + der = load_file(*file, &len); if (der == NULL && errno != ENOENT) warn("parse file %s", *file); @@ -293,21 +299,63 @@ proc_parser_mft_pre(struct entity *entp, *crl = parse_load_crl_from_mft(entp, mft, DIR_VALID, crlfile); a = valid_ski_aki(*file, &auths, mft->ski, mft->aki, NULL); - if (!valid_x509(*file, ctx, x509, a, *crl, errstr)) { - X509_free(x509); - mft_free(mft); - crl_free(*crl); - *crl = NULL; - free(*crlfile); - *crlfile = NULL; - return NULL; - } + if (!valid_x509(*file, ctx, x509, a, *crl, errstr)) + goto err; X509_free(x509); + x509 = NULL; mft->repoid = entp->repoid; mft->talid = a->cert->talid; + if (cached_mft == NULL) + return mft; + + /* + * Check that the cached manifest is older in the sense that it was + * issued earlier and that it has a smaller sequence number. + */ + + if ((issued_cmp = mft_compare_issued(mft, cached_mft)) < 0) { + warnx("%s: unexpected manifest issuance time (want >= %lld, " + "got %lld)", *file, (long long)cached_mft->thisupdate, + (long long)mft->thisupdate); + goto err; + } + if ((seqnum_cmp = mft_compare_seqnum(mft, cached_mft)) < 0) { + warnx("%s: unexpected manifest number (want >= #%s, got #%s)", + *file, cached_mft->seqnum, mft->seqnum); + goto err; + } + if (issued_cmp > 0 && seqnum_cmp == 0) { + warnx("%s#%s: reissued manifest at %lld and %lld with same " + "sequence number", *file, cached_mft->seqnum, + (long long)mft->thisupdate, + (long long)cached_mft->thisupdate); + goto err; + } + if (issued_cmp == 0 && seqnum_cmp > 0) { + warnx("%s#%s: reissued manifest same issuance time %lld as #%s", + *file, mft->seqnum, (long long)mft->thisupdate, + cached_mft->seqnum); + goto err; + } + if (issued_cmp == 0 && seqnum_cmp == 0 && memcmp(mft->mfthash, + cached_mft->mfthash, SHA256_DIGEST_LENGTH) != 0) { + warnx("%s: manifest misissuance, #%s was recycled", *file, + mft->seqnum); + goto err; + } + return mft; + + err: + X509_free(x509); + mft_free(mft); + crl_free(*crl); + *crl = NULL; + free(*crlfile); + *crlfile = NULL; + return NULL; } /* @@ -367,32 +415,22 @@ proc_parser_mft(struct entity *entp, str struct crl *crl, *crl1, *crl2; char *file, *file1, *file2, *crl1file, *crl2file; const char *err1, *err2; - int r, warned = 0; + int warned = 0; *mp = NULL; *crlmtime = 0; - mft1 = proc_parser_mft_pre(entp, DIR_TEMP, &file1, &crl1, &crl1file, - &err1); mft2 = proc_parser_mft_pre(entp, DIR_VALID, &file2, &crl2, &crl2file, - &err2); + NULL, &err2); + mft1 = proc_parser_mft_pre(entp, DIR_TEMP, &file1, &crl1, &crl1file, + mft2, &err1); /* overload error from temp file if it is set */ if (mft1 == NULL && mft2 == NULL) - if (err2 != NULL) - err1 = err2; - - r = mft_compare(mft1, mft2); - if (r == -1 && mft1 != NULL && mft2 != NULL) - warnx("%s: unexpected manifest number (want >= #%s, got #%s)", - file1, mft2->seqnum, mft1->seqnum); - - if (r == 0 && memcmp(mft1->mfthash, mft2->mfthash, - SHA256_DIGEST_LENGTH) != 0) - warnx("%s: manifest misissuance, #%s was recycled", - file1, mft1->seqnum); + if (err1 != NULL) + err2 = err1; - if (!noop && r == 1) { + if (!noop && mft1 != NULL) { *mp = proc_parser_mft_post(file1, mft1, entp->path, err1, &warned); if (*mp == NULL) {