On Tue, Jul 04, 2023 at 11:39:19AM -0400, Dave Voutila wrote:
> vmd's doing something close to shotgun parsing of the "local prefix" and
> "local inet6 prefix" settings in vm.conf(5). The parser intermixes ipv4
> and ipv6 parsing even when we know which one is valid in the parsing
> context. This makes me sad.
>
> Even worse, we're not validating the inputs at time of parsing and
> deferring to vm creation time. This makes me even sadder.
>
> The diff below:
> - splits parsing ipv4 and ipv6 into distinct functions
> - puts the validation into those functions (e.g prefix length, prefix
> has properly zero'd octets)
> - does *not* muck (yet) with the existing validation sprinkled in
> priv.c or config.c
>
> I thought about making pulling apart the prefix from prefix length
> parsing, but getaddrinfo(3) appears to not like parsing addresses like
> "192.168.0.0" vs "192.168.0.0/16". (I'm not a network person...maybe I'm
> being dumb here.)
>
> kn: this addresses some of your feedback on my previous diff from a few
> weeks ago.
>
> ok?
Most of these issues have been solved in for example bgpd.
The code there is able to parse most address forms also shortcuts like
192.168/24. Have a look at bgpd/config.c::host() and host_ip() on how it
works.
> diff refs/heads/master refs/heads/vmd-parse
> commit - 5d90c77abd2d7447f16f88ac9ea9e0485eac9f73
> commit + 228fe48802ec6250e3487aa791daceba4626b03f
> blob - b538d40be1a1e600c1021d95e3fadd310079fa7a
> blob + f5a2507ff5742ea3a62b0112e14d17aa8cbdf99d
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -49,7 +49,7 @@ config_init_localprefix(struct vmd_config *cfg)
> {
> struct sockaddr_in6 *sin6;
>
> - if (host(VMD_DHCP_PREFIX, &cfg->cfg_localprefix) == -1)
> + if (parse_prefix4(VMD_DHCP_PREFIX, &cfg->cfg_localprefix, NULL) == -1)
> return (-1);
>
> /* IPv6 is disabled by default */
> @@ -58,7 +58,7 @@ config_init_localprefix(struct vmd_config *cfg)
> /* Generate random IPv6 prefix only once */
> if (cfg->cfg_flags & VMD_CFG_AUTOINET6)
> return (0);
> - if (host(VMD_ULA_PREFIX, &cfg->cfg_localprefix6) == -1)
> + if (parse_prefix6(VMD_ULA_PREFIX, &cfg->cfg_localprefix6, NULL) == -1)
> return (-1);
> /* Randomize the 56 bits "Global ID" and "Subnet ID" */
> sin6 = ss2sin6(&cfg->cfg_localprefix6.ss);
> blob - 09468e3fe2c9f4f9193710c65667132f79a90df3
> blob + 3d030c201db3e8167831846cb1c8f6e3facc40fc
> --- usr.sbin/vmd/parse.y
> +++ usr.sbin/vmd/parse.y
> @@ -190,31 +190,30 @@ main : LOCAL INET6 {
> }
> | LOCAL INET6 PREFIX STRING {
> struct address h;
> + const char *err;
>
> - if (host($4, &h) == -1 ||
> - h.ss.ss_family != AF_INET6 ||
> - h.prefixlen > 64 || h.prefixlen < 0) {
> - yyerror("invalid local inet6 prefix: %s", $4);
> - free($4);
> + if (parse_prefix6($4, &h, &err)) {
> + yyerror("invalid local inet6 prefix: %s", err);
> YYERROR;
> + } else {
> + env->vmd_cfg.cfg_flags |= VMD_CFG_INET6;
> + env->vmd_cfg.cfg_flags &= ~VMD_CFG_AUTOINET6;
> + memcpy(&env->vmd_cfg.cfg_localprefix6, &h,
> + sizeof(h));
> }
> -
> - env->vmd_cfg.cfg_flags |= VMD_CFG_INET6;
> - env->vmd_cfg.cfg_flags &= ~VMD_CFG_AUTOINET6;
> - memcpy(&env->vmd_cfg.cfg_localprefix6, &h, sizeof(h));
> + free($4);
> }
> | LOCAL PREFIX STRING {
> struct address h;
> + const char *err;
>
> - if (host($3, &h) == -1 ||
> - h.ss.ss_family != AF_INET ||
> - h.prefixlen > 32 || h.prefixlen < 0) {
> - yyerror("invalid local prefix: %s", $3);
> - free($3);
> + if (parse_prefix4($3, &h, &err)) {
> + yyerror("invalid local prefix: %s", err);
> YYERROR;
> - }
> -
> - memcpy(&env->vmd_cfg.cfg_localprefix, &h, sizeof(h));
> + } else
> + memcpy(&env->vmd_cfg.cfg_localprefix, &h,
> + sizeof(h));
> + free($3);
> }
> | SOCKET OWNER owner_id {
> env->vmd_ps.ps_csock.cs_uid = $3.uid;
> @@ -1404,42 +1403,119 @@ int
> return (0);
> }
>
> +/*
> + * Parse an ipv4 address and prefix for local interfaces and validate
> + * constraints for vmd networking.
> + */
> int
> -host(const char *str, struct address *h)
> +parse_prefix4(const char *str, struct address *h, const char **errstr)
> {
> - struct addrinfo hints, *res;
> - int prefixlen;
> - char *s, *p;
> - const char *errstr;
> + struct addrinfo hints, *res = NULL;
> + in_addr_t addr;
> + int prefixlen, ret;
> + char *s = NULL, *p = NULL;
>
> - if ((s = strdup(str)) == NULL) {
> - log_warn("%s", __func__);
> - goto fail;
> + if ((s = strdup(str)) == NULL)
> + fatal("%s: strdup", __func__);
> + p = strrchr(s, '/');
> + if (p == NULL) {
> + if (errstr)
> + *errstr = "missing netmask";
> + return (1);
> }
> + *p++ = '\0';
>
> - if ((p = strrchr(s, '/')) != NULL) {
> - *p++ = '\0';
> - prefixlen = strtonum(p, 0, 128, &errstr);
> - if (errstr) {
> - log_warnx("prefixlen is %s: %s", errstr, p);
> - goto fail;
> + /* Parse our prefix. For ipv4, we accept 16 as maximum. */
> + prefixlen = strtonum(p, 1, 16, errstr);
> + if (prefixlen == 0) {
> + free(s);
> + return (1);
> + }
> +
> + /* Attempt to construct an address from the user input. */
> + memset(&hints, 0, sizeof(hints));
> + hints.ai_family = AF_INET;
> + hints.ai_flags = AI_NUMERICHOST;
> + if ((ret = getaddrinfo(s, NULL, &hints, &res)) != 0) {
> + if (errstr != NULL)
> + *errstr = gai_strerror(ret);
> + free(s);
> + return (1);
> + }
> + free(s);
> +
> + memset(h, 0, sizeof(*h));
> + memcpy(&h->ss, res->ai_addr, res->ai_addrlen);
> + h->prefixlen = prefixlen;
> + freeaddrinfo(res);
> +
> + /* Our prefix address should end with zeros. */
> + addr = ss2sin(&h->ss)->sin_addr.s_addr;
> + if (addr & 0xFFFF0000) {
> + if (errstr != NULL)
> + *errstr = "last two octets must be zero";
> + return (1);
> + }
> +
> + return (0);
> +}
> +
> +/*
> + * Parse an ipv6 address and prefix for local interfaces and validate
> + * constraints for vmd networking.
> + */
> +int
> +parse_prefix6(const char *str, struct address *h, const char **errstr)
> +{
> + struct addrinfo hints, *res = NULL;
> + struct in6_addr *addr6 = NULL;
> + int prefixlen, ret;
> + char *s = NULL, *p = NULL;
> + size_t i;
> +
> + if ((s = strdup(str)) == NULL)
> + fatal("%s: strdup", __func__);
> + p = strrchr(s, '/');
> + if (p == NULL) {
> + if (errstr)
> + *errstr = "missing netmask";
> + return (1);
> + }
> + *p++ = '\0';
> +
> + /* Parse our prefix. For ipv6, we accept 64 as a maximum. */
> + prefixlen = strtonum(p, 1, 64, errstr);
> + if (prefixlen == 0) {
> + free(s);
> + return (1);
> + }
> +
> + /* Attempt to construct an address from the user input. */
> + memset(&hints, 0, sizeof(hints));
> + hints.ai_family = AF_INET6;
> + hints.ai_flags = AI_NUMERICHOST;
> + if ((ret = getaddrinfo(s, NULL, &hints, &res)) != 0) {
> + if (errstr != NULL)
> + *errstr = gai_strerror(ret);
> + free(s);
> + return (1);
> + }
> + free(s);
> +
> + memset(h, 0, sizeof(*h));
> + memcpy(&h->ss, res->ai_addr, res->ai_addrlen);
> + h->prefixlen = prefixlen;
> + freeaddrinfo(res);
> +
> + /* Last 8 octets are reserved for vmd. */
> + addr6 = &ss2sin6(&h->ss)->sin6_addr;
> + for (i = 8; i < 16; i++) {
> + if (addr6->s6_addr[i] != 0) {
> + if (errstr != NULL)
> + *errstr = "last eight octets must be zero";
> + return (1);
> }
> - } else
> - prefixlen = 128;
> -
> - memset(&hints, 0, sizeof(hints));
> - hints.ai_family = AF_UNSPEC;
> - hints.ai_flags = AI_NUMERICHOST;
> - if (getaddrinfo(s, NULL, &hints, &res) == 0) {
> - memset(h, 0, sizeof(*h));
> - memcpy(&h->ss, res->ai_addr, res->ai_addrlen);
> - h->prefixlen = prefixlen;
> - freeaddrinfo(res);
> - free(s);
> - return (0);
> }
>
> - fail:
> - free(s);
> - return (-1);
> + return (0);
> }
> blob - 9c25b0c92ade2e1a1d3a1f67548becfc3d4eca7b
> blob + 91a8e1117f4859026db8adaa01d951ce1d9b4c11
> --- usr.sbin/vmd/vmd.h
> +++ usr.sbin/vmd/vmd.h
> @@ -518,7 +518,8 @@ int host(const char *, struct address *);
> /* parse.y */
> int parse_config(const char *);
> int cmdline_symset(char *);
> -int host(const char *, struct address *);
> +int parse_prefix4(const char *, struct address *, const char **);
> +int parse_prefix6(const char *, struct address *, const char **);
>
> /* virtio.c */
> int virtio_get_base(int, char *, size_t, int, const char *);
>
--
:wq Claudio
