Re: Is panic() the way to handle errors in pf?

[Kristof Provost]

On 1 Aug 2017, at 11:30, Kajetan Staszkiewicz wrote:
> Hey, group.
>
> A thought came to me: is it really the best thing to panic when errors are
> encountered within pf? I understand there are situations where it is safer for
> the kernel to not continue running like some low-level operations in memory
> allocator or filesystems. But a firewall? Especially that a firewall handles
> packets coming from the Interent which can be arbitrarily crafted.
>
pf does not use panic() to handle bad packets, but to handle **impossible** 
situations.
Basically, what you see here are assertions (go count KASSERT() too), not error 
paths.

If it were possible to trigger such a panic by sending a bad packet it would be 
a bug, yes, but that’s not what’s happening here. These panics document 
invariants. They are assertions.
Once the impossible has happened there’s no sane way for the system to 
continue. It would be irresponsible to even try.
Removing them would make pf **more** vulnerable to exploitation, not less.

Regards,
Kristof

signature.asc
Description: OpenPGP digital signature

Is panic() the way to handle errors in pf?

[Kajetan Staszkiewicz]

Hey, group.

A thought came to me: is it really the best thing to panic when errors are 
encountered within pf? I understand there are situations where it is safer for 
the kernel to not continue running like some low-level operations in memory 
allocator or filesystems. But a firewall? Especially that a firewall handles 
packets coming from the Interent which can be arbitrarily crafted.


root:freebsd.git/ (releng/11.1) # git grep panic sys/netpfil/pf/

   
[11:25:04]
sys/netpfil/pf/if_pfsync.c: panic("%s: unable to find deferred state", 
__func__);
sys/netpfil/pf/if_pfsync.c: panic("%s: unexpected sync state %d", 
__func__, st->sync_state);
sys/netpfil/pf/if_pfsync.c: panic("%s: unexpected sync state %d", 
__func__, st->sync_state);
sys/netpfil/pf/if_pfsync.c: panic("%s: unexpected sync state %d", 
__func__, st->sync_state);
sys/netpfil/pf/in4_cksum.c: panic("in4_cksum: offset too 
short");
sys/netpfil/pf/in4_cksum.c: panic("in4_cksum: bad mbuf 
chain");
sys/netpfil/pf/pf.c:panic("%s: unknown address family %u", 
__func__, af);
sys/netpfil/pf/pf.c:panic("%s: unknown address family %u", 
__func__, af);
sys/netpfil/pf/pf.c:panic("%s: dir %u", __func__, dir);
sys/netpfil/pf/pf.c:panic("%s: unknown type", __func__);
sys/netpfil/pf/pf.c:panic("%s: unsupported af %d", __func__, af);
sys/netpfil/pf/pf_lb.c:  * prefixes (or even IPv4) would cause a 
panic.
sys/netpfil/pf/pf_lb.c: panic("%s: unknown action %u", __func__, r-
>action);
sys/netpfil/pf/pf_table.c:  panic("%s: unknown address family %u", 
__func__, af);

That is 14 places in pf code. Wouldn't it be safer to just drop the packet if 
it can not be processed?

-- 
| pozdrawiam / greetings | powered by Debian, FreeBSD and CentOS |
|  Kajetan Staszkiewicz  | jabber,email: vegeta()tuxpowered net  |
|Vegeta  | www: http://vegeta.tuxpowered.net |
`^---'

signature.asc
Description: This is a digitally signed message part.