On Fri, Apr 01, 2022 at 06:52:48PM +0200, Claudio Jeker wrote:
> 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.
>
And here is the updated diff
--
:wq Claudio
Index: cert.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.58
diff -u -p -r1.58 cert.c
--- cert.c 1 Apr 2022 13:27:38 -0000 1.58
+++ cert.c 1 Apr 2022 16:40:17 -0000
@@ -1052,13 +1052,10 @@ certificate_policies(struct parse *p, X5
/*
* Parse and partially validate an RPKI X509 certificate (either a trust
* anchor or a certificate) as defined in RFC 6487.
- * If "ta" is set, this is a trust anchor and must be self-signed.
- * Returns the parse results or NULL on failure ("xp" will be NULL too).
- * On success, free the pointer with cert_free() and make sure that "xp"
- * is also dereferenced.
+ * Returns the parse results or NULL on failure.
*/
static struct cert *
-cert_parse_inner(const char *fn, const unsigned char *der, size_t len, int ta)
+cert_parse_inner(const char *fn, const unsigned char *der, size_t len)
{
int rc = 0, extsz, c;
int sia_present = 0;
@@ -1132,12 +1129,14 @@ cert_parse_inner(const char *fn, const u
goto out;
}
- p.res->aki = x509_get_aki(x, ta, p.fn);
- p.res->ski = x509_get_ski(x, p.fn);
- if (!ta) {
- p.res->aia = x509_get_aia(x, p.fn);
- p.res->crl = x509_get_crl(x, p.fn);
- }
+ if (!x509_get_aki(x, p.fn, &p.res->aki))
+ goto out;
+ if (!x509_get_ski(x, p.fn, &p.res->ski))
+ goto out;
+ if (!x509_get_aia(x, p.fn, &p.res->aia))
+ goto out;
+ if (!x509_get_crl(x, p.fn, &p.res->crl))
+ goto out;
if (!x509_get_expire(x, p.fn, &p.res->expires))
goto out;
p.res->purpose = x509_get_purpose(x, p.fn);
@@ -1198,7 +1197,7 @@ cert_parse(const char *fn, const unsigne
{
struct cert *p;
- if ((p = cert_parse_inner(fn, der, len, 0)) == NULL)
+ if ((p = cert_parse_inner(fn, der, len)) == NULL)
return NULL;
if (p->aki == NULL) {
@@ -1212,8 +1211,12 @@ cert_parse(const char *fn, const unsigne
goto badcert;
}
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);
+ goto badcert;
+ }
+ if (p->crl == NULL) {
+ warnx("%s: RFC 6487 section 4.8.6: CRL: "
+ "no CRL distribution point extension", fn);
goto badcert;
}
return p;
@@ -1231,7 +1234,7 @@ ta_parse(const char *fn, const unsigned
EVP_PKEY *pk = NULL, *opk = NULL;
struct cert *p;
- if ((p = cert_parse_inner(fn, der, len, 1)) == NULL)
+ if ((p = cert_parse_inner(fn, der, len)) == NULL)
return NULL;
/* first check pubkey against the one from the TAL */
Index: extern.h
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.122
diff -u -p -r1.122 extern.h
--- extern.h 31 Mar 2022 12:00:00 -0000 1.122
+++ extern.h 1 Apr 2022 13:46:32 -0000
@@ -578,11 +578,11 @@ struct ibuf *io_buf_recvfd(int, struct i
/* X509 helpers. */
void x509_init_oid(void);
-char *x509_get_aia(X509 *, const char *);
-char *x509_get_aki(X509 *, int, const char *);
-char *x509_get_ski(X509 *, const char *);
+int x509_get_aia(X509 *, const char *, char **);
+int x509_get_aki(X509 *, const char *, char **);
+int x509_get_ski(X509 *, const char *, char **);
int x509_get_expire(X509 *, const char *, time_t *);
-char *x509_get_crl(X509 *, const char *);
+int x509_get_crl(X509 *, const char *, char **);
char *x509_crl_get_aki(X509_CRL *, const char *);
char *x509_get_pubkey(X509 *, const char *);
enum cert_purpose x509_get_purpose(X509 *, const char *);
Index: gbr.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/gbr.c,v
retrieving revision 1.14
diff -u -p -r1.14 gbr.c
--- gbr.c 18 Jan 2022 16:24:55 -0000 1.14
+++ gbr.c 1 Apr 2022 14:53:47 -0000
@@ -63,19 +63,24 @@ gbr_parse(X509 **x509, const char *fn, c
err(1, NULL);
free(cms);
- p.res->aia = x509_get_aia(*x509, fn);
- p.res->aki = x509_get_aki(*x509, 0, fn);
- p.res->ski = x509_get_ski(*x509, fn);
+ if (!x509_get_aia(*x509, fn, &p.res->aia))
+ goto out;
+ if (!x509_get_aki(*x509, fn, &p.res->aki))
+ goto out;
+ if (!x509_get_ski(*x509, fn, &p.res->ski))
+ goto out;
if (p.res->aia == NULL || p.res->aki == NULL || p.res->ski == NULL) {
warnx("%s: RFC 6487 section 4.8: "
"missing AIA, AKI or SKI X509 extension", fn);
- gbr_free(p.res);
- X509_free(*x509);
- *x509 = NULL;
- return NULL;
+ goto out;
}
-
return p.res;
+
+out:
+ gbr_free(p.res);
+ X509_free(*x509);
+ *x509 = NULL;
+ return NULL;
}
/*
Index: mft.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
retrieving revision 1.54
diff -u -p -r1.54 mft.c
--- mft.c 31 Mar 2022 12:00:00 -0000 1.54
+++ mft.c 1 Apr 2022 13:52:47 -0000
@@ -444,9 +444,12 @@ mft_parse(X509 **x509, const char *fn, c
if ((p.res = calloc(1, sizeof(struct mft))) == NULL)
err(1, NULL);
- p.res->aia = x509_get_aia(*x509, fn);
- p.res->aki = x509_get_aki(*x509, 0, fn);
- p.res->ski = x509_get_ski(*x509, fn);
+ if (!x509_get_aia(*x509, fn, &p.res->aia))
+ goto out;
+ if (!x509_get_aki(*x509, fn, &p.res->aki))
+ goto out;
+ if (!x509_get_ski(*x509, fn, &p.res->ski))
+ goto out;
if (p.res->aia == NULL || p.res->aki == NULL || p.res->ski == NULL) {
warnx("%s: RFC 6487 section 4.8: "
"missing AIA, AKI or SKI X509 extension", fn);
Index: parser.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.64
diff -u -p -r1.64 parser.c
--- parser.c 10 Feb 2022 15:33:47 -0000 1.64
+++ parser.c 1 Apr 2022 16:53:39 -0000
@@ -360,7 +360,14 @@ 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) {
+ if (c == NULL)
+ warnx("%s: RFC 6487 section 4.8.6: CRL: "
+ "no CRL distribution point extension", file);
+ mft_free(mft);
+ X509_free(x509);
+ return NULL;
+ }
crlfile = strrchr(c, '/');
if (crlfile != NULL)
crlfile++;
@@ -1078,7 +1085,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);
parse_load_crl(crl_uri);
free(crl_uri);
if (auth_find(&auths, aki) == NULL)
Index: roa.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
retrieving revision 1.38
diff -u -p -r1.38 roa.c
--- roa.c 10 Feb 2022 15:33:47 -0000 1.38
+++ roa.c 1 Apr 2022 14:00:48 -0000
@@ -351,9 +351,12 @@ roa_parse(X509 **x509, const char *fn, c
if ((p.res = calloc(1, sizeof(struct roa))) == NULL)
err(1, NULL);
- p.res->aia = x509_get_aia(*x509, fn);
- p.res->aki = x509_get_aki(*x509, 0, fn);
- p.res->ski = x509_get_ski(*x509, fn);
+ if (!x509_get_aia(*x509, fn, &p.res->aia))
+ goto out;
+ if (!x509_get_aki(*x509, fn, &p.res->aki))
+ goto out;
+ if (!x509_get_ski(*x509, fn, &p.res->ski))
+ goto out;
if (p.res->aia == NULL || p.res->aki == NULL || p.res->ski == NULL) {
warnx("%s: RFC 6487 section 4.8: "
"missing AIA, AKI or SKI X509 extension", fn);
Index: x509.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
retrieving revision 1.37
diff -u -p -r1.37 x509.c
--- x509.c 25 Mar 2022 08:19:04 -0000 1.37
+++ x509.c 1 Apr 2022 16:40:44 -0000
@@ -83,22 +83,18 @@ x509_init_oid(void)
* Returns the AKI or NULL if it could not be parsed.
* The AKI is formatted as a hex string.
*/
-char *
-x509_get_aki(X509 *x, int ta, const char *fn)
+int
+x509_get_aki(X509 *x, const char *fn, char **aki)
{
const unsigned char *d;
AUTHORITY_KEYID *akid;
ASN1_OCTET_STRING *os;
- int dsz, crit;
- char *res = NULL;
+ int dsz, crit, rc = 0;
+ *aki = NULL;
akid = X509_get_ext_d2i(x, NID_authority_key_identifier, &crit, NULL);
- if (akid == NULL) {
- if (!ta)
- warnx("%s: RFC 6487 section 4.8.3: AKI: "
- "extension missing", fn);
- return NULL;
- }
+ if (akid == NULL)
+ return 1;
if (crit != 0) {
warnx("%s: RFC 6487 section 4.8.3: "
"AKI: extension not non-critical", fn);
@@ -128,11 +124,11 @@ x509_get_aki(X509 *x, int ta, const char
goto out;
}
- res = hex_encode(d, dsz);
-
+ *aki = hex_encode(d, dsz);
+ rc = 1;
out:
AUTHORITY_KEYID_free(akid);
- return res;
+ return rc;
}
/*
@@ -140,19 +136,17 @@ out:
* Returns the SKI or NULL if it could not be parsed.
* The SKI is formatted as a hex string.
*/
-char *
-x509_get_ski(X509 *x, const char *fn)
+int
+x509_get_ski(X509 *x, const char *fn, char **ski)
{
const unsigned char *d;
ASN1_OCTET_STRING *os;
- int dsz, crit;
- char *res = NULL;
+ int dsz, crit, rc = 0;
+ *ski = NULL;
os = X509_get_ext_d2i(x, NID_subject_key_identifier, &crit, NULL);
- if (os == NULL) {
- warnx("%s: RFC 6487 section 4.8.2: SKI: extension missing", fn);
- return NULL;
- }
+ if (os == NULL)
+ return 1;
if (crit != 0) {
warnx("%s: RFC 6487 section 4.8.2: "
"SKI: extension not non-critical", fn);
@@ -169,10 +163,11 @@ x509_get_ski(X509 *x, const char *fn)
goto out;
}
- res = hex_encode(d, dsz);
+ *ski = hex_encode(d, dsz);
+ rc = 1;
out:
ASN1_OCTET_STRING_free(os);
- return res;
+ return rc;
}
/*
@@ -281,19 +276,18 @@ x509_get_pubkey(X509 *x, const char *fn)
* Returns NULL on failure, on success returns the AIA URI
* (which has to be freed after use).
*/
-char *
-x509_get_aia(X509 *x, const char *fn)
+int
+x509_get_aia(X509 *x, const char *fn, char **aia)
{
ACCESS_DESCRIPTION *ad;
AUTHORITY_INFO_ACCESS *info;
- char *aia = NULL;
- int crit;
+ int crit, rc = 0;
+ *aia = NULL;
info = X509_get_ext_d2i(x, NID_info_access, &crit, NULL);
- if (info == NULL) {
- warnx("%s: RFC 6487 section 4.8.7: AIA: extension missing", fn);
- return NULL;
- }
+ if (info == NULL)
+ return 1;
+
if (crit != 0) {
warnx("%s: RFC 6487 section 4.8.7: "
"AIA: extension not non-critical", fn);
@@ -325,15 +319,16 @@ x509_get_aia(X509 *x, const char *fn)
goto out;
}
- aia = strndup(
+ *aia = strndup(
ASN1_STRING_get0_data(ad->location->d.uniformResourceIdentifier),
ASN1_STRING_length(ad->location->d.uniformResourceIdentifier));
- if (aia == NULL)
+ if (*aia == NULL)
err(1, NULL);
+ rc = 1;
out:
AUTHORITY_INFO_ACCESS_free(info);
- return aia;
+ return rc;
}
/*
@@ -364,21 +359,19 @@ x509_get_expire(X509 *x, const char *fn,
* Returns NULL on failure, the crl URI on success which has to be freed
* after use.
*/
-char *
-x509_get_crl(X509 *x, const char *fn)
+int
+x509_get_crl(X509 *x, const char *fn, char **crl)
{
CRL_DIST_POINTS *crldp;
DIST_POINT *dp;
GENERAL_NAME *name;
- char *crl = NULL;
- int crit;
+ int crit, rc = 0;
+ *crl = NULL;
crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, &crit, NULL);
- if (crldp == NULL) {
- warnx("%s: RFC 6487 section 4.8.6: CRL: "
- "no CRL distribution point extension", fn);
- return NULL;
- }
+ if (crldp == NULL)
+ return 1;
+
if (crit != 0) {
warnx("%s: RFC 6487 section 4.8.6: "
"CRL distribution point: extension not non-critical", fn);
@@ -425,14 +418,15 @@ x509_get_crl(X509 *x, const char *fn)
goto out;
}
- crl = strndup(ASN1_STRING_get0_data(name->d.uniformResourceIdentifier),
+ *crl = strndup(ASN1_STRING_get0_data(name->d.uniformResourceIdentifier),
ASN1_STRING_length(name->d.uniformResourceIdentifier));
- if (crl == NULL)
+ if (*crl == NULL)
err(1, NULL);
+ rc = 1;
out:
CRL_DIST_POINTS_free(crldp);
- return crl;
+ return rc;
}
/*
