Re: pfctl: defuse `-F all -i ...'
Hello,
> mcbride introduced this code with r1.298 in 2010 but used
>
> if (*ifaceopt) {
>
> only to have stsp fix a segfault in r1.299 by changing it to the current
> form.
>
> One might as well assume that my proposed condition was the originally
> intended behaviour after all and stsp's fix should've just removed the
> derefence.
>
> OK?
OK sashan@
>
> Index: pfctl.c
> ===
> RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
> retrieving revision 1.362
> diff -u -p -r1.362 pfctl.c
> --- pfctl.c 2 Jan 2019 23:08:00 - 1.362
> +++ pfctl.c 5 Jan 2019 22:01:56 -
> @@ -2626,13 +2626,13 @@ main(int argc, char *argv[])
> pfctl_clear_stats(dev, ifaceopt, opts);
> break;
> case 'a':
> - pfctl_clear_tables(anchorname, opts);
> - pfctl_clear_rules(dev, opts, anchorname);
> - if (ifaceopt && *ifaceopt) {
> + if (ifaceopt) {
> warnx("don't specify an interface with -Fall");
> usage();
> /* NOTREACHED */
> }
> + pfctl_clear_tables(anchorname, opts);
> + pfctl_clear_rules(dev, opts, anchorname);
> if (!*anchorname) {
> pfctl_clear_states(dev, ifaceopt, opts);
> pfctl_clear_src_nodes(dev, opts);
>
Re: pfctl: defuse `-F all -i ...'
On Sat, Jan 05, 2019 at 08:04:07PM +0100, Klemens Nanni wrote:
> Diff below bails out immediately when `-i ...' is passed
Just that now.
Ignore the option argument if the option was passed since that already
fulfills our error condition of passing `-i ...' with `-F all'.
`ifaceopt' is global and therefore NULL initialised.
Still clearing tables and rules when this error is hit seems wrong to
me, so don't do anything unless the pfctl command is actually valid.
mcbride introduced this code with r1.298 in 2010 but used
if (*ifaceopt) {
only to have stsp fix a segfault in r1.299 by changing it to the current
form.
One might as well assume that my proposed condition was the originally
intended behaviour after all and stsp's fix should've just removed the
derefence.
OK?
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.362
diff -u -p -r1.362 pfctl.c
--- pfctl.c 2 Jan 2019 23:08:00 - 1.362
+++ pfctl.c 5 Jan 2019 22:01:56 -
@@ -2626,13 +2626,13 @@ main(int argc, char *argv[])
pfctl_clear_stats(dev, ifaceopt, opts);
break;
case 'a':
- pfctl_clear_tables(anchorname, opts);
- pfctl_clear_rules(dev, opts, anchorname);
- if (ifaceopt && *ifaceopt) {
+ if (ifaceopt) {
warnx("don't specify an interface with -Fall");
usage();
/* NOTREACHED */
}
+ pfctl_clear_tables(anchorname, opts);
+ pfctl_clear_rules(dev, opts, anchorname);
if (!*anchorname) {
pfctl_clear_states(dev, ifaceopt, opts);
pfctl_clear_src_nodes(dev, opts);
Re: pfctl: defuse `-F all -i ...', catch empty argument values
On Sat, Jan 05, 2019 at 12:07:59PM -0700, Theo de Raadt wrote:
> + if (!*optarg)
>
> I despise this idiom. You are checking for a zero-length string.
> But you are hiding what is going on.
Because the value is used in many places. Some check for nullity, some
check for emptyness, others just strlcpy() it and leave error checks to
the ioctls.
> But why do you do it at every step??? Why not do it at the end
> after getopt?
How so?
This won't work:
while (ch = getopt(...)) {
switch (ch) {
...
}
/* breaks `-v' et al. unless I check `ch' again */
if (!*optarg)
errx(1, "empty -%c argument", ch);
}
And this is the same check but just scattered and deferred really:
while (...) {
...
}
...
if (ifaceopt && !*ifaceopt)
errx(1, "empty interface name");
...
memset(anchorname, 0, sizeof(anchorname));
if (anchoropt != NULL) {
if (!*anchoropt)
errx(1. "empty anchor name");
...
}
...
> Your diff feels wrong.
Well, it's an effective solution to multiple deficiencies that does not
require major code reshuffling.
I can work on further improvements but checking these tings as early as
possible is one step in the right direction, I think.
pfctl: defuse `-F all -i ...', catch empty argument values
Limiting the "flush all" operation to a specific interface does not
make sense, and the intention was clear as well:
pfctl.c revision 1.298
date: 2010/06/28 23:21:41; author: mcbride; state: Exp; lines: +27
-11;
Clean up iterface stats handling:
- 'make -Fi' reset ALL the interface statistics
can be restricted with -i ifname
- 'make -Fa -i ifname' fail (it's meaningless)
- get rid of a silly little struct that's only used for one thing
ok henning
Yet, tables and rules are always cleared:
# pfctl -F all -i foo
0 tables deleted.
rules cleared
pfctl: don't specify an interface with -Fall
usage: pfctl ...
It even clears statistics for the empty interface:
# pfctl -F all -i ''
0 tables deleted.
rules cleared
8 states cleared
source tracking entries cleared
pf: statistics cleared for interface
pf: interface flags reset
That's unacceptable.
Currently, the idiom `opt && *opt' is used to test for table, key and
interface arguments, but is inherently unable to detect empy values
since the NULL initialised `opt' is unconditionally set to `optarg'
which may be empty.
Diff below bails out immediately when `-i ...' is passed and hoists
empty option argument checks into the getopt(3) routine. All code using
these arguments can now rely on them not being empty if given.
$ ./obj/pfctl -F all -i ''
pfctl: empty interface name
# ./obj/pfctl -F all -i foo
pfctl: don't specify an interface with -Fall
usage: pfctl ...
Feedback? OK?
Regress passes.
NB: parse.y does accept empty strings for anchors, tables, interfaces
and tables and handles them poorly, but that's stuff for another diff.
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.362
diff -u -p -r1.362 pfctl.c
--- pfctl.c 2 Jan 2019 23:08:00 - 1.362
+++ pfctl.c 5 Jan 2019 19:03:07 -
@@ -2342,6 +2342,8 @@ main(int argc, char *argv[])
"a:dD:eqf:F:ghi:k:K:L:Nno:Pp:R:rS:s:t:T:vV:x:z")) != -1) {
switch (ch) {
case 'a':
+ if (!*optarg)
+ errx(1, "emtpy anchor name");
anchoropt = optarg;
break;
case 'd':
@@ -2369,6 +2371,8 @@ main(int argc, char *argv[])
mode = O_RDWR;
break;
case 'i':
+ if (!*optarg)
+ errx(1, "empty interface name");
ifaceopt = optarg;
break;
case 'k':
@@ -2377,6 +2381,8 @@ main(int argc, char *argv[])
usage();
/* NOTREACHED */
}
+ if (!*optarg)
+ errx(1, "empty state key");
state_kill[state_killers++] = optarg;
mode = O_RDWR;
break;
@@ -2386,6 +2392,8 @@ main(int argc, char *argv[])
usage();
/* NOTREACHED */
}
+ if (!*optarg)
+ errx(1, "empty source tracking key");
src_node_kill[src_node_killers++] = optarg;
mode = O_RDWR;
break;
@@ -2434,6 +2442,8 @@ main(int argc, char *argv[])
}
break;
case 't':
+ if (!*optarg)
+ errx(1, "empty table name");
tableopt = optarg;
break;
case 'T':
@@ -2626,13 +2636,13 @@ main(int argc, char *argv[])
pfctl_clear_stats(dev, ifaceopt, opts);
break;
case 'a':
- pfctl_clear_tables(anchorname, opts);
- pfctl_clear_rules(dev, opts, anchorname);
- if (ifaceopt && *ifaceopt) {
+ if (ifaceopt) {
warnx("don't specify an interface with -Fall");
usage();
/* NOTREACHED */
}
+ pfctl_clear_tables(anchorname, opts);
+ pfctl_clear_rules(dev, opts, anchorname);
if (!*anchorname) {
pfctl_clear_states(dev, ifaceopt, opts);
pfctl_clear_src_nodes(dev, opts);
