Re: pf max-src-{states,conn} without overload/flush useless?
On Thu, 09 Feb 2023 at 11:51:19 +0100, Alexandr Nedvedicky wrote: > I gave it a try after doing a sysupgrade to: > > penBSD 7.2-current (GENERIC.MP) #1025: Wed Feb 8 19:16:09 MST 2023 > > it still works for me as expected: > disk$ for i in `seq 5` ; do nc 192.168.2.175 22 & done > [1] 51566 > [2] 78983 > [3] 77864 > [4] 37474 > [5] 98599 > disk$ SSH-2.0-OpenSSH_9.2 > SSH-2.0-OpenSSH_9.2 > SSH-2.0-OpenSSH_9.2 > > my connection arrives over iwn0 interface which is in egress group > so our environments are almost identical. Ok now with the latest snapshot kernel I can no longer reproduce this. Maybe there was something unrelated in that snapshot that was causing it. I would still like to have it not fully open the new connection when the max-src-* limit is reached rather than opening and closing, but I guess that is a separate discussion to be had. Thanks for looking into it though.
Re: pf max-src-{states,conn} without overload/flush useless?
Hello,
On Wed, Feb 08, 2023 at 09:42:11PM -0600, joshua stein wrote:
> $ for i in `seq 5` ; do nc 192.168.1.240 22 & done
> [2] 68892
> [3] 6303
> [4] 63554
> [5] 87833
> [6] 49997
> $ SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
>
> vm:~$ doas pfctl -sr
> block return all
> pass out all flags S/SA
> pass in on egress inet6 proto tcp from any to ::1 port = 22 flags S/SA keep
> state (source-track rule, max-src-conn 3)
> pass in on egress inet proto tcp from any to 127.0.0.1 port = 22 flags S/SA
> keep state (source-track rule, max-src-conn 3)
> pass in on egress inet proto tcp from any to 192.168.1.240 port = 22 flags
> S/SA keep state (source-track rule, max-src-conn 3)
>
> This is with:
>
> OpenBSD 7.2-current (GENERIC.MP) #2014: Tue Feb 7 16:24:04 MST 2023
> dera...@arm64.openbsd.org:/usr/src/sys/arch/arm64/compile/GENERIC.MP
I gave it a try after doing a sysupgrade to:
penBSD 7.2-current (GENERIC.MP) #1025: Wed Feb 8 19:16:09 MST 2023
it still works for me as expected:
disk$ for i in `seq 5` ; do nc 192.168.2.175 22 & done
[1] 51566
[2] 78983
[3] 77864
[4] 37474
[5] 98599
disk$ SSH-2.0-OpenSSH_9.2
SSH-2.0-OpenSSH_9.2
SSH-2.0-OpenSSH_9.2
my connection arrives over iwn0 interface which is in egress group
so our environments are almost identical.
>
> > > diff --git sys/net/pf.c sys/net/pf.c
> > > index 8cb1326a160..89703feab12 100644
> > > --- sys/net/pf.c
> > > +++ sys/net/pf.c
> > > @@ -481,12 +481,10 @@ pf_src_connlimit(struct pf_state **stp)
> > > if ((sn = pf_get_src_node((*stp), PF_SN_NONE)) == NULL)
> > > return (0);
> > >
> > > - sn->conn++;
> > > - (*stp)->src.tcp_est = 1;
> > > pf_add_threshold(&sn->conn_rate);
> > >
> > > if ((*stp)->rule.ptr->max_src_conn &&
> > > - (*stp)->rule.ptr->max_src_conn < sn->conn) {
> > > + sn->conn >= (*stp)->rule.ptr->max_src_conn) {
> > > pf_status.lcounters[LCNT_SRCCONN]++;
> > > bad++;
> > > }
> > > @@ -497,8 +495,11 @@ pf_src_connlimit(struct pf_state **stp)
> > > bad++;
> > > }
> > >
> > > - if (!bad)
> > > + if (!bad) {
> > > + sn->conn++;
> > > + (*stp)->src.tcp_est = 1;
> > > return (0);
> > > + }
> > >
> > > if ((*stp)->rule.ptr->overload_tbl) {
> > > struct pfr_addr p;
> >
> > it seems to me the change to pf_src_connlimit() does
> > not alter behavior. I think change to pf_src_connlimit()
> > can be dropped.
>
> But don't we not want to increment the source node's connection
> count since we're not going to accept the connection (in the !bad
> case)? I'm not sure what kind of bookkeeping that would screw up.
>
what we currently do is we always bump connection count
for source node we found. then we are going to check limit
(*stp)->rule.ptr->max_src_conn < sn->conn
if the limit is exceeded we mark as closed and expired (timeout
PFTM_PURGE). We also report that to caller which should close the
connection.
your change stops counting connections as soon as limit is
reached. So now I see there is a change in behavior. I've missed
that yesterday. I'm not able to tell if we want go that way or not.
> > currently we do conn limit check in step (4). Your change moves this
> > to earlier step (3) (given I understand things right here).
> > It's awfully early here I need sleep on this.
>
> Yes, that was my understanding too. We wait until the remote has
> done enough work to be a valid connection but then block it before
> sending the final ack.
>
> > can you give it a try with your slightly modified diff? just drop
> > changes to pf_src_connlimit() and keep those in pf_tcp_track_full() which
> > I believe is the only relevant part.
>
> Yes, it still works, and only allows me 3 connections with the final
> 2 timing out as expected:
>
> $ for i in `seq 5` ; do nc 192.168.1.240 22 & done
> [2] 10193
> [3] 30197
> [4] 72235
> [5] 69900
> [6] 99044
> $ SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> [5] - exit 1 nc 192.168.1.240 22
> $
> [6] + exit 1 nc 192.168.1.240 22
>
the only explanation why it does not work for you is latency. The packets which
match state run as a readers. so if we call pf_set_protostate() before we
actually check the limit then it might be the cause here. I admit it's
rather a speculation.
can you give a try to diff below?
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 8cb1326a160..f81b0c793ce 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -4919,14 +4919,14 @@ pf_tcp_track_full(struct pf_pdesc *pd, struct pf_state
**stp, u_short *reason,
pf_set_protostate(*stp, psrc, TCPS_CLOSING);
if (th->th_flags & TH_ACK) {
if (dst->state == TCPS_
Re: pf max-src-{states,conn} without overload/flush useless?
On Thu, 09 Feb 2023 at 02:42:22 +0100, Alexandr Nedvedicky wrote:
> this is my test terminal on remote host:
> router$ for i in `seq 5` ; do nc 192.168.2.175 22 & done
> [1] 32472
> [2] 97453
> [3] 7192
> [4] 50386
> [5] 57517
> router$ SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
>
> there are three SSH version strings which indicates
> I got 3 working connection of 5 attempts.
Interesting, I tried with your SSH example and it allowed me to
connect 5 times.
$ for i in `seq 5` ; do nc 192.168.1.240 22 & done
[2] 68892
[3] 6303
[4] 63554
[5] 87833
[6] 49997
$ SSH-2.0-OpenSSH_9.1
SSH-2.0-OpenSSH_9.1
SSH-2.0-OpenSSH_9.1
SSH-2.0-OpenSSH_9.1
SSH-2.0-OpenSSH_9.1
vm:~$ doas pfctl -sr
block return all
pass out all flags S/SA
pass in on egress inet6 proto tcp from any to ::1 port = 22 flags S/SA keep
state (source-track rule, max-src-conn 3)
pass in on egress inet proto tcp from any to 127.0.0.1 port = 22 flags S/SA
keep state (source-track rule, max-src-conn 3)
pass in on egress inet proto tcp from any to 192.168.1.240 port = 22 flags S/SA
keep state (source-track rule, max-src-conn 3)
This is with:
OpenBSD 7.2-current (GENERIC.MP) #2014: Tue Feb 7 16:24:04 MST 2023
dera...@arm64.openbsd.org:/usr/src/sys/arch/arm64/compile/GENERIC.MP
> > diff --git sys/net/pf.c sys/net/pf.c
> > index 8cb1326a160..89703feab12 100644
> > --- sys/net/pf.c
> > +++ sys/net/pf.c
> > @@ -481,12 +481,10 @@ pf_src_connlimit(struct pf_state **stp)
> > if ((sn = pf_get_src_node((*stp), PF_SN_NONE)) == NULL)
> > return (0);
> >
> > - sn->conn++;
> > - (*stp)->src.tcp_est = 1;
> > pf_add_threshold(&sn->conn_rate);
> >
> > if ((*stp)->rule.ptr->max_src_conn &&
> > - (*stp)->rule.ptr->max_src_conn < sn->conn) {
> > + sn->conn >= (*stp)->rule.ptr->max_src_conn) {
> > pf_status.lcounters[LCNT_SRCCONN]++;
> > bad++;
> > }
> > @@ -497,8 +495,11 @@ pf_src_connlimit(struct pf_state **stp)
> > bad++;
> > }
> >
> > - if (!bad)
> > + if (!bad) {
> > + sn->conn++;
> > + (*stp)->src.tcp_est = 1;
> > return (0);
> > + }
> >
> > if ((*stp)->rule.ptr->overload_tbl) {
> > struct pfr_addr p;
>
> it seems to me the change to pf_src_connlimit() does
> not alter behavior. I think change to pf_src_connlimit()
> can be dropped.
But don't we not want to increment the source node's connection
count since we're not going to accept the connection (in the !bad
case)? I'm not sure what kind of bookkeeping that would screw up.
> > @@ -4919,14 +4920,14 @@ pf_tcp_track_full(struct pf_pdesc *pd, struct
> > pf_state **stp, u_short *reason,
> > pf_set_protostate(*stp, psrc, TCPS_CLOSING);
> > if (th->th_flags & TH_ACK) {
> > if (dst->state == TCPS_SYN_SENT) {
> > - pf_set_protostate(*stp, pdst,
> > - TCPS_ESTABLISHED);
> > - if (src->state == TCPS_ESTABLISHED &&
> > + if (src->state == TCPS_SYN_SENT &&
> > !SLIST_EMPTY(&(*stp)->src_nodes) &&
> > pf_src_connlimit(stp)) {
> > REASON_SET(reason, PFRES_SRCLIMIT);
> > return (PF_DROP);
> > }
> > + pf_set_protostate(*stp, pdst,
> > + TCPS_ESTABLISHED);
> > } else if (dst->state == TCPS_CLOSING)
> > pf_set_protostate(*stp, pdst,
> > TCPS_FIN_WAIT_2);
> >
>
> If I understand things right then in current pf we check conn limit
> when we see acknowledgment of 3rd client's ACK sent by server. Not sure
> if I sound clear here so let me draw little diagram:
>
> SYN > sent by client moves state to
>
> TCPS_SYN_SENT : TCPS_LISTEN/TCPS_CLOSED (1)
>
> SYN | ACK < sent by server moves state to
>
> TCPS_SYN_SENT : TCPS_SYN_SENT (2)
>
> ACK > 3rd ack sent by client move state to:
>
> TCPS_ESTABLISHED : TCPS_SYN_SENT (3)
>
> ACK < server acknowledges client's 3rd ack moves state to
>
> TCPS_ESTABLISHED : TCPS_ESTABLISHED (4)
>
> currently we do conn limit check in step (4). Your change moves this
> to earlier step (3) (given I understand things right here).
> It's awfully early here I need sleep on this.
Yes, that was my understanding too. We wait until the remote has
done enough work to be a valid connection but then block it before
sending the final ack.
> can you give it a try with your slightly modified diff? just drop
> changes to pf_src_connlimit() and keep those in pf_tcp_track_full() which
> I believe is the only
Re: pf max-src-{states,conn} without overload/flush useless?
Hello,
I did test similar rules on my system
OpenBSD 7.2-current (GENERIC.MP) #978: Sun Jan 22 11:41:04 MST 2023
these are my rules:
set skip on lo
block return# block stateless traffic
pass out log# establish keep-state
pass in on iwn0 proto tcp from 192.168.2.0/24 to self port 22 \
keep state (source-track rule, max-src-conn 3)
this is my test terminal on remote host:
router$ for i in `seq 5` ; do nc 192.168.2.175 22 & done
[1] 32472
[2] 97453
[3] 7192
[4] 50386
[5] 57517
router$ SSH-2.0-OpenSSH_9.1
SSH-2.0-OpenSSH_9.1
SSH-2.0-OpenSSH_9.1
there are three SSH version strings which indicates
I got 3 working connection of 5 attempts.
I'm not sure why it seems to work for me (ssh) while
it is broken for you (http). I'll give it a try with
webserver.
On Wed, Feb 08, 2023 at 04:34:55PM -0600, joshua stein wrote:
> diff --git sys/net/pf.c sys/net/pf.c
> index 8cb1326a160..89703feab12 100644
> --- sys/net/pf.c
> +++ sys/net/pf.c
> @@ -481,12 +481,10 @@ pf_src_connlimit(struct pf_state **stp)
> if ((sn = pf_get_src_node((*stp), PF_SN_NONE)) == NULL)
> return (0);
>
> - sn->conn++;
> - (*stp)->src.tcp_est = 1;
> pf_add_threshold(&sn->conn_rate);
>
> if ((*stp)->rule.ptr->max_src_conn &&
> - (*stp)->rule.ptr->max_src_conn < sn->conn) {
> + sn->conn >= (*stp)->rule.ptr->max_src_conn) {
> pf_status.lcounters[LCNT_SRCCONN]++;
> bad++;
> }
> @@ -497,8 +495,11 @@ pf_src_connlimit(struct pf_state **stp)
> bad++;
> }
>
> - if (!bad)
> + if (!bad) {
> + sn->conn++;
> + (*stp)->src.tcp_est = 1;
> return (0);
> + }
>
> if ((*stp)->rule.ptr->overload_tbl) {
> struct pfr_addr p;
it seems to me the change to pf_src_connlimit() does
not alter behavior. I think change to pf_src_connlimit()
can be dropped.
> @@ -4919,14 +4920,14 @@ pf_tcp_track_full(struct pf_pdesc *pd, struct
> pf_state **stp, u_short *reason,
> pf_set_protostate(*stp, psrc, TCPS_CLOSING);
> if (th->th_flags & TH_ACK) {
> if (dst->state == TCPS_SYN_SENT) {
> - pf_set_protostate(*stp, pdst,
> - TCPS_ESTABLISHED);
> - if (src->state == TCPS_ESTABLISHED &&
> + if (src->state == TCPS_SYN_SENT &&
> !SLIST_EMPTY(&(*stp)->src_nodes) &&
> pf_src_connlimit(stp)) {
> REASON_SET(reason, PFRES_SRCLIMIT);
> return (PF_DROP);
> }
> + pf_set_protostate(*stp, pdst,
> + TCPS_ESTABLISHED);
> } else if (dst->state == TCPS_CLOSING)
> pf_set_protostate(*stp, pdst,
> TCPS_FIN_WAIT_2);
>
If I understand things right then in current pf we check conn limit
when we see acknowledgment of 3rd client's ACK sent by server. Not sure
if I sound clear here so let me draw little diagram:
SYN > sent by client moves state to
TCPS_SYN_SENT : TCPS_LISTEN/TCPS_CLOSED (1)
SYN | ACK < sent by server moves state to
TCPS_SYN_SENT : TCPS_SYN_SENT (2)
ACK > 3rd ack sent by client move state to:
TCPS_ESTABLISHED : TCPS_SYN_SENT (3)
ACK < server acknowledges client's 3rd ack moves state to
TCPS_ESTABLISHED : TCPS_ESTABLISHED (4)
currently we do conn limit check in step (4). Your change moves this
to earlier step (3) (given I understand things right here).
It's awfully early here I need sleep on this.
can you give it a try with your slightly modified diff? just drop
changes to pf_src_connlimit() and keep those in pf_tcp_track_full() which
I believe is the only relevant part.
thanks and
regards
sashan
pf max-src-{states,conn} without overload/flush useless?
I want to limit incoming connections on a server to 5 per IP. I
don't want to put violators into a pf table (overload) or kill the
other connections (flush), I just want to not accept new connections
from that IP once their limit is reached and then accept them again
when they are under the limit. Is this broken or am I holding it
wrong?
With a simple pf.conf on the server specifying max-src-conn of 5:
set skip on lo
block return
pass out
pass in on egress proto tcp to port 80 keep state \
(source-track rule, max-src-conn 5)
Run a dumb webserver that prints the pf states on each new
connection, and sleeps for a while before responding to keep the
connection open:
$ doas ruby
require "webrick"
server = WEBrick::HTTPServer.new(:Port => 80)
server.mount_proc "/" do |req, res|
puts "states for #{req.remote_ip}:"
system "pfctl -ss | grep ' #{req.remote_ip}.*ESTABLISHED'"
sleep 30
end
trap "INT" do
server.shutdown
end
server.start
^D
Now from another machine (192.168.1.196 in this case) send 20
requests at once to the server (192.168.1.240):
$ for f in `jot 20`; do ftp -o - http://192.168.1.240/ &; sleep 0.5; done
And on the server, you can see that there are now many more than 5
established states:
states for 192.168.1.196:
all tcp 192.168.1.240:80 <- 192.168.1.196:16727 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 <- 192.168.1.196:24725 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 <- 192.168.1.196:2436 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 <- 192.168.1.196:16529 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 <- 192.168.1.196:4323 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:45576 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:36693 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:2976 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:9395 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:46616 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:46122 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:22969 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:48742 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:43477 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:29154 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:1876 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:47743 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:43833 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:12074 ESTABLISHED:ESTABLISHED
all tcp 192.168.1.240:80 -> 192.168.1.196:17816 ESTABLISHED:SYN_SENT
Looking at pf.c, the logic in pf_tcp_track_full seems to indicate
that it's accepting the connection (moving it to TCPS_ESTABLISHED),
then checking pf_src_connlimit, but the PF_DROP gets ignored because
the connection is already established.
Shouldn't it not accept the connection if pf_src_connlimit returns
1? This does that:
diff --git sys/net/pf.c sys/net/pf.c
index 8cb1326a160..89703feab12 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -481,12 +481,10 @@ pf_src_connlimit(struct pf_state **stp)
if ((sn = pf_get_src_node((*stp), PF_SN_NONE)) == NULL)
return (0);
- sn->conn++;
- (*stp)->src.tcp_est = 1;
pf_add_threshold(&sn->conn_rate);
if ((*stp)->rule.ptr->max_src_conn &&
- (*stp)->rule.ptr->max_src_conn < sn->conn) {
+ sn->conn >= (*stp)->rule.ptr->max_src_conn) {
pf_status.lcounters[LCNT_SRCCONN]++;
bad++;
}
@@ -497,8 +495,11 @@ pf_src_connlimit(struct pf_state **stp)
bad++;
}
- if (!bad)
+ if (!bad) {
+ sn->conn++;
+ (*stp)->src.tcp_est = 1;
return (0);
+ }
if ((*stp)->rule.ptr->overload_tbl) {
struct pfr_addr p;
@@ -4919,14 +4920,14 @@ pf_tcp_track_full(struct pf_pdesc *pd, struct pf_state
**stp, u_short *reason,
pf_set_protostate(*stp, psrc, TCPS_CLOSING);
if (th->th_flags & TH_ACK) {
if (dst->state == TCPS_SYN_SENT) {
- pf_set_protostate(*stp, pdst,
- TCPS_ESTABLISHED);
- if (src->state == TCPS_ESTABLISHED &&
+ if (src->state == TCPS_SYN_SENT &&
!SLIST_EMPTY(&(*stp)->src_nodes) &&
pf_src_connlimit(stp)) {
REASON_SET(reason, PFRES_SRCLIMIT);
return (PF_DROP);
