On Fri, Mar 05, 2021 at 01:48:43PM +0100, Claudio Jeker wrote:
> Instead of adding similar checks all over the place introduce a
> valid_uri() function that checks if a URI is valid enough for rpki-client.
> rpki-client does not accept files or directories starting with ., bails on
> URI that have strange characters and valid_uri() will also check that the
> protocol is the expected one if provided.
>
> This diff converts the 4 places where URI are handled.
I'm ok with this. A few comments inline.
> --
> :wq Claudio
>
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 cert.c
> --- cert.c 18 Feb 2021 16:23:17 -0000 1.27
> +++ cert.c 5 Mar 2021 12:33:50 -0000
> @@ -19,7 +19,6 @@
>
> #include <arpa/inet.h>
> #include <assert.h>
> -#include <ctype.h>
> #include <err.h>
> #include <inttypes.h>
> #include <stdarg.h>
> @@ -140,34 +139,22 @@ sbgp_addr(struct parse *p,
> * Returns zero on failure, non-zero on success.
> */
> static int
> -sbgp_sia_resource_notify(struct parse *p,
> - const unsigned char *d, size_t dsz)
> +sbgp_sia_resource_notify(struct parse *p, const char *d, size_t dsz)
> {
> - size_t i;
> -
> if (p->res->notify != NULL) {
> warnx("%s: RFC 6487 section 4.8.8: SIA: "
> "Notify location already specified", p->fn);
> return 0;
> }
>
> + if ((p->res->notify = strndup(d, dsz)) == NULL)
This was slightly more robust before as p->res->notify would only get
assigned after all checks passed. That way it's guaranteed that an
invalid string is never used erroneously.
Perhaps it's not worth the extra gymnastics of strnduping to a local
variable and freeing it on error. After all, it will currently get
freed in cert_parse_inner() on validation failure...
> + err(1, NULL);
> +
> /* Make sure it's a https:// address. */
> - if (dsz <= 8 || strncasecmp(d, "https://";, 8)) {
> - warnx("%s: RFC 8182 section 3.2: not using https schema",
> - p->fn);
> + if (!valid_uri(p->res->notify, "https://";)) {
> + warnx("%s: RFC 8182 section 3.2: bad Notify URI", p->fn);
> return 0;
> }
> - /* make sure only US-ASCII chars are in the URL */
> - for (i = 0; i < dsz; i++) {
> - if (isalnum(d[i]) || ispunct(d[i]))
> - continue;
> - warnx("%s: invalid URI", p->fn);
> - return 0;
> - }
> -
> -
> - if ((p->res->notify = strndup((const char *)d, dsz)) == NULL)
> - err(1, NULL);
>
> return 1;
> }
> @@ -177,40 +164,29 @@ sbgp_sia_resource_notify(struct parse *p
> * Returns zero on failure, non-zero on success.
> */
> static int
> -sbgp_sia_resource_mft(struct parse *p,
> - const unsigned char *d, size_t dsz)
> +sbgp_sia_resource_mft(struct parse *p, const char *d, size_t dsz)
> {
> - size_t i;
> -
> if (p->res->mft != NULL) {
> warnx("%s: RFC 6487 section 4.8.8: SIA: "
> "MFT location already specified", p->fn);
> return 0;
> }
>
> - /* Make sure it's an MFT rsync address. */
> - if (dsz <= 8 || strncasecmp(d, "rsync://", 8)) {
> - warnx("%s: RFC 6487 section 4.8.8: not using rsync schema",
> - p->fn);
> - return 0;
> - }
> -
> if (strcasecmp(d + dsz - 4, ".mft") != 0) {
Since you dropped the above check that ensures that dsz > 8, I think this
needs to be
if (dsz < 4 || strcasecmp(...))
to avoid a potential out-of-bounds access
> warnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "invalid rsync URI suffix", p->fn);
> - return 0;
> - }
> - /* make sure only US-ASCII chars are in the URL */
> - for (i = 0; i < dsz; i++) {
> - if (isalnum(d[i]) || ispunct(d[i]))
> - continue;
> - warnx("%s: invalid URI", p->fn);
> + "not an MFT file", p->fn);
> return 0;
> }
>
> - if ((p->res->mft = strndup((const char *)d, dsz)) == NULL)
> + if ((p->res->mft = strndup(d, dsz)) == NULL)
> err(1, NULL);
>
> + /* Make sure it's an MFT rsync address. */
> + if (!valid_uri(p->res->mft, "rsync://")) {
> + warnx("%s: RFC 6487 section 4.8.8: bad MFT location", p->fn);
> + return 0;
> + }
> +
> return 1;
> }
>
> @@ -219,35 +195,24 @@ sbgp_sia_resource_mft(struct parse *p,
> * Returns zero on failure, non-zero on success.
> */
> static int
> -sbgp_sia_resource_carepo(struct parse *p,
> - const unsigned char *d, size_t dsz)
> +sbgp_sia_resource_carepo(struct parse *p, const char *d, size_t dsz)
> {
> - size_t i;
> -
> if (p->res->repo != NULL) {
> warnx("%s: RFC 6487 section 4.8.8: SIA: "
> "CA repository already specified", p->fn);
> return 0;
> }
>
> + if ((p->res->repo = strndup(d, dsz)) == NULL)
> + err(1, NULL);
> +
> /* Make sure it's an rsync:// address. */
> - if (dsz <= 8 || strncasecmp(d, "rsync://", 8)) {
> - warnx("%s: RFC 6487 section 4.8.8: not using rsync schema",
> + if (!valid_uri(p->res->repo, "rsync://")) {
> + warnx("%s: RFC 6487 section 4.8.8: bad CA repository URI",
> p->fn);
> return 0;
> }
>
> - /* make sure only US-ASCII chars are in the URL */
> - for (i = 0; i < dsz; i++) {
> - if (isalnum(d[i]) || ispunct(d[i]))
> - continue;
> - warnx("%s: invalid URI", p->fn);
> - return 0;
> - }
> -
> - if ((p->res->repo = strndup((const char *)d, dsz)) == NULL)
> - err(1, NULL);
> -
> return 1;
> }
>
> @@ -1175,7 +1140,6 @@ out:
> struct cert *
> cert_parse(X509 **xp, const char *fn)
> {
> -
> return cert_parse_inner(xp, fn, 0);
> }
>
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.50
> diff -u -p -r1.50 extern.h
> --- extern.h 4 Mar 2021 13:01:41 -0000 1.50
> +++ extern.h 5 Mar 2021 12:36:51 -0000
> @@ -354,6 +354,7 @@ int valid_ta(const char *, struct auth
> int valid_cert(const char *, struct auth_tree *,
> const struct cert *);
> int valid_roa(const char *, struct auth_tree *, struct roa *);
> +int valid_uri(const char *, const char *);
>
> /* Working with CMS files. */
>
> Index: tal.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/tal.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 tal.c
> --- tal.c 19 Feb 2021 10:23:50 -0000 1.27
> +++ tal.c 5 Mar 2021 12:34:54 -0000
> @@ -82,7 +82,6 @@ tal_parse_buffer(const char *fn, char *b
> char *nl, *line, *f, *file = NULL;
> unsigned char *der;
> size_t sz, dersz;
> - ssize_t i;
> int rc = 0;
> struct tal *tal = NULL;
> EVP_PKEY *pkey = NULL;
> @@ -103,9 +102,7 @@ tal_parse_buffer(const char *fn, char *b
> break;
>
> /* make sure only US-ASCII chars are in the URL */
> - for (i = 0; i < nl - line; i++) {
> - if (isalnum(line[i]) || ispunct(line[i]))
> - continue;
> + if (!valid_uri(line, NULL)) {
> warnx("%s: invalid URI", fn);
> goto out;
> }
> Index: validate.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 validate.c
> --- validate.c 12 Sep 2020 15:46:48 -0000 1.11
> +++ validate.c 5 Mar 2021 12:36:40 -0000
> @@ -19,7 +19,9 @@
>
> #include <arpa/inet.h>
> #include <assert.h>
> +#include <ctype.h>
> #include <err.h>
> +#include <fcntl.h>
> #include <inttypes.h>
> #include <stdarg.h>
> #include <stdlib.h>
> @@ -235,6 +237,40 @@ valid_roa(const char *fn, struct auth_tr
> warnx("%s: RFC 6482: uncovered IP: "
> "%s", fn, buf);
> tracewarn(a);
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +/*
> + * Validate a URI to make sure it is pure ASCII and does not point backwards
> + * or doing some other silly tricks. To enforce the protocol pass either
> + * https:// or rsync:// as proto, if NULL is passed no protocol is enforced.
> + * Returns 1 if valid, 0 otherwise.
> + */
> +int
> +valid_uri(const char *uri, const char *proto)
> +{
> + const unsigned char *u;
> +
> + for (u = uri; *u != '\0'; u++)
> + if (!isalnum(*u) && !ispunct(*u)) {
> + warnx("URI: non-ascii %s", uri);
> + return 0;
> + }
> +
> + if (proto) {
I would do proto != NULL :)
> + size_t plen = strlen(proto);
Perhaps add an empty line here. style(9) doesn't like function calls for
variable initialization.
> + if (strncasecmp(uri, proto, plen) != 0) {
> + warnx("URI: not proto %s: %s", proto, uri);
> + return 0;
> + }
> + }
> +
> + /* do not allow files or directories to start with a . */
> + if (strstr(uri, "/.") != NULL) {
> + warnx("URI: dot-file %s", uri);
> return 0;
> }
>
>
