Download raw body.
rpki-client: check that the purported "new" manifest's thisUpdate is recent enough
rpki-client: check that the purported "new" manifest's thisUpdate is recent enough
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) {
rpki-client: check that the purported "new" manifest's thisUpdate is recent enough
rpki-client: check that the purported "new" manifest's thisUpdate is recent enough