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

Attachment: signature.asc
Description: This is a digitally signed message part.

Reply via email to