On Fri, Apr 01, 2022 at 06:31:43PM +0200, Theo Buehler wrote:
> On Fri, Apr 01, 2022 at 05:01:00PM +0200, Claudio Jeker wrote:
> > cert_parse_inner() now only uses the ta flag to change behaviour of
> > loading the various x509 extensions (AKI, SKI, AIA und CRL DP).
> >
> > This diff changes these functions to work always. Make AKI, AIA and CRL DP
> > optional and have the code calling those functions check if they must have
> > the extension. I modelled the functions after x509_get_expire() so they
> > return 0 on failure and 1 on success.
>
> This looks pretty good.
>
> There is a certain asymmetry with the SKI handling that confused me a
> bit. At first, the strcmp(p->aki, p->ski) in ta_parse() and cert_parse()
> looked like potential NULL derefs since it looks as if p->ski isn't
> checked.
>
> The check for p.res->ski != NULL at the end of cert_parse_inner() is no
> longer necessary since x509_get_ski() would have failed. Strictly
> speaking, the ...->ski == NULL tests in {gbr,mft,roa}_parse() are also
> no longer needed.
>
> I would probably prefer to make x509_get_ski() behave the same way as
> the AKI, AIA, CRL getters and add a p->ski != NULL check to ta_parse()
> and cert_parse().
I think you're right, it may be better to behave the same for all of these
extensions.
> > if (p->aia == NULL) {
> > - warnx("%s: RFC 6487 section 8.4.7: "
> > - "non-trust anchor missing AIA", fn);
> > + warnx("%s: RFC 6487 section 8.4.7: AIA: extension missing", fn);
>
> The warnings in this function aren't super consistent. We can clean this
> up in a later pass.
I think this is a general concern for all of rpki-client. I switched the
text because it is shorter :) Also it was the warning that would have been
printed first.
> > @@ -360,7 +360,11 @@ proc_parser_mft_pre(char *file, const un
> >
> > a = valid_ski_aki(file, &auths, mft->ski, mft->aki);
> > /* load CRL by hand, since it is referenced by the MFT itself */
> > - c = x509_get_crl(x509, file);
> > + if (!x509_get_crl(x509, file, &c) || c == NULL) {
>
> The c == NULL case now fails silently. Previously this would have
> warned and crashed so it doesn't seem to occur in practice.
It could be triggered by a bad MFT cert. I added an error here.
> > + mft_free(mft);
> > + X509_free(x509);
> > + return NULL;
> > + }
> > crlfile = strrchr(c, '/');
> > if (crlfile != NULL)
> > crlfile++;
> > @@ -1078,7 +1082,7 @@ proc_parser_file(char *file, unsigned ch
> > struct crl *c;
> > char *crl_uri;
> >
> > - crl_uri = x509_get_crl(x509, file);
> > + x509_get_crl(x509, file, &crl_uri);
Here luckily it does not matter, the code handles NULL just fine and fails
on the verify because of the missing cert.
> > parse_load_crl(crl_uri);
> > free(crl_uri);
> > if (auth_find(&auths, aki) == NULL)
> > - aia = strndup(
> > + *aia = strndup(
> > ASN1_STRING_get0_data(ad->location->d.uniformResourceIdentifier),
> > ASN1_STRING_length(ad->location->d.uniformResourceIdentifier));
>
> Unrelated to your diff: I think we may want to ensure that the URI doesn't
> contain embedded NULs before calling strndup on it.
Maybe we need a function that does all this so it can be used in a few
additional places. I would suggest to tackle this as a seperate diff.
--
:wq Claudio
