On Wed, Mar 27, 2019 at 12:34:52PM +0100, Petr Hoffmann wrote:
> I noticed it is possible to specify an invalid netmask,
> e.g. 1.1.1.1/10/20 and still get the address loaded into a table. I
> conjecture this was introduced by the following change:
>
> a7ede25358dad545e0342d2a9f8ef6ce68c6df66
> Zap bits in host_v4(), use mask parameter
That was me:
revision 1.326
date: 2018/07/31 22:48:04; author: kn; state: Exp; lines: +10 -11
Zap v4mask and v6mask in host()
Simply defer checks whether a mask has been specified to where it's set
in
host_*(); this is to reduce address family specific code.
OK sashan
> It looks like the author missed the path addresses are loaded by pfctl's '-T
> add' command. I guess the '/20' is dropped in host() and then '/10' is
> processed within host_ip() by inet_net_pton() so no error is reported.
Good find, thanks.
> OLD:
> # pfctl -t tableta -T add 1.1.1.1/10/20
> 1 table created.
> 1/1 addresses added.
>
> NEW:
> # $PFCTL -t tableta -T add
> 1.1.1.1/10/20
> netmask is invalid: /10/20
Yup; this only affects tables, though. For the ruleset parser, strings
with more than one "/" already fail the `xhost' production in parse.y:
$ pfctl -vnf-
pass to 1/2/3
stdin:1: syntax error
> diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
> index ee3c2926f1a..5737846123d 100644
> --- a/sbin/pfctl/pfctl_parser.c
> +++ b/sbin/pfctl/pfctl_parser.c
> @@ -1627,7 +1627,7 @@ host(const char *s, int opts)
> if_name++;
> }
>
> - if ((p = strrchr(ps, '/')) != NULL) {
> + if ((p = strchr(ps, '/')) != NULL) {
> mask = strtonum(p+1, 0, 128, &errstr);
> if (errstr) {
> fprintf(stderr, "netmask is %s: %s\n", errstr, p);
OK kn; anyone else?
