On Tue, Apr 12, 2022 at 09:58:21AM +0200, Theo Buehler wrote:
> We can generalize sbgp_sia_location() and reuse it for AIAs and CRLs.
> This makes the checks a bit more stringent, which seems to be fine in
> practice. It also ensures that there are no embedded NULs which came
> up recently. One thing I'm not sure about is that valid_uri() refuses
> URIs with "/." which is an additional check.
>
> I'll also fix the regress Makefile.inc if this goes in.
We do not allow /. in URI to avoid both hidden dirs and path traversals
from happening. Since AIA and CRL are both RPKI URI they need to respect
the same rules as caRepository and manifest URI. So doing this check there
as well is fine.
OK claudio@
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 cert.c
> --- cert.c 11 Apr 2022 10:39:45 -0000 1.66
> +++ cert.c 12 Apr 2022 07:28:38 -0000
> @@ -125,40 +125,6 @@ sbgp_addr(struct parse *p,
> }
>
> /*
> - * Extract and validate a SIA accessLocation, RFC 6487, 4.8.8 and RFC 8192,
> 3.2.
> - * Returns 0 on failure and 1 on success.
> - */
> -static int
> -sbgp_sia_location(const char *fn, const char *descr, const char *proto,
> - GENERAL_NAME *location, char **out)
> -{
> - ASN1_IA5STRING *uri;
> -
> - if (*out != NULL) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: %s already specified",
> - fn, descr);
> - return 0;
> - }
> -
> - if (location->type != GEN_URI) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: %s not URI", fn, descr);
> - return 0;
> - }
> -
> - uri = location->d.uniformResourceIdentifier;
> -
> - if (!valid_uri(uri->data, uri->length, proto)) {
> - warnx("%s: RFC 6487 section 4.8.8: bad %s location", fn, descr);
> - return 0;
> - }
> -
> - if ((*out = strndup(uri->data, uri->length)) == NULL)
> - err(1, NULL);
> -
> - return 1;
> -}
> -
> -/*
> * Parse "Subject Information Access" extension, RFC 6487 4.8.8.
> * Returns zero on failure, non-zero on success.
> */
> @@ -188,15 +154,15 @@ sbgp_sia(struct parse *p, X509_EXTENSION
> oid = ad->method;
>
> if (OBJ_cmp(oid, carepo_oid) == 0) {
> - if (!sbgp_sia_location(p->fn, "caRepository",
> + if (!x509_location(p->fn, "SIA: caRepository",
> "rsync://", ad->location, &p->res->repo))
> goto out;
> } else if (OBJ_cmp(oid, manifest_oid) == 0) {
> - if (!sbgp_sia_location(p->fn, "rpkiManifest",
> + if (!x509_location(p->fn, "SIA: rpkiManifest",
> "rsync://", ad->location, &p->res->mft))
> goto out;
> } else if (OBJ_cmp(oid, notify_oid) == 0) {
> - if (!sbgp_sia_location(p->fn, "rpkiNotify",
> + if (!x509_location(p->fn, "SIA: rpkiNotify",
> "https://";, ad->location, &p->res->notify))
> goto out;
> }
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.125
> diff -u -p -r1.125 extern.h
> --- extern.h 4 Apr 2022 16:02:54 -0000 1.125
> +++ extern.h 12 Apr 2022 07:33:20 -0000
> @@ -22,6 +22,7 @@
> #include <sys/time.h>
>
> #include <openssl/x509.h>
> +#include <openssl/x509v3.h>
>
> enum cert_as_type {
> CERT_AS_ID, /* single identifier */
> @@ -589,6 +590,8 @@ char *x509_get_pubkey(X509 *, const cha
> enum cert_purpose x509_get_purpose(X509 *, const char *);
> int x509_get_time(const ASN1_TIME *, time_t *);
> char *x509_convert_seqnum(const char *, const ASN1_INTEGER *);
> +int x509_location(const char *, const char *, const char *,
> + GENERAL_NAME *, char **);
>
> /* printers */
> char *time2str(time_t);
> Index: x509.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 x509.c
> --- x509.c 8 Apr 2022 15:29:59 -0000 1.39
> +++ x509.c 12 Apr 2022 07:33:02 -0000
> @@ -306,24 +306,10 @@ x509_get_aia(X509 *x, const char *fn, ch
> "expected caIssuers, have %d", fn, OBJ_obj2nid(ad->method));
> goto out;
> }
> - if (ad->location->type != GEN_URI) {
> - warnx("%s: RFC 6487 section 4.8.7: AIA: "
> - "want GEN_URI type, have %d", fn, ad->location->type);
> - goto out;
> - }
>
> - if (ASN1_STRING_length(ad->location->d.uniformResourceIdentifier)
> - > MAX_URI_LENGTH) {
> - warnx("%s: RFC 6487 section 4.8.7: AIA: "
> - "URI exceeds max length of %d", fn, MAX_URI_LENGTH);
> + if (!x509_location(fn, "AIA: caIssuers", NULL, ad->location, aia))
> goto out;
> - }
>
> - *aia = strndup(
> - ASN1_STRING_get0_data(ad->location->d.uniformResourceIdentifier),
> - ASN1_STRING_length(ad->location->d.uniformResourceIdentifier));
> - if (*aia == NULL)
> - err(1, NULL);
> rc = 1;
>
> out:
> @@ -405,23 +391,9 @@ x509_get_crl(X509 *x, const char *fn, ch
> }
>
> name = sk_GENERAL_NAME_value(dp->distpoint->name.fullname, 0);
> - if (name->type != GEN_URI) {
> - warnx("%s: RFC 6487 section 4.8.6: CRL: "
> - "want URI type, have %d", fn, name->type);
> - goto out;
> - }
>
> - if (ASN1_STRING_length(name->d.uniformResourceIdentifier)
> - > MAX_URI_LENGTH) {
> - warnx("%s: RFC 6487 section 4.8.6: CRL: "
> - "URI exceeds max length of %d", fn, MAX_URI_LENGTH);
> + if (!x509_location(fn, "CRL distribution point", NULL, name, crl))
> goto out;
> - }
> -
> - *crl = strndup(ASN1_STRING_get0_data(name->d.uniformResourceIdentifier),
> - ASN1_STRING_length(name->d.uniformResourceIdentifier));
> - if (*crl == NULL)
> - err(1, NULL);
> rc = 1;
>
> out:
> @@ -500,6 +472,40 @@ x509_get_time(const ASN1_TIME *at, time_
> return 0;
> if ((*t = timegm(&tm)) == -1)
> errx(1, "timegm failed");
> + return 1;
> +}
> +
> +/*
> + * Extract and validate an accessLocation, RFC 6487, 4.8 and RFC 8192, 3.2.
> + * Returns 0 on failure and 1 on success.
> + */
> +int
> +x509_location(const char *fn, const char *descr, const char *proto,
> + GENERAL_NAME *location, char **out)
> +{
> + ASN1_IA5STRING *uri;
> +
> + if (*out != NULL) {
> + warnx("%s: RFC 6487 section 4.8: %s already specified", fn,
> + descr);
> + return 0;
> + }
> +
> + if (location->type != GEN_URI) {
> + warnx("%s: RFC 6487 section 4.8: %s not URI", fn, descr);
> + return 0;
> + }
> +
> + uri = location->d.uniformResourceIdentifier;
> +
> + if (!valid_uri(uri->data, uri->length, proto)) {
> + warnx("%s: RFC 6487 section 4.8: %s bad location", fn, descr);
> + return 0;
> + }
> +
> + if ((*out = strndup(uri->data, uri->length)) == NULL)
> + err(1, NULL);
> +
> return 1;
> }
>
>
--
:wq Claudio
