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);
                                }
+                               pf_set_protostate(*stp, pdst,
+                                   TCPS_ESTABLISHED);
                        } else if (dst->state == TCPS_CLOSING)
                                pf_set_protostate(*stp, pdst,
                                    TCPS_FIN_WAIT_2);

Reply via email to