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