From: tb <tb@openbsd.org>
Date: Wed, 31 Jan 2024 06:53:21 +0000 (+0000)
Subject: Pull mft comparison into proc_parser_mft_pre()
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=86eea840d5bb556b2429db8c813a33d1d2357546;p=openbsd

Pull mft comparison into proc_parser_mft_pre()

This way we can be sure more easily that both manifests are non-NULL,
thus avoiding some NULL checks and risk of use-after-free. This also
makes it clearer which manifest is the "older" one and will simplify
an upcoming commit doing issuance time comparison.

This adds a bit of a hack to proc_parser_mft_pre() to ensure we don't
look into DIR_TEMP in noop mode.

ok job
---

diff --git a/usr.sbin/rpki-client/parser.c b/usr.sbin/rpki-client/parser.c
index f8e4f95d163..3d89f9bf615 100644
--- a/usr.sbin/rpki-client/parser.c
+++ b/usr.sbin/rpki-client/parser.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: parser.c,v 1.110 2024/01/31 06:48:27 tb Exp $ */
+/*	$OpenBSD: parser.c,v 1.111 2024/01/31 06:53:21 tb Exp $ */
 /*
  * Copyright (c) 2019 Claudio Jeker <claudio@openbsd.org>
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
@@ -258,22 +258,28 @@ parse_load_crl_from_mft(struct entity *entp, struct mft *mft, enum location loc,
  */
 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		 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);
@@ -301,6 +307,26 @@ proc_parser_mft_pre(struct entity *entp, enum location loc, char **file,
 	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 has
+	 * a smaller sequence number.
+	 */
+
+	if ((seqnum_cmp = mft_compare(mft, cached_mft)) < 0) {
+		warnx("%s: unexpected manifest number (want >= #%s, got #%s)",
+		    *file, cached_mft->seqnum, mft->seqnum);
+		goto err;
+	}
+	if (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:
@@ -370,32 +396,22 @@ proc_parser_mft(struct entity *entp, struct mft **mp, char **crlfile,
 	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 (err1 != NULL)
 			err2 = err1;
 
-	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 (!noop && r == 1) {
+	if (!noop && mft1 != NULL) {
 		*mp = proc_parser_mft_post(file1, mft1, entp->path, err1,
 		    &warned);
 		if (*mp == NULL) {