On Wed, Sep 16, 2020 at 06:22:00PM +0900, YASUOKA Masahiko wrote:
> Let me continue this separetely.
Yes, let's get your diff in for release and then work out the other
approach.
> Make pfctl check if the rtable really exists when parsing the config.
The diff is a bit hard to read (nothing you can do), but after applying
the code reads good in principal.
> Index: sbin/pfctl/parse.y
> ===================================================================
> RCS file: /cvs/src/sbin/pfctl/parse.y,v
> retrieving revision 1.701
> diff -u -p -r1.701 parse.y
> --- sbin/pfctl/parse.y 28 Jan 2020 15:40:35 -0000 1.701
> +++ sbin/pfctl/parse.y 16 Sep 2020 09:11:21 -0000
> @@ -392,7 +392,9 @@ int invalid_redirect(struct node_host *
> u_int16_t parseicmpspec(char *, sa_family_t);
> int kw_casecmp(const void *, const void *);
> int map_tos(char *string, int *);
> +int get_domainid(u_int);
> int rdomain_exists(u_int);
> +int rtable_exists(u_int);
> int filteropts_to_rule(struct pf_rule *, struct filter_opts *);
>
> TAILQ_HEAD(loadanchorshead, loadanchors)
> @@ -1217,6 +1219,10 @@ antispoof_opt : LABEL label {
> yyerror("invalid rtable id");
> YYERROR;
> }
> + else if (rtable_exists($2) != 1) {
Using the function verb would reads a bit clearer/more intuitive, i.e.
else if (!rtable_exists($2)) {
> + yyerror("rtable %lld does not exist", $2);
> + YYERROR;
> + }
> antispoof_opts.rtableid = $2;
> }
> ;
> @@ -2001,6 +2007,10 @@ filter_opt : USER uids {
> yyerror("invalid rtable id");
> YYERROR;
> }
> + else if (rtable_exists($2) != 1) {
Same here.
> + yyerror("rtable %lld does not exist", $2);
> + YYERROR;
> + }
> filter_opts.rtableid = $2;
> }
> | DIVERTTO STRING PORT portplain {
> @@ -5868,15 +5878,15 @@ map_tos(char *s, int *val)
> }
>
> int
> -rdomain_exists(u_int rdomain)
> +get_domainid(u_int rdomain)
> {
> size_t len;
> struct rt_tableinfo info;
> int mib[6];
> - static u_int found[RT_TABLEID_MAX+1];
> + static u_int domainid[RT_TABLEID_MAX+1];
>
> - if (found[rdomain] == 1)
> - return 1;
> + if (domainid[rdomain] != 0)
> + return domainid[rdomain];
>
> mib[0] = CTL_NET;
> mib[1] = PF_ROUTE;
> @@ -5887,17 +5897,37 @@ rdomain_exists(u_int rdomain)
>
> len = sizeof(info);
> if (sysctl(mib, 6, &info, &len, NULL, 0) == -1) {
> - if (errno == ENOENT) {
> + if (errno == ENOENT)
> /* table nonexistent */
> - return 0;
> - }
> - err(1, "%s", __func__);
> - }
> - if (info.rti_domainid == rdomain) {
> - found[rdomain] = 1;
> + domainid[rdomain] = RT_TABLEID_MAX;
This does not look correct, RT_TABLEID_MAX (255) is the biggest *valid*
id, so you cannot use it to denote a nonexistent routing table.
$ doas ifconfig lo255 rdomain 255
$ netstat -R | grep 255
Rdomain 255
Interface: lo255
Routing table: 255
Perhaps use `static int domainid[RT_TABLEID_MAX+1]' and `-1' to reflect
ENOENT?
> + else
> + err(1, "%s", __func__);
> + } else
> + domainid[rdomain] = info.rti_domainid;
> +
> + return domainid[rdomain];
> +}
> +
> +int
> +rdomain_exists(u_int rdomain)
> +{
> + int domainid;
> +
> + domainid = get_domainid(rdomain);
> + if (domainid == rdomain)
> return 1;
> - }
> /* rdomain is a table, but not an rdomain */
> + return 0;
> +}
> +
> +int
> +rtable_exists(u_int rtable)
> +{
> + int domainid;
> +
> + domainid = get_domainid(rtable);
> + if (domainid < RT_TABLEID_MAX)
As per above, RT_TABLEID_MAX is valid.
> + return 1;
> return 0;
> }
