pfctl should check pfctl.astack is not overrun
Hi,
I noticed pfctl crashes on segfault when anchors go too deep:
--8<---
$ cat ~/pf.conf | head -5
anchor foo {
anchor foo {
anchor foo {
anchor foo {
anchor foo {
$ grep anchor ~/pf.conf | wc -l
66
$ /sbin/pfctl -nf ~/pf.conf
Segmentation fault (core dumped)
--->8--
It seems there is no check we fit into pfctl.astack[]. The attached
patch resolves this issue:
--8<---
$ ./pfctl -nf ~/pf.conf
pfctl: pfa_anchor: anchors too deep
$ grep anchor ~/pf2.conf | wc -l
63
$ ./pfctl -nf ~/pf2.conf
$
--->8--
Petr
diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index 1e7ce21..5e19c5f39da 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -846,6 +846,8 @@ pfa_anchor : '{'
/* steping into a brace anchor */
pf->asd++;
+ if (pf->asd >= PFCTL_ANCHOR_STACK_DEPTH)
+ errx(1, "pfa_anchor: anchors too deep");
pf->bn++;
pf->brace = 1;
Re: introduce 'pfctl -FR' to reset settings to defaults
On 02.04.2019 12:06, Klemens Nanni wrote: On Tue, Apr 02, 2019 at 11:28:43AM +0200, Petr Hoffmann wrote: would make me believe everything mentioned as OPTIONS in pf.conf(5) is about to be reset. I see e.g. the debug level is reset, but what about the other stuff like fingerprints, 'skip on' and other options set via the 'set' command? Maybe the manpage should be more precise here? Seems fine to me, given that a) some options are not persisted in the driver but only effective during ruleset parsing and b) stuff like fingerprints is already flushed separately, see pfctl(8) `-F osfp'. For me, forcing the user to think what is meant by 'options' is not very friendly, though I understand the idea behind *some* options being used only while parsing. Let's assume I'm the smart user who is able to distinguish them. But then, 'set skip on' is the persistent one, right, but still not reset, I guess. Petr On 02.04.2019 12:06, Klemens Nanni wrote: On Tue, Apr 02, 2019 at 11:28:43AM +0200, Petr Hoffmann wrote: would make me believe everything mentioned as OPTIONS in pf.conf(5) is about to be reset. I see e.g. the debug level is reset, but what about the other stuff like fingerprints, 'skip on' and other options set via the 'set' command? Maybe the manpage should be more precise here? Seems fine to me, given that a) some options are not persisted in the driver but only effective during ruleset parsing and b) stuff like fingerprints is already flushed separately, see pfctl(8) `-F osfp'.
Re: introduce 'pfctl -FR' to reset settings to defaults
Hi,
seeing this in the manpage
--8<--
+.It Fl F Cm Reset
+Reset limits, timeouts and options back to default settings.
-->8--
would make me believe everything mentioned as OPTIONS in pf.conf(5) is
about to be reset. I see e.g. the debug level is reset, but what about
the other stuff like fingerprints, 'skip on' and other options set via
the 'set' command? Maybe the manpage should be more precise here?
PH
On 02.04.2019 9:40, Alexandr Nedvedicky wrote:
Hello,
below is diff I plan to commit. I did add a comment to pfctl_reset()
and wording in manpage.
thanks and
regards
sashan
8<---8<---8<---8<---
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..00bd27c200a 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -197,6 +197,8 @@ Flush the filter information (statistics that are not bound
to rules).
Flush the tables.
.It Fl F Cm osfp
Flush the passive operating system fingerprints.
+.It Fl F Cm Reset
+Reset limits, timeouts and options back to default settings.
.It Fl F Cm all
Flush all of the above.
.El
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..40929d90530 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -105,6 +105,7 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule
*, int);
const char*pfctl_lookup_option(char *, const char **);
void pfctl_state_store(int, const char *);
void pfctl_state_load(int, const char *);
+void pfctl_reset(int, int);
const char *clearopt;
char *rulesopt;
@@ -205,7 +206,8 @@ static const struct {
};
static const char *clearopt_list[] = {
- "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
+ "rules", "Sources", "states", "info", "Tables", "osfp", "Reset",
+ "all", NULL
};
static const char *showopt_list[] = {
@@ -2232,6 +2234,41 @@ pfctl_state_load(int dev, const char *file)
fclose(f);
}
+void
+pfctl_reset(int dev, int opts)
+{
+ struct pfctlpf;
+ struct pfr_buffer t;
+ int i;
+
+ pf.dev = dev;
+ pfctl_init_options(&pf);
+
+ /* Force reset upon pfctl_load_options() */
+ pf.debug_set = 1;
+ pf.reass_set = 1;
+ pf.syncookieswat_set = 1;
+ pf.ifname = strdup("none");
+ pf.ifname_set = 1;
+
+ memset(&t, 0, sizeof(t));
+ t.pfrb_type = PFRB_TRANS;
+ if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
+ warn("%s, DIOCXBEGIN", __func__);
+
+
+ for (i = 0; pf_limits[i].name; i++)
+ pf.limit_set[pf_limits[i].index] = 1;
+
+ for (i = 0; pf_timeouts[i].name; i++)
+ pf.timeout_set[pf_timeouts[i].timeout] = 1;
+
+ pfctl_load_options(&pf);
+
+ if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
+ warn("%s, DIOCXCOMMIT", __func__);
+}
+
int
main(int argc, char *argv[])
{
@@ -2558,6 +2595,7 @@ main(int argc, char *argv[])
pfctl_clear_stats(dev, ifaceopt, opts);
pfctl_clear_fingerprints(dev, opts);
pfctl_clear_interface_flags(dev, opts);
+ pfctl_reset(dev, opts);
}
break;
case 'o':
@@ -2566,6 +2604,9 @@ main(int argc, char *argv[])
case 'T':
pfctl_clear_tables(anchorname, opts);
break;
+ case 'R':
+ pfctl_reset(dev, opts);
+ break;
}
}
if (state_killers) {
invalid netmasks should be reported
Hi,
I noticed it is possible to specify an invalid netmask,
e.g. 1.1.1.1/10/20 and still get the address loaded into a table. I
conjecture this was introduced by the following change:
a7ede25358dad545e0342d2a9f8ef6ce68c6df66
Zap bits in host_v4(), use mask parameter
It looks like the author missed the path addresses are loaded by pfctl's
'-T add' command. I guess the '/20' is dropped in host() and then '/10'
is processed within host_ip() by inet_net_pton() so no error is reported.
The proposed patch is attached. For me it works:
OLD:
# pfctl -t tableta -T add 1.1.1.1/10/20
1 table created.
1/1 addresses added.
NEW:
# $PFCTL -t tableta -T add
1.1.1.1/10/20
netmask is invalid: /10/20
Petr
diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
index ee3c2926f1a..5737846123d 100644
--- a/sbin/pfctl/pfctl_parser.c
+++ b/sbin/pfctl/pfctl_parser.c
@@ -1627,7 +1627,7 @@ host(const char *s, int opts)
if_name++;
}
- if ((p = strrchr(ps, '/')) != NULL) {
+ if ((p = strchr(ps, '/')) != NULL) {
mask = strtonum(p+1, 0, 128, &errstr);
if (errstr) {
fprintf(stderr, "netmask is %s: %s\n", errstr, p);
Re: once rules fix
Klemens Nanni writes:
> Thanks! Diff makes sense, see comments inline. I confirm that this
> restores intended behaviour and regress is fine as well.
>
> With those addressed OK kn; or I take care of it after getting an OK.
> sashan?
Thanks for pointing to the details. Fixed now:
diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index e8dd97f6222..ceca208ab71 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -354,7 +354,7 @@ struct pfctl_watermarks syncookie_opts;
int disallow_table(struct node_host *, const char *);
int disallow_urpf_failed(struct node_host *, const char *);
int disallow_alias(struct node_host *, const char *);
-int rule_consistent(struct pf_rule *, int);
+int rule_consistent(struct pf_rule *);
int process_tabledef(char *, struct table_opts *, int);
voidexpand_label_str(char *, size_t, const char *, const char *);
voidexpand_label_if(const char *, char *, size_t, const char *);
@@ -377,8 +377,7 @@ void expand_rule(struct pf_rule *, int,
struct node_if *,
struct node_proto *,
struct node_os *, struct node_host *, struct node_port *,
struct node_host *, struct node_port *, struct node_uid *,
- struct node_gid *, struct node_if *, struct node_icmp *,
- const char *);
+ struct node_gid *, struct node_if *, struct node_icmp *);
int expand_queue(char *, struct node_if *, struct queue_opts *);
int expand_skip_interface(struct node_if *);
@@ -876,6 +875,7 @@ anchorrule : ANCHOR anchorname dir quick interface af
proto fromto
{
struct pf_rule r;
struct node_proto *proto;
+ char*p;
memset(&r, 0, sizeof(r));
if (pf->astack[pf->asd + 1]) {
@@ -913,7 +913,33 @@ anchorrule : ANCHOR anchorname dir quick interface af
proto fromto
"rules must specify a name");
YYERROR;
}
+
+ /*
+* Don't make non-brace anchors part of the
main anchor pool.
+*/
+ if ((r.anchor = calloc(1, sizeof(*r.anchor)))
== NULL) {
+ err(1, "anchorrule: calloc");
+ }
+ pf_init_ruleset(&r.anchor->ruleset);
+ r.anchor->ruleset.anchor = r.anchor;
+ if (strlcpy(r.anchor->path, $2,
+ sizeof(r.anchor->path)) >=
sizeof(r.anchor->path)) {
+ errx(1, "anchorrule: strlcpy");
+ }
+ if ((p = strrchr($2, '/')) != NULL) {
+ if (strlen(p) == 1) {
+ yyerror("anchorrule: bad anchor
name %s",
+ $2);
+ YYERROR;
+ }
+ } else
+ p = $2;
+ if (strlcpy(r.anchor->name, p,
+ sizeof(r.anchor->name)) >=
sizeof(r.anchor->name)) {
+ errx(1, "anchorrule: strlcpy");
+ }
}
+
r.direction = $3;
r.quick = $4.quick;
r.af = $6;
@@ -955,8 +981,7 @@ anchorrule : ANCHOR anchorname dir quick interface af
proto fromto
expand_rule(&r, 0, $5, NULL, NULL, NULL, $7, $8.src_os,
$8.src.host, $8.src.port, $8.dst.host, $8.dst.port,
- $9.uid, $9.gid, $9.rcv, $9.icmpspec,
- pf->astack[pf->asd + 1] ? pf->alast->name : $2);
+ $9.uid, $9.gid, $9.rcv, $9.icmpspec);
free($2);
pf->astack[pf->asd + 1] = NULL;
}
@@ -1110,7 +1135,7 @@ antispoof : ANTISPOOF logquick antispoof_ifspc af
antispoof_opts {
if (h != NULL)
expand_rule(&r, 0, j, NULL, NULL, NULL,
NULL, NULL, h, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL, "");
+ NULL, NULL, NULL, NULL);
if ((i->ifa_flags & IFF_LOOPBACK) == 0) {
Re: once rules fix
Sorry, my MUA replaced tabs with spaces in the patch I sent
previously. Find the correct one below:
diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index e8dd97f6222..e55b2893069 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -354,7 +354,7 @@ struct pfctl_watermarks syncookie_opts;
int disallow_table(struct node_host *, const char *);
int disallow_urpf_failed(struct node_host *, const char *);
int disallow_alias(struct node_host *, const char *);
-int rule_consistent(struct pf_rule *, int);
+int rule_consistent(struct pf_rule *);
int process_tabledef(char *, struct table_opts *, int);
voidexpand_label_str(char *, size_t, const char *, const char *);
voidexpand_label_if(const char *, char *, size_t, const char *);
@@ -377,8 +377,7 @@ void expand_rule(struct pf_rule *, int,
struct node_if *,
struct node_proto *,
struct node_os *, struct node_host *, struct node_port *,
struct node_host *, struct node_port *, struct node_uid *,
- struct node_gid *, struct node_if *, struct node_icmp *,
- const char *);
+ struct node_gid *, struct node_if *, struct node_icmp *);
int expand_queue(char *, struct node_if *, struct queue_opts *);
int expand_skip_interface(struct node_if *);
@@ -876,6 +875,7 @@ anchorrule : ANCHOR anchorname dir quick interface af
proto fromto
{
struct pf_rule r;
struct node_proto *proto;
+ char*p;
memset(&r, 0, sizeof(r));
if (pf->astack[pf->asd + 1]) {
@@ -913,7 +913,33 @@ anchorrule : ANCHOR anchorname dir quick interface af
proto fromto
"rules must specify a name");
YYERROR;
}
+
+ /*
+* Don't make non-brace anchors part of the
main anchor pool.
+*/
+ if ((r.anchor = calloc(1, sizeof(*r.anchor)))
== NULL) {
+ err(1, "anchorrule: calloc");
+ }
+ pf_init_ruleset(&r.anchor->ruleset);
+ r.anchor->ruleset.anchor = r.anchor;
+ if (strlcpy(r.anchor->path, $2,
+ sizeof(r.anchor->path)) >=
sizeof(r.anchor->path)) {
+ errx(1, "anchorrule: strlcpy");
+ }
+ if ((p = strrchr($2, '/')) != NULL) {
+ if (strlen(p) == 1) {
+ yyerror("anchorrule: bad anchor
name %s",
+ $2);
+ YYERROR;
+ }
+ } else
+ p = (char *)$2;
+ if (strlcpy(r.anchor->name, p,
+ sizeof(r.anchor->name)) >=
sizeof(r.anchor->name)) {
+ errx(1, "anchorrule: strlcpy");
+ }
}
+
r.direction = $3;
r.quick = $4.quick;
r.af = $6;
@@ -955,8 +981,7 @@ anchorrule : ANCHOR anchorname dir quick interface af
proto fromto
expand_rule(&r, 0, $5, NULL, NULL, NULL, $7, $8.src_os,
$8.src.host, $8.src.port, $8.dst.host, $8.dst.port,
- $9.uid, $9.gid, $9.rcv, $9.icmpspec,
- pf->astack[pf->asd + 1] ? pf->alast->name : $2);
+ $9.uid, $9.gid, $9.rcv, $9.icmpspec);
free($2);
pf->astack[pf->asd + 1] = NULL;
}
@@ -1110,7 +1135,7 @@ antispoof : ANTISPOOF logquick antispoof_ifspc af
antispoof_opts {
if (h != NULL)
expand_rule(&r, 0, j, NULL, NULL, NULL,
NULL, NULL, h, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL, "");
+ NULL, NULL, NULL, NULL);
if ((i->ifa_flags & IFF_LOOPBACK) == 0) {
bzero(&r, sizeof(r));
@@ -1132,7 +1157,7 @@ antispoof : ANTISPOOF logquick antispoof_ifspc af
antispoof_opts {
expand_rule(&
once rules fix
Hi,
I noticed that pfctl says 'once' can be used only with pass/block rules,
but it is not true - it can't for block but can for anchor rules:
--8<---
# echo 'block once' | pfctl -f -
stdin:1: 'once' only applies to pass/block rules
pfctl: Syntax error in config file: pf rules not loaded
# echo 'anchor xxx once' | pfctl -f -
# pfctl -sr
anchor "xxx" all once
--->8--
The patch below resolves the issue. Note that pf_rule.action has no
special constant for anchors. The patch uses pf_rule.anchor to recognize
an anchor rule. To do this, I moved a piece of code from pfctl_add_rule()
to parse.y so that the anchor is created early and pf_rule.anchor becomes
non-NULL. This further allowed tosimplify expand_rule() and
pfctl_add_rule() by shortening their parameterlists.
The patched pfctl responds as expected:
--8<---
# echo 'block once' | ./pfctl -f -
# pfctl -sr
block drop all once
# echo 'anchor xxx once' | ./pfctl -f -
stdin:1: 'once' only applies to pass/block rules
pfctl: Syntax error in config file: pf rules not loaded
--->8--
diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index e8dd97f6222..ce866883b04 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -354,7 +354,7 @@ struct pfctl_watermarks syncookie_opts;
int disallow_table(struct node_host *, const char *);
int disallow_urpf_failed(struct node_host *, const char *);
int disallow_alias(struct node_host *, const char *);
-int rule_consistent(struct pf_rule *, int);
+int rule_consistent(struct pf_rule *);
int process_tabledef(char *, struct table_opts *, int);
void expand_label_str(char *, size_t, const char *, const char *);
void expand_label_if(const char *, char *, size_t, const char *);
@@ -377,8 +377,7 @@ void expand_rule(struct pf_rule *, int,
struct node_if *,
struct node_proto *,
struct node_os *, struct node_host *, struct node_port *,
struct node_host *, struct node_port *, struct node_uid *,
- struct node_gid *, struct node_if *, struct node_icmp *,
- const char *);
+ struct node_gid *, struct node_if *, struct node_icmp *);
int expand_queue(char *, struct node_if *, struct queue_opts *);
int expand_skip_interface(struct node_if *);
@@ -876,6 +875,7 @@ anchorrule : ANCHOR anchorname dir quick
interface af proto fromto
{
struct pf_rule r;
struct node_proto *proto;
+ char *p;
memset(&r, 0, sizeof(r));
if (pf->astack[pf->asd + 1]) {
@@ -913,7 +913,33 @@ anchorrule : ANCHOR anchorname dir quick
interface af proto fromto
"rules must specify a name");
YYERROR;
}
+
+ /*
+ * Don't make non-brace anchors part of the main anchor
pool.
+ */
+ if ((r.anchor = calloc(1, sizeof(*r.anchor))) == NULL) {
+ err(1, "anchorrule: calloc");
+ }
+ pf_init_ruleset(&r.anchor->ruleset);
+ r.anchor->ruleset.anchor = r.anchor;
+ if (strlcpy(r.anchor->path, $2,
+ sizeof(r.anchor->path)) >= sizeof(r.anchor->path)) {
+ errx(1, "anchorrule: strlcpy");
+ }
+ if ((p = strrchr($2, '/')) != NULL) {
+ if (strlen(p) == 1) {
+ yyerror("anchorrule: bad anchor name %s",
+ $2);
+ YYERROR;
+ }
+ } else
+ p = (char *)$2;
+ if (strlcpy(r.anchor->name, p,
+ sizeof(r.anchor->name)) >= sizeof(r.anchor->name)) {
+ errx(1, "anchorrule: strlcpy");
+ }
}
+
r.direction = $3;
r.quick = $4.quick;
r.af = $6;
@@ -955,8 +981,7 @@ anchorrule : ANCHOR anchorname dir quick
interface af proto fromto
expand_rule(&r, 0, $5, NULL, NULL, NULL, $7, $8.src_os,
$8.src.host, $8.src.port, $8.dst.host, $8.dst.port,
- $9.uid, $9.gid, $9.rcv, $9.icmpspec,
- pf->astack[pf->asd + 1] ? pf->alast->name : $2);
+ $9.uid, $9.gid, $9.rcv, $9.icmpspec);
free($2);
pf->astack[pf->asd + 1] = NULL;
}
@@ -1110,7 +1135,7 @@ antispoof : ANTISPOOF logquick antispoof_ifspc
af antispoof_opts {
if (h != NULL)
expand_rule(&r, 0, j, NULL, NULL, NULL,
NULL, NULL, h, NULL, NULL, NULL,
-
