When using anchors, they ought to have a non-empty name or none at all.
By accident, I discovered the following:
$ printf 'anchor ""\n' | pfctl -vnf-
pass all no state
No errors and it parses in a potentially harmful way. Other use cases
behave badly as well:
$ printf 'anchor "" {\n}\n' | pfctl -vnf-
pfctl: anchorrule: unable to create ruleset: Permission denied
$ printf 'load anchor "" from "/dev/null"\n' | pfctl -vnf-
Loading anchor from /dev/null
None of them make sense, so I propose to error out on empty anchor names
as early as possible. Given that typos happen and folks generate their
pf.conf automatically, such errors do not seem entirely out of scope.
The following diff makes `load anchor' use the `anchorname' production
like everything else does in the parser; it moves all sanity checks
in one place and thus also makes `load anchor' benefiet as well
(it currently accepts names beginning with "_1").
`pfctl -a ""' does behave like `pfctl -a /' or no anchor at all, hence
it uses the main anchor. This diff errors out on `-a ""' as well for
consistency and to discourage the use of the empty name, but as it does
not fix anything, this hunk might just be omitted as well.
Regress passes.
Feedback? OK?
Index: sbin/pfctl/parse.y
===================================================================
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.690
diff -u -p -r1.690 parse.y
--- sbin/pfctl/parse.y 31 Jan 2019 18:08:36 -0000 1.690
+++ sbin/pfctl/parse.y 6 Feb 2019 19:52:11 -0000
@@ -809,7 +809,21 @@ varset : STRING '=' varstring {
}
;
-anchorname : STRING { $$ = $1; }
+anchorname : STRING {
+ if ($1[0] == '\0') {
+ free($1);
+ yyerror("anchor name must not be empty");
+ YYERROR;
+ }
+ if (strlen(pf->anchor->path) + 1 +
+ strlen($1) >= PATH_MAX) {
+ free($1);
+ yyerror("anchor name is longer than %u",
+ PATH_MAX - 1);
+ YYERROR;
+ }
+ $$ = $1;
+ }
| /* empty */ { $$ = NULL; }
;
@@ -949,14 +963,11 @@ anchorrule : ANCHOR anchorname dir quick
}
;
-loadrule : LOAD ANCHOR string FROM string {
+loadrule : LOAD ANCHOR anchorname FROM string {
struct loadanchors *loadanchor;
- if (strlen(pf->anchor->path) + 1 +
- strlen($3) >= PATH_MAX) {
- yyerror("anchorname %s too long, max %u\n",
- $3, PATH_MAX - 1);
- free($3);
+ if ($3 == NULL) {
+ yyerror("anchor name is missing");
YYERROR;
}
loadanchor = calloc(1, sizeof(struct loadanchors));
Index: sbin/pfctl/pfctl.c
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.369
diff -u -p -r1.369 pfctl.c
--- sbin/pfctl/pfctl.c 29 Jan 2019 10:58:31 -0000 1.369
+++ sbin/pfctl/pfctl.c 6 Feb 2019 18:58:22 -0000
@@ -2441,6 +2441,8 @@ main(int argc, char *argv[])
memset(anchorname, 0, sizeof(anchorname));
if (anchoropt != NULL) {
+ if (anchoropt[0] == '\0')
+ errx(1, "anchor name must not be empty");
if (mode == O_RDONLY && showopt == NULL && tblcmdopt == NULL) {
warnx("anchors apply to -f, -F, -s, and -T only");
usage();
