From: Job Snijders <job@openbsd.org>
Subject: rpki-client: check that the purported "new" manifest's thisUpdate is recent enough
To: tech@openbsd.org
Date: Thu, 18 Jan 2024 23:19:15 +0000

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.

The memcmp() is needed because the thisUpdate recency check shouldn't be
executed if the file didn't change (a same file could end up in DIR_TEMP
when a switchover from RRDP to RSYNC happened and the remote publication
point doesn't normalize the timestamps draft-ietf-sidrops-cms-signing-time)

OK?

Kind regards,

Job

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	18 Jan 2024 23:12:56 -0000
@@ -377,6 +377,23 @@ proc_parser_mft(struct entity *entp, str
 	mft2 = proc_parser_mft_pre(entp, DIR_VALID, &file2, &crl2, &crl2file,
 	    &err2);
 
+	if (mft1 != NULL && mft2 != NULL &&
+	    memcmp(mft1->mfthash, mft2->mfthash, SHA256_DIGEST_LENGTH) != 0 &&
+	    mft1->thisupdate <= mft2->thisupdate) {
+		warnx("%s: unexpected thisUpdate (want > %lld, got %lld), "
+		    "continuing with #%s from cache", file2,
+		    (long long)mft2->thisupdate, (long long)mft1->thisupdate,
+		    mft2->seqnum);
+		mft_free(mft1);
+		mft1 = NULL;
+		crl_free(crl1);
+		crl1 = NULL;
+		free(crl1file);
+		crl1file = NULL;
+		free(file1);
+		file1 = NULL;
+	}
+
 	/* overload error from temp file if it is set */
 	if (mft1 == NULL && mft2 == NULL)
 		if (err2 != NULL)