Re: PF synproxy should act on inbound packets only
On Fri, Dec 04, 2020 at 01:08:53AM +0100, Alexandr Nedvedicky wrote: > below is updated diff. The new diff also updates pf.conf(5) manpage. OK bluhm@ A note for the man page. > @@ -2126,6 +2126,9 @@ will not work if > .Xr pf 4 > operates on a > .Xr bridge 4 . > +Also > +.Cm synproxy state > +option acts on inbound packets only. The synproxy rules are the subject of the previous sentence. I would not repeate synproxy state in one paragraph. What about Also they act on incoming SYN packets only.
Re: PF synproxy should act on inbound packets only
Hello,
>
> Just a style nit. Other errors do not put stdin:1 in brackes. One
> line per error. In pf.conf the rule direction matters. What about
>
> stdin:1 warning: synproxy used for inbound rules only, ignored for outbound
>
thanks, I like your suggestion.
below is updated diff. The new diff also updates pf.conf(5) manpage.
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index f06171158cb..6c4dde1261f 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -4042,6 +4042,12 @@ rule_consistent(struct pf_rule *r)
"synproxy state or modulate state");
problems++;
}
+
+ if ((r->keep_state == PF_STATE_SYNPROXY) && (r->direction != PF_IN))
+ fprintf(stderr, "%s:%d: warning: "
+ "synproxy used for inbound rules only, "
+ "ignored for outbound\n", file->name, yylval.lineno);
+
if ((r->nat.addr.type != PF_ADDR_NONE ||
r->rdr.addr.type != PF_ADDR_NONE) &&
r->action != PF_MATCH && !r->keep_state) {
diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
index e81198370c9..b77ba5d326c 100644
--- a/share/man/man5/pf.conf.5
+++ b/share/man/man5/pf.conf.5
@@ -2126,6 +2126,9 @@ will not work if
.Xr pf 4
operates on a
.Xr bridge 4 .
+Also
+.Cm synproxy state
+option acts on inbound packets only.
.Pp
Example:
.Bd -literal -offset indent
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 823fdc22133..986ee57bff9 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -4161,7 +4161,7 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r,
struct pf_rule *a,
s->tag = tag;
}
if (pd->proto == IPPROTO_TCP && (th->th_flags & (TH_SYN|TH_ACK)) ==
- TH_SYN && r->keep_state == PF_STATE_SYNPROXY) {
+ TH_SYN && r->keep_state == PF_STATE_SYNPROXY && pd->dir == PF_IN) {
int rtid = pd->rdomain;
if (act->rtableid >= 0)
rtid = act->rtableid;
Re: PF synproxy should act on inbound packets only
On Wed, Dec 02, 2020 at 12:43:28AM +0100, Alexandr Nedvedicky wrote:
> the fix is to apply synproxy action on inbound packets only. Diff below
> does that exactly. Furthermore it also makes pfctl(8) to emit warning,
> when synproxy is being used in outbound/unbound rule:
Sounds reasonable.
> lumpy$ echo 'pass proto tcp from any to any port = 80 synproxy state'
> |./pfctl -nf -
> warning (stdin:1): synproxy acts on inbound packets only
> synproxy action is ignored for outbound packets
Just a style nit. Other errors do not put stdin:1 in brackes. One
line per error. In pf.conf the rule direction matters. What about
stdin:1 warning: synproxy used for inbound rules only, ignored for outbound
> OK?
OK bluhm@
> 8<---8<---8<--8<
> diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
> index f06171158cb..d052b5b9a0e 100644
> --- a/sbin/pfctl/parse.y
> +++ b/sbin/pfctl/parse.y
> @@ -4042,6 +4042,13 @@ rule_consistent(struct pf_rule *r)
> "synproxy state or modulate state");
> problems++;
> }
> +
> + if ((r->keep_state == PF_STATE_SYNPROXY) && (r->direction != PF_IN))
> + fprintf(stderr, "warning (%s:%d): "
> + "synproxy acts on inbound packets only\n"
> + "synproxy action is ignored for outbound packets\n",
> + file->name, yylval.lineno);
> +
> if ((r->nat.addr.type != PF_ADDR_NONE ||
> r->rdr.addr.type != PF_ADDR_NONE) &&
> r->action != PF_MATCH && !r->keep_state) {
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 823fdc22133..986ee57bff9 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -4161,7 +4161,7 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r,
> struct pf_rule *a,
> s->tag = tag;
> }
> if (pd->proto == IPPROTO_TCP && (th->th_flags & (TH_SYN|TH_ACK)) ==
> - TH_SYN && r->keep_state == PF_STATE_SYNPROXY) {
> + TH_SYN && r->keep_state == PF_STATE_SYNPROXY && pd->dir == PF_IN) {
> int rtid = pd->rdomain;
> if (act->rtableid >= 0)
> rtid = act->rtableid;
PF synproxy should act on inbound packets only
Hello,
the issue described here has been hit bu Stuart some time ago. feel free to
stop reading if you don't care/use pf(4) synproxy.
let's assume there are rules which allow just surfing web over http:
block all
pass proto tcp from any to any port = 80 synproxy state
pass proto udp from any to any port = 53
The 'synproxy' rule is not bound to any interface nor any direction. As such
it matches every forwarded SYN packet to port 80 two times. If there would
have been no 'synproxy state' in rule, then pair of states gets created:
A -> B:80 @ IN # inbound direction at RX NIC
A -> B:80 @ OUT # outbound direction at TX NIC
The 'synproxy' action changes things a bit. The 'synproxy' action is
handled here in pf_create_state():
4163 if (pd->proto == IPPROTO_TCP && (th->th_flags & (TH_SYN|TH_ACK)) ==
4164 TH_SYN && r->keep_state == PF_STATE_SYNPROXY) {
4165 int rtid = pd->rdomain;
4166 if (act->rtableid >= 0)
4167 rtid = act->rtableid;
4168 pf_set_protostate(s, PF_PEER_SRC, PF_TCPS_PROXY_SRC);
4169 s->src.seqhi = arc4random();
4170 /* Find mss option */
4171 mss = pf_get_mss(pd);
4172 mss = pf_calc_mss(pd->src, pd->af, rtid, mss);
4173 mss = pf_calc_mss(pd->dst, pd->af, rtid, mss);
4174 s->src.mss = mss;
4175 pf_send_tcp(r, pd->af, pd->dst, pd->src, th->th_dport,
4176 th->th_sport, s->src.seqhi, ntohl(th->th_seq) + 1,
4177 TH_SYN|TH_ACK, 0, s->src.mss, 0, 1, 0, pd->rdomain);
^^
4178 REASON_SET(, PFRES_SYNPROXY);
4179 return (PF_SYNPROXY_DROP);
4180 }
Note pf_send_tcp() at line 4175 is being called with argument `tag` set to 1.
`tag` == 1 orders pf_send_tcp() to send PF_TAG_GENERATED flag at packet.
2770 struct mbuf *
2771 pf_build_tcp(const struct pf_rule *r, sa_family_t af,
2772 const struct pf_addr *saddr, const struct pf_addr *daddr,
2773 u_int16_t sport, u_int16_t dport, u_int32_t seq, u_int32_t ack,
2774 u_int8_t flags, u_int16_t win, u_int16_t mss, u_int8_t ttl, int tag,
2775 u_int16_t rtag, u_int sack, u_int rdom)
2776 {
2805
2806 /* create outgoing mbuf */
2807 m = m_gethdr(M_DONTWAIT, MT_HEADER);
2808 if (m == NULL)
2809 return (NULL);
2810 if (tag)
2811 m->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
2812 m->m_pkthdr.pf.tag = rtag;
the PF_TAG_GENERATED flag brings an unexpected effect to packet sent by
pf_send_tcp() at line 4175. The packet sent by pf_send_tcp() is intercepted
by pf_test() as outbound. However PF_TAG_GENERATED flag turns pf_test()
into No-OP:
6855 int
6856 pf_test(sa_family_t af, int fwdir, struct ifnet *ifp, struct mbuf **m0)
6857 {
#ifdef DIAGNOSTIC
6890 if (((*m0)->m_flags & M_PKTHDR) == 0)
6891 panic("non-M_PKTHDR is passed to pf_test");
6892 #endif /* DIAGNOSTIC */
6893
6894 if ((*m0)->m_pkthdr.pf.flags & PF_TAG_GENERATED)
6895 return (PF_PASS);
6896
6897 if ((*m0)->m_pkthdr.pf.flags & PF_TAG_DIVERTED_PACKET)
6898 return (PF_PASS);
6899
however it is not desired in this case. We actually do want PF to create a
state for such packet, so response B:80 -> A can enter the firewall.
the fix is to apply synproxy action on inbound packets only. Diff below
does that exactly. Furthermore it also makes pfctl(8) to emit warning,
when synproxy is being used in outbound/unbound rule:
lumpy$ echo 'pass proto tcp from any to any port = 80 synproxy state' |./pfctl
-nf -
warning (stdin:1): synproxy acts on inbound packets only
synproxy action is ignored for outbound packets
OK?
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index f06171158cb..d052b5b9a0e 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -4042,6 +4042,13 @@ rule_consistent(struct pf_rule *r)
"synproxy state or modulate state");
problems++;
}
+
+ if ((r->keep_state == PF_STATE_SYNPROXY) && (r->direction != PF_IN))
+ fprintf(stderr, "warning (%s:%d): "
+ "synproxy acts on inbound packets only\n"
+ "synproxy action is ignored for outbound packets\n",
+ file->name, yylval.lineno);
+
if ((r->nat.addr.type != PF_ADDR_NONE ||
r->rdr.addr.type != PF_ADDR_NONE) &&
r->action != PF_MATCH && !r->keep_state) {
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 823fdc22133..986ee57bff9 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -4161,7 +4161,7 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r,
struct
