pf(4) ignores 'keep state' and 'nat-to' actions for unsolicited

icmp error responses. Fix tightens rule matching logic so icmp
error responses no longer match 'keep state' rule. In typical
scenarios icmp errors (if solicited) should match existing state.
The change is going to bite firewalls which deal with asymmetric
routes. In those cases the 'keep state' action should be relaxed
to sloppy or new 'no state' rule to explicitly match icmp
errors should be added.

The issue has been reported by Peter J. Philip (pjp _at_ delphinusdns.org).

Discussed with bluhm@ and florian@

OK bluhm@

diff --git a/sys/net/pf.c b/sys/net/pf.c
index 4f0fc3f..bf6b6d0 100644 (file)
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: pf.c,v 1.1184 2023/07/31 11:13:09 dlg Exp $ */
+/*     $OpenBSD: pf.c,v 1.1185 2023/09/07 09:59:43 sashan Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -4148,6 +4148,10 @@ enter_ruleset:
                            (r->rule_flag & PFRULE_STATESLOPPY) == 0 &&
                            ctx->icmp_dir != PF_IN),
                                TAILQ_NEXT(r, entries));
+                       /* icmp packet must match existing state */
+                       PF_TEST_ATTRIB(r->keep_state && ctx->state_icmp &&
+                           (r->rule_flag & PFRULE_STATESLOPPY) == 0,
+                               TAILQ_NEXT(r, entries));
                        break;
 
                case IPPROTO_ICMPV6:
@@ -4165,6 +4169,10 @@ enter_ruleset:
                            ctx->icmp_dir != PF_IN &&
                            ctx->icmptype != ND_NEIGHBOR_ADVERT),
                                TAILQ_NEXT(r, entries));
+                       /* icmp packet must match existing state */
+                       PF_TEST_ATTRIB(r->keep_state && ctx->state_icmp &&
+                           (r->rule_flag & PFRULE_STATESLOPPY) == 0,
+                               TAILQ_NEXT(r, entries));
                        break;
 
                default: