Re: pfctl should check pfctl.astack is not overrun

[Klemens Nanni]

On Wed, Apr 17, 2019 at 03:06:16PM +0200, Petr Hoffmann wrote:
> I noticed pfctl crashes on segfault when anchors go too deep:
Yes, I've already seen this at some point but didn't get around to
fixing it properly - thanks for the reminder.

 
> It seems there is no check we fit into pfctl.astack[]. The attached
> patch resolves this issue:
Fixing the parser alone does not suffice:

# echo block | pfctl -a $(jot -s/ 66) -f-
# pfctl -vsA -a1 | wc -l
  65

/sys/net/pf.c
160:#define PF_ANCHOR_STACK_MAX 64

This limit is not hit in my example;  I have not yet spend time on this,
but I think the kernel should refuse this.

> 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");
This looks sane.

>   pf->bn++;
>   pf->brace = 1;
>  

pfctl should check pfctl.astack is not overrun

[Petr Hoffmann]

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;