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 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
