On Thu, Jan 06, 2022 at 03:30:33PM +0100, Claudio Jeker wrote:
> On Thu, Jan 06, 2022 at 01:48:01PM +0100, Theo Buehler wrote:
> > On Thu, Jan 06, 2022 at 01:21:03PM +0100, Claudio Jeker wrote:
> > > Ran into this the other day and could not help myself to adjust the code.
> > > If the mft is stale just bump the stale counter and be done. If not stale
> > > queue all files from the mft for the next round.
> > >
> > > In mft_parse I switched to a switch statement which is more obvious in my
> > > opinion.
> >
> > I'm ok with this.
> >
> > Have you considered setting this flag directly on inspecting
> > check_validity()'s return value instead of bubbling rc = 0 up to
> > mft_parse()? This should allow us to get rid of the "Reset rc"
> > dance in mft_parse_econtent() and it could have a two-valued error.
With "it" I meant mft_parse_econtent()...
> I'm not sure if setting stale in check_validity() is that much better.
> If we do that check_validity() needs to return success for stale mfts
> which also feels wrong.
Agree totally.
> Also fun fact right now stale mft skip mft_parse_flist() so there was
> never a list of files present in the first place.
>
> How about this version which pulls the change of flag up into
> mft_parse_econtent() which makes that function only fail or succeed.
> I left check_validity() alone, it has currently no access to the parse
> context. Another big change is that stale mft call now
> mft_parse_flist() and also verify the hashes of all files in them.
Apart form the last big change and all the other cleanup, that's exactly
what I meant. I like this a lot better.
ok tb
>
> I sneaked in some additional cleanup -- like using valid_filename instead
> of the basic checks done there.
> --
> :wq Claudio
>
>
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.171
> diff -u -p -r1.171 main.c
> --- main.c 4 Jan 2022 18:41:32 -0000 1.171
> +++ main.c 6 Jan 2022 11:41:26 -0000
> @@ -518,9 +518,10 @@ entity_process(struct ibuf *b, struct st
> break;
> }
> mft = mft_read(b);
> - if (mft->stale)
> + if (!mft->stale)
> + queue_add_from_mft_set(mft);
> + else
> st->mfts_stale++;
> - queue_add_from_mft_set(mft);
> mft_free(mft);
> break;
> case RTYPE_CRL:
> Index: mft.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 mft.c
> --- mft.c 28 Oct 2021 13:51:42 -0000 1.42
> +++ mft.c 6 Jan 2022 14:19:40 -0000
> @@ -161,20 +161,8 @@ mft_parse_filehash(struct parse *p, cons
> if (fn == NULL)
> err(1, NULL);
>
> - /*
> - * Make sure we're just a pathname and either an ROA or CER.
> - * I don't think that the RFC specifically mentions this, but
> - * it's in practical use and would really screw things up
> - * (arbitrary filenames) otherwise.
> - */
> -
> - if (strchr(fn, '/') != NULL) {
> - warnx("%s: path components disallowed in filename: %s",
> - p->fn, fn);
> - goto out;
> - } else if (strlen(fn) <= 4) {
> - warnx("%s: filename must be large enough for suffix part: %s",
> - p->fn, fn);
> + if (!valid_filename(fn)) {
> + warnx("%s: invalid filename: %s", p->fn, fn);
> goto out;
> }
>
> @@ -257,7 +245,7 @@ mft_parse_flist(struct parse *p, const A
>
> /*
> * Handle the eContent of the manifest object, RFC 6486 sec. 4.2.
> - * Returns <0 on failure, 0 on stale, >0 on success.
> + * Returns 0 on failure and 1 on success.
> */
> static int
> mft_parse_econtent(const unsigned char *d, size_t dsz, struct parse *p)
> @@ -267,7 +255,7 @@ mft_parse_econtent(const unsigned char *
> const ASN1_GENERALIZEDTIME *from, *until;
> long mft_version;
> BIGNUM *mft_seqnum = NULL;
> - int i = 0, rc = -1;
> + int i = 0, rc = 0;
>
> if ((seq = d2i_ASN1_SEQUENCE_ANY(NULL, &d, dsz)) == NULL) {
> cryptowarnx("%s: RFC 6486 section 4.2: Manifest: "
> @@ -366,22 +354,26 @@ mft_parse_econtent(const unsigned char *
> }
> until = t->value.generalizedtime;
>
> - rc = check_validity(from, until, p->fn);
> - if (rc != 1)
> + switch (check_validity(from, until, p->fn)) {
> + case 0:
> + p->res->stale = 1;
> + /* FALLTHROUGH */
> + case 1:
> + break;
> + case -1:
> goto out;
> -
> - /* The mft is valid. Reset rc so later 'goto out' return failure. */
> - rc = -1;
> + }
>
> /* File list algorithm. */
>
> t = sk_ASN1_TYPE_value(seq, i++);
> if (t->type != V_ASN1_OBJECT) {
> warnx("%s: RFC 6486 section 4.2.1: fileHashAlg: "
> - "want ASN.1 object time, have %s (NID %d)",
> + "want ASN.1 object, have %s (NID %d)",
> p->fn, ASN1_tag2str(t->type), t->type);
> goto out;
> - } else if (OBJ_obj2nid(t->value.object) != NID_sha256) {
> + }
> + if (OBJ_obj2nid(t->value.object) != NID_sha256) {
> warnx("%s: RFC 6486 section 4.2.1: fileHashAlg: "
> "want SHA256 object, have %s (NID %d)", p->fn,
> ASN1_tag2str(OBJ_obj2nid(t->value.object)),
> @@ -397,7 +389,9 @@ mft_parse_econtent(const unsigned char *
> "want ASN.1 sequence, have %s (NID %d)",
> p->fn, ASN1_tag2str(t->type), t->type);
> goto out;
> - } else if (!mft_parse_flist(p, t->value.octet_string))
> + }
> +
> + if (!mft_parse_flist(p, t->value.octet_string))
> goto out;
>
> rc = 1;
> @@ -418,8 +412,8 @@ struct mft *
> mft_parse(X509 **x509, const char *fn, const unsigned char *der, size_t len)
> {
> struct parse p;
> - int c, rc = 0;
> - size_t i, cmsz;
> + int rc = 0;
> + size_t cmsz;
> unsigned char *cms;
>
> memset(&p, 0, sizeof(struct parse));
> @@ -451,27 +445,7 @@ mft_parse(X509 **x509, const char *fn, c
> goto out;
> }
>
> - /*
> - * If we're stale, then remove all of the files that the MFT
> - * references as well as marking it as stale.
> - */
> -
> - if ((c = mft_parse_econtent(cms, cmsz, &p)) == 0) {
> - /*
> - * FIXME: it should suffice to just mark this as stale
> - * and have the logic around mft_read() simply ignore
> - * the contents of stale entries, just like it does for
> - * invalid ROAs or certificates.
> - */
> -
> - p.res->stale = 1;
> - if (p.res->files != NULL)
> - for (i = 0; i < p.res->filesz; i++)
> - free(p.res->files[i].file);
> - free(p.res->files);
> - p.res->filesz = 0;
> - p.res->files = NULL;
> - } else if (c == -1)
> + if (mft_parse_econtent(cms, cmsz, &p) == 0)
> goto out;
>
> rc = 1;
