pfctl: anchor names must not be empty, unify sanity checks

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

pfctl: anchor names must not be empty, unify sanity checks

Klemens Nanni-2
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();

Reply | Threaded
Open this post in threaded view
|

Re: pfctl: anchor names must not be empty, unify sanity checks

Alexandr Nedvedicky
Hello,


On Wed, Feb 06, 2019 at 10:20:35PM +0100, Klemens Nanni wrote:

> 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.

    makes sense to me. and change also looks good to me.

Also it looks like tables are suffering from the same problem:

    puffy# echo "table <''> const { 10/8, 172.16/12, 192.168/16 }" |pfctl -nf -
    puffy# echo $?
    0
    puffy# echo "table <''> const { 10/8, 172.16/12, 192.168/16 }" |pfctl -f -
    stdin:1: cannot define table : Invalid argument
    pfctl: Syntax error in config file: pf rules not loaded

We parse.y can be also taught to fail on empty name for table, but it can be
fixed in yet another patch.

OK sashan@