On Saturday 21 July 2007, Jordan Gordeev wrote: > J.D. Bronson wrote: > > At 02:52 AM 02/26/2007, you wrote: > >> Wow, this fixed my FTP-over-DSL-to-6.2 problem too. With modulate > >> state, I was getting ~30K/sec. With just keep state, I'm now getting > >> more like what my connection is capable of. This is between two 6.2 > >> hosts on opposite sides of the Atlantic. > >> > >> Ted, I use pf because I like the format of the configuration file, I > >> like the logging and pftop, and like how it's harder to lock > >> yourself out of a remote machine by accident :) > >> > >> /JMS > > > > I use pf since its newer (I think?) and I came from openbsd..pf just > > works and the config file is nice and sweet. > > > > I had thought that modulate state would put a load on my proc, but > > sheesh, its a p4-3.06 - thats more than robust for a router. > > > > I wonder if we should file a bug on this? > > > > I am glad my post helped here. I still use modulate state for any > > INCOMING connections though (www/smtp/etc). > > I'm replying to an old and long-forgotten thread to report my recent > findings. > There's a bug in PF with modulate/synproxy state. Modulate/synproxy > state modulate sequence numbers, but don't modulate sequence numbers in > TCP SACK options. Some firewalls block TCP segments with sequence > numbers in the SACK option pointing outside the window, which causes > connection stalls. The bug was fixed in OpenBSD with revision 1.509 of > src/sys/net/pf.c about an year and a half ago. The bug is present in > FreeBSD-STABLE. A fix for the bug was imported in FreeBSD-CURRENT with > the big import of PF from OpenBSD 4.1. > I'm CC-ing Max to notify him of the bug present in -STABLE and to ask > him to deal with the issue by either porting the fix from OpenBSD, or > by documenting that modulate/synproxy state is broken.
Good catch - sorry for the delay. Here is the diff (almost verbatim from OPENBSD_3_8). Please test and report back. I plan to commit this to RELENG_6 in a bit. -- /"\ Best regards, | [EMAIL PROTECTED] \ / Max Laier | ICQ #67774661 X http://pf4freebsd.love2party.net/ | [EMAIL PROTECTED] / \ ASCII Ribbon Campaign | Against HTML Mail and News
Index: pf.c
===================================================================
RCS file: /usr/store/mlaier/fcvs/src/sys/contrib/pf/net/pf.c,v
retrieving revision 1.34.2.4
diff -u -r1.34.2.4 pf.c
--- pf.c 19 Sep 2006 15:45:20 -0000 1.34.2.4
+++ pf.c 25 Jul 2007 18:51:35 -0000
@@ -1,5 +1,5 @@
/* $FreeBSD: src/sys/contrib/pf/net/pf.c,v 1.34.2.4 2006/09/19 15:45:20 csjp Exp $ */
-/* $OpenBSD: pf.c,v 1.483 2005/03/15 17:38:43 dhartmei Exp $ */
+/* $OpenBSD: pf.c,v 1.502.2.1 2006/05/02 22:55:52 brad Exp $ */
/*
* Copyright (c) 2001 Daniel Hartmeier
@@ -170,6 +170,8 @@
void pf_change_ap(struct pf_addr *, u_int16_t *,
u_int16_t *, u_int16_t *, struct pf_addr *,
u_int16_t, u_int8_t, sa_family_t);
+int pf_modulate_sack(struct mbuf *, int, struct pf_pdesc *,
+ struct tcphdr *, struct pf_state_peer *);
#ifdef INET6
void pf_change_a6(struct pf_addr *, u_int16_t *,
struct pf_addr *, u_int8_t);
@@ -1483,6 +1485,63 @@
}
#endif /* INET6 */
+
+/*
+ * Need to modulate the sequence numbers in the TCP SACK option
+ */
+int
+pf_modulate_sack(struct mbuf *m, int off, struct pf_pdesc *pd,
+ struct tcphdr *th, struct pf_state_peer *dst)
+{
+ int hlen = (th->th_off << 2) - sizeof(*th), thoptlen = hlen;
+ u_int8_t opts[MAX_TCPOPTLEN], *opt = opts;
+ int copyback = 0, i, olen;
+ struct sackblk sack;
+
+#define TCPOLEN_SACKLEN (TCPOLEN_SACK + 2)
+ if (hlen < TCPOLEN_SACKLEN ||
+ !pf_pull_hdr(m, off + sizeof(*th), opts, hlen, NULL, NULL, pd->af))
+ return 0;
+
+ while (hlen >= TCPOLEN_SACKLEN) {
+ olen = opt[1];
+ switch (*opt) {
+ case TCPOPT_EOL: /* FALLTHROUGH */
+ case TCPOPT_NOP:
+ opt++;
+ hlen--;
+ break;
+ case TCPOPT_SACK:
+ if (olen > hlen)
+ olen = hlen;
+ if (olen >= TCPOLEN_SACKLEN) {
+ for (i = 2; i + TCPOLEN_SACK <= olen;
+ i += TCPOLEN_SACK) {
+ memcpy(&sack, &opt[i], sizeof(sack));
+ pf_change_a(&sack.start, &th->th_sum,
+ htonl(ntohl(sack.start) -
+ dst->seqdiff), 0);
+ pf_change_a(&sack.end, &th->th_sum,
+ htonl(ntohl(sack.end) -
+ dst->seqdiff), 0);
+ memcpy(&opt[i], &sack, sizeof(sack));
+ }
+ copyback = 1;
+ }
+ /* FALLTHROUGH */
+ default:
+ if (olen < 2)
+ olen = 2;
+ hlen -= olen;
+ opt += olen;
+ }
+ }
+
+ if (copyback)
+ m_copyback(m, off + sizeof(*th), thoptlen, opts);
+ return (copyback);
+}
+
void
pf_change_icmp(struct pf_addr *ia, u_int16_t *ip, struct pf_addr *oa,
struct pf_addr *na, u_int16_t np, u_int16_t *pc, u_int16_t *h2c,
@@ -4577,6 +4636,25 @@
ackskew = dst->seqlo - ack;
+
+ /*
+ * Need to demodulate the sequence numbers in any TCP SACK options
+ * (Selective ACK). We could optionally validate the SACK values
+ * against the current ACK window, either forwards or backwards, but
+ * I'm not confident that SACK has been implemented properly
+ * everywhere. It wouldn't surprise me if several stacks accidently
+ * SACK too far backwards of previously ACKed data. There really aren't
+ * any security implications of bad SACKing unless the target stack
+ * doesn't validate the option length correctly. Someone trying to
+ * spoof into a TCP connection won't bother blindly sending SACK
+ * options anyway.
+ */
+ if (dst->seqdiff && (th->th_off << 2) > sizeof(struct tcphdr)) {
+ if (pf_modulate_sack(m, off, pd, th, dst))
+ copyback = 1;
+ }
+
+
#define MAXACKWINDOW (0xffff + 1500) /* 1500 is an arbitrary fudge factor */
if (SEQ_GEQ(src->seqhi, end) &&
/* Last octet inside other's window space */
signature.asc
Description: This is a digitally signed message part.
