Download raw body.
rpki-client: improve SIA handling
On Mon, Jun 03, 2024 at 11:09:57AM +0200, Claudio Jeker wrote:
> On Sun, Jun 02, 2024 at 04:56:51PM +0200, Theo Buehler wrote:
> > The way I read 4.8.8.1 of RFC 6487, the current handling of the SIA
> > extension is not entirely correct for CA certs. We reject non-rsync URI
> > for the caRepository and rpkiManifest access methods while the spec only
> > requires that an rsync URI be present for both of them, not that all
> > URIs use the rsync scheme. So accept all valid URIs and retain the first
> > rsync one we encounter. If there's no rsync URI we will reject the cert.
> > In contrast, RFC 8182 3.2 only allows https URI for rpkiNotify.
> >
> > Also make it clearer that sbgp_sia() is for CA certs and x509_get_sia()
> > for EE certs.
> >
> > Index: cert.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> > diff -u -p -r1.132 cert.c
> > --- cert.c 31 May 2024 02:45:15 -0000 1.132
> > +++ cert.c 2 Jun 2024 14:34:56 -0000
> > @@ -495,7 +495,8 @@ sbgp_ipaddrblk(const char *fn, struct ce
> > }
> >
> > /*
> > - * Parse "Subject Information Access" extension, RFC 6487 4.8.8.
> > + * Parse "Subject Information Access" extension for a CA cert,
> > + * RFC 6487, section 4.8.8.1 and RFC 8182, section 3.2.
> > * Returns zero on failure, non-zero on success.
> > */
> > static int
> > @@ -525,13 +526,23 @@ sbgp_sia(const char *fn, struct cert *ce
> > oid = ad->method;
> >
> > if (OBJ_cmp(oid, carepo_oid) == 0) {
> > - if (!x509_location(fn, "SIA: caRepository",
> > - RSYNC_PROTO, ad->location, &cert->repo))
> > + if (!x509_location(fn, "SIA: caRepository", NULL,
> > + ad->location, &cert->repo))
> > goto out;
> > + if (strncasecmp(cert->repo, RSYNC_PROTO,
> > + RSYNC_PROTO_LEN) == 0)
> > + continue;
> > + free(cert->repo);
> > + cert->repo = NULL;
>
> This seems very fragile on how multiple locations are handled.
Not sure how it is fragile except that x509_location() isn't the
greatest of APIs I ever came up with, but maybe the below is more
robust.
> Especially the warnx() in x509_location() depends on the order of uri.
This warning is mostly useless noise here in my opinion. But yes, we
could do better... The below will warn about every ignored URI, but only
in verbose mode as I don't think this is of any interest to regular
users.
I think x509_location() should lose its proto argument since it's almost
always NULL now. I'll clean that up as a follow-up.
Index: cert.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
diff -u -p -r1.132 cert.c
--- cert.c 31 May 2024 02:45:15 -0000 1.132
+++ cert.c 3 Jun 2024 10:15:46 -0000
@@ -495,7 +495,8 @@ sbgp_ipaddrblk(const char *fn, struct ce
}
/*
- * Parse "Subject Information Access" extension, RFC 6487 4.8.8.
+ * Parse "Subject Information Access" extension for a CA cert,
+ * RFC 6487, section 4.8.8.1 and RFC 8182, section 3.2.
* Returns zero on failure, non-zero on success.
*/
static int
@@ -505,8 +506,11 @@ sbgp_sia(const char *fn, struct cert *ce
ACCESS_DESCRIPTION *ad;
ASN1_OBJECT *oid;
const char *mftfilename;
+ char *carepo = NULL, *rpkimft = NULL;
int i, rc = 0;
+ assert(cert->repo == NULL && cert->mft == NULL && cert->notify == NULL);
+
if (X509_EXTENSION_get_critical(ext)) {
warnx("%s: RFC 6487 section 4.8.8: SIA: "
"extension not non-critical", fn);
@@ -525,13 +529,35 @@ sbgp_sia(const char *fn, struct cert *ce
oid = ad->method;
if (OBJ_cmp(oid, carepo_oid) == 0) {
- if (!x509_location(fn, "SIA: caRepository",
- RSYNC_PROTO, ad->location, &cert->repo))
+ if (!x509_location(fn, "SIA: caRepository", NULL,
+ ad->location, &carepo))
goto out;
+ if (cert->repo == NULL && strncasecmp(carepo,
+ RSYNC_PROTO, RSYNC_PROTO_LEN) == 0) {
+ cert->repo = carepo;
+ carepo = NULL;
+ continue;
+ }
+ if (verbose)
+ warnx("%s: RFC 6487 section 4.8.8: SIA: "
+ "ignoring location %s", fn, carepo);
+ free(carepo);
+ carepo = NULL;
} else if (OBJ_cmp(oid, manifest_oid) == 0) {
- if (!x509_location(fn, "SIA: rpkiManifest",
- RSYNC_PROTO, ad->location, &cert->mft))
+ if (!x509_location(fn, "SIA: rpkiManifest", NULL,
+ ad->location, &rpkimft))
goto out;
+ if (cert->mft == NULL && strncasecmp(rpkimft,
+ RSYNC_PROTO, RSYNC_PROTO_LEN) == 0) {
+ cert->mft = rpkimft;
+ rpkimft = NULL;
+ continue;
+ }
+ if (verbose)
+ warnx("%s: RFC 6487 section 4.8.8: SIA: "
+ "ignoring location %s", fn, rpkimft);
+ free(rpkimft);
+ rpkimft = NULL;
} else if (OBJ_cmp(oid, notify_oid) == 0) {
if (!x509_location(fn, "SIA: rpkiNotify",
HTTPS_PROTO, ad->location, &cert->notify))
@@ -844,6 +870,10 @@ cert_parse_pre(const char *fn, const uns
case NID_sinfo_access:
if (sia++ > 0)
goto dup;
+ /*
+ * This will fail for BGPsec certs, but they must omit
+ * this extension anyway (RFC 8209, section 3.1.3.3).
+ */
if (!sbgp_sia(fn, cert, ext))
goto out;
break;
Index: x509.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
diff -u -p -r1.90 x509.c
--- x509.c 31 May 2024 11:27:34 -0000 1.90
+++ x509.c 3 Jun 2024 09:59:17 -0000
@@ -467,8 +467,8 @@ out:
}
/*
- * Parse the Subject Information Access (SIA) extension
- * See RFC 6487, section 4.8.8 for details.
+ * Parse the Subject Information Access (SIA) extension for an EE cert.
+ * See RFC 6487, section 4.8.8.2 for details.
* Returns NULL on failure, on success returns the SIA signedObject URI
* (which has to be freed after use).
*/
rpki-client: improve SIA handling