I'll bite, using text from your regress. > +pass out proto tcp all user 1234:12345 flags S/SA > +pass out proto tcp all user 0:12345 flags S/SA > +pass out proto tcp all group 1234:12345 flags S/SA > +pass out proto tcp all group 0:12345 flags S/SA
What does 1234:12345 mean. It must be uid 1234 _and_ gid 12345? I don't quite understand, what is the purpose to this precise check? How often does this kind of precise request happen? i would expect the normal pattern is to pass "for this uid" or "for this gid", but I'm surprised to see "for this uid AND this gid, both must match", and I doubt you are representing "either this uid or this gid". Can you explain the use case? Vadim Zhukov <[email protected]> wrote: > I've just found that pfctl doesn't like 'X:Y' syntax for user and group > clauses, despite of the words in manpage. The problem is caused by > parser eating the colon character in STRING version of "uid" and "gid" > rules. The solution is similar to the way ports parsing is done. Now we > have "uidrange" and "gidrange" rules, similar to "portrange". I didn't > try to unify those two (or even three) to avoid increasing parentheses > madness, but if somebody would come with better diff/idea, I'm open. :) > > This diff also adds a regression test, for the sake of completeness. > Existing regression tests pass on amd64. > > OK? > > -- > WBR, > Vadim Zhukov > > > Index: sbin/pfctl/parse.y > =================================================================== > RCS file: /cvs/src/sbin/pfctl/parse.y,v > retrieving revision 1.699 > diff -u -p -r1.699 parse.y > --- sbin/pfctl/parse.y 17 Oct 2019 21:54:28 -0000 1.699 > +++ sbin/pfctl/parse.y 16 Jan 2020 00:44:14 -0000 > @@ -468,6 +468,7 @@ typedef struct { > #define PPORT_RANGE 1 > #define PPORT_STAR 2 > int parseport(char *, struct range *r, int); > +int parse_ugid(char *, struct range *r, int); > > #define DYNIF_MULTIADDR(addr) ((addr).type == PF_ADDR_DYNIFTL && \ > (!((addr).iflags & PFI_AFLAG_NOALIAS) || \ > @@ -496,7 +497,7 @@ int parseport(char *, struct range *r, i > %token <v.number> NUMBER > %token <v.i> PORTBINARY > %type <v.interface> interface if_list if_item_not if_item > -%type <v.number> number icmptype icmp6type uid gid > +%type <v.number> number icmptype icmp6type > %type <v.number> tos not yesno optnodf > %type <v.probability> probability > %type <v.weight> optweight > @@ -504,7 +505,7 @@ int parseport(char *, struct range *r, i > %type <v.i> sourcetrack flush unaryop statelock > %type <v.b> action > %type <v.b> flags flag blockspec prio > -%type <v.range> portplain portstar portrange > +%type <v.range> portplain portstar portrange uidrange > gidrange > %type <v.hashkey> hashkey > %type <v.proto> proto proto_list proto_item > %type <v.number> protoval > @@ -2957,69 +2958,67 @@ uid_list : uid_item optnl { $$ = > $1; } > } > ; > > -uid_item : uid { > +uid_item : uidrange { > $$ = calloc(1, sizeof(struct node_uid)); > if ($$ == NULL) > err(1, "uid_item: calloc"); > - $$->uid[0] = $1; > - $$->uid[1] = $1; > - $$->op = PF_OP_EQ; > + $$->uid[0] = $1.a; > + $$->uid[1] = $1.b; > + if ($1.t) > + $$->op = PF_OP_RRG; > + else > + $$->op = PF_OP_EQ; > $$->next = NULL; > $$->tail = $$; > } > - | unaryop uid { > - if ($2 == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) { > + | unaryop uidrange { > + if ($2.a == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) { > yyerror("user unknown requires operator = or " > "!="); > YYERROR; > } > + if ($2.t) { > + yyerror("':' cannot be used with an other " > + "user operator"); > + YYERROR; > + } > $$ = calloc(1, sizeof(struct node_uid)); > if ($$ == NULL) > err(1, "uid_item: calloc"); > - $$->uid[0] = $2; > - $$->uid[1] = $2; > + $$->uid[0] = $2.a; > + $$->uid[1] = $2.b; > $$->op = $1; > $$->next = NULL; > $$->tail = $$; > } > - | uid PORTBINARY uid { > - if ($1 == -1 || $3 == -1) { > + | uidrange PORTBINARY uidrange { > + if ($1.a == -1 || $3.a == -1) { > yyerror("user unknown requires operator = or " > "!="); > YYERROR; > } > + if ($1.t || $3.t) { > + yyerror("':' cannot be used with an other " > + "user operator"); > + YYERROR; > + } > $$ = calloc(1, sizeof(struct node_uid)); > if ($$ == NULL) > err(1, "uid_item: calloc"); > - $$->uid[0] = $1; > - $$->uid[1] = $3; > + $$->uid[0] = $1.a; > + $$->uid[1] = $3.a; > $$->op = $2; > $$->next = NULL; > $$->tail = $$; > } > ; > > -uid : STRING { > - if (!strcmp($1, "unknown")) > - $$ = -1; > - else { > - uid_t uid; > - > - if (uid_from_user($1, &uid) == -1) { > - yyerror("unknown user %s", $1); > - free($1); > - YYERROR; > - } > - $$ = uid; > - } > - free($1); > - } > - | NUMBER { > - if ($1 < 0 || $1 >= UID_MAX) { > - yyerror("illegal uid value %lld", $1); > +uidrange : numberstring { > + if (parse_ugid($1, &$$, 1) == -1) { > + free($1); > YYERROR; > } > - $$ = $1; > + free($1); > } > ; > > @@ -3035,69 +3034,67 @@ gid_list : gid_item optnl { $$ = > $1; } > } > ; > > -gid_item : gid { > +gid_item : gidrange { > $$ = calloc(1, sizeof(struct node_gid)); > if ($$ == NULL) > err(1, "gid_item: calloc"); > - $$->gid[0] = $1; > - $$->gid[1] = $1; > - $$->op = PF_OP_EQ; > + $$->gid[0] = $1.a; > + $$->gid[1] = $1.b; > + if ($1.t) > + $$->op = PF_OP_RRG; > + else > + $$->op = PF_OP_EQ; > $$->next = NULL; > $$->tail = $$; > } > - | unaryop gid { > - if ($2 == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) { > + | unaryop gidrange { > + if ($2.a == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) { > yyerror("group unknown requires operator = or " > "!="); > YYERROR; > } > + if ($2.t) { > + yyerror("':' cannot be used with an other " > + "group operator"); > + YYERROR; > + } > $$ = calloc(1, sizeof(struct node_gid)); > if ($$ == NULL) > err(1, "gid_item: calloc"); > - $$->gid[0] = $2; > - $$->gid[1] = $2; > + $$->gid[0] = $2.a; > + $$->gid[1] = $2.b; > $$->op = $1; > $$->next = NULL; > $$->tail = $$; > } > - | gid PORTBINARY gid { > - if ($1 == -1 || $3 == -1) { > + | gidrange PORTBINARY gidrange { > + if ($1.a == -1 || $3.a == -1) { > yyerror("group unknown requires operator = or " > "!="); > YYERROR; > } > + if ($1.t || $3.t) { > + yyerror("':' cannot be used with an other " > + "group operator"); > + YYERROR; > + } > $$ = calloc(1, sizeof(struct node_gid)); > if ($$ == NULL) > err(1, "gid_item: calloc"); > - $$->gid[0] = $1; > - $$->gid[1] = $3; > + $$->gid[0] = $1.a; > + $$->gid[1] = $3.a; > $$->op = $2; > $$->next = NULL; > $$->tail = $$; > } > ; > > -gid : STRING { > - if (!strcmp($1, "unknown")) > - $$ = -1; > - else { > - gid_t gid; > - > - if (gid_from_group($1, &gid) == -1) { > - yyerror("unknown group %s", $1); > - free($1); > - YYERROR; > - } > - $$ = gid; > - } > - free($1); > - } > - | NUMBER { > - if ($1 < 0 || $1 >= GID_MAX) { > - yyerror("illegal gid value %lld", $1); > +gidrange : numberstring { > + if (parse_ugid($1, &$$, 0) == -1) { > + free($1); > YYERROR; > } > - $$ = $1; > + free($1); > } > ; > > @@ -5802,6 +5799,56 @@ parseport(char *port, struct range *r, i > } > yyerror("port is invalid: %s", port); > return (-1); > +} > + > +int > +parse_ugid(char *spec, struct range *r, int is_uid) > +{ > + uid_t id; > + const char *errstr; > + char *p; > + > + if ((p = strchr(spec, ':')) != NULL) > + *p++ = '\0'; > + > + id = strtonum(spec, 0, is_uid ? UID_MAX : GID_MAX, &errstr); > + if (errstr) { > + if (strcmp("unknown", spec) == 0) > + id = -1; > + else if (is_uid && uid_from_user(spec, &id) == -1) > + return -1; > + else if (!is_uid && gid_from_group(spec, &id) == -1) > + return -1; > + } > + r->a = (int)id; > + > + if (p == NULL) { > + r->b = 0; > + r->t = PF_OP_NONE; > + } else { > + id = strtonum(p, 0, is_uid ? UID_MAX : GID_MAX, &errstr); > + if (errstr) { > + if (strcmp("unknown", spec) == 0) > + id = -1; > + else if (is_uid && uid_from_user(spec, &id) == -1) > + goto range_fail; > + else if (!is_uid && gid_from_group(spec, &id) == -1) > + goto range_fail; > + } > + > + if (id == r->a) { > + r->b = 0; > + r->t = PF_OP_NONE; > + } else { > + r->b = (int)id; > + r->t = PF_OP_RRG; > + } > + } > + return 0; > + > +range_fail: > + *p = ':'; > + return -1; > } > > int > Index: regress/sbin/pfctl/Makefile > =================================================================== > RCS file: /cvs/src/regress/sbin/pfctl/Makefile,v > retrieving revision 1.231 > diff -u -p -r1.231 Makefile > --- regress/sbin/pfctl/Makefile 12 Dec 2017 19:49:19 -0000 1.231 > +++ regress/sbin/pfctl/Makefile 16 Jan 2020 00:44:14 -0000 > @@ -17,7 +17,7 @@ PFTESTS=1 2 3 4 5 6 7 8 9 10 11 12 13 14 > PFTESTS+=28 29 30 31 32 34 35 36 38 39 40 41 44 46 47 48 49 50 > PFTESTS+=52 53 54 55 56 57 60 61 65 66 67 68 69 70 71 72 73 > PFTESTS+=74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 > -PFTESTS+=97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 > +PFTESTS+=97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 114 > PFFAIL=1 2 3 4 5 6 7 8 11 12 13 14 15 16 17 19 20 23 25 27 > PFFAIL+=30 37 38 39 40 41 42 43 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 > 62 > PFFAIL+=63 64 65 66 67 > Index: regress/sbin/pfctl/pf114.in > =================================================================== > RCS file: regress/sbin/pfctl/pf114.in > diff -N regress/sbin/pfctl/pf114.in > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ regress/sbin/pfctl/pf114.in 16 Jan 2020 00:44:14 -0000 > @@ -0,0 +1,5 @@ > +# check that X:Y syntax works for users and groups > +pass out proto tcp all user 1234:12345 > +pass out proto tcp all user root:12345 > +pass out proto tcp all group 1234:12345 > +pass out proto tcp all group wheel:12345 > Index: regress/sbin/pfctl/pf114.ok > =================================================================== > RCS file: regress/sbin/pfctl/pf114.ok > diff -N regress/sbin/pfctl/pf114.ok > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ regress/sbin/pfctl/pf114.ok 16 Jan 2020 00:44:14 -0000 > @@ -0,0 +1,4 @@ > +pass out proto tcp all user 1234:12345 flags S/SA > +pass out proto tcp all user 0:12345 flags S/SA > +pass out proto tcp all group 1234:12345 flags S/SA > +pass out proto tcp all group 0:12345 flags S/SA >
