From owner-freebsd-security Tue Jan 25 10:34:37 2000 Delivered-To: freebsd-security@freebsd.org Received: from lariat.lariat.org (lariat.lariat.org [206.100.185.2]) by hub.freebsd.org (Postfix) with ESMTP id 9AA1A1505B for ; Tue, 25 Jan 2000 10:34:26 -0800 (PST) (envelope-from brett@lariat.org) Received: from workhorse (IDENT:ppp0.lariat.org@lariat.lariat.org [206.100.185.2]) by lariat.lariat.org (8.9.3/8.9.3) with ESMTP id LAA09828; Tue, 25 Jan 2000 11:34:16 -0700 (MST) Message-Id: <4.2.2.20000125110039.01a517c0@localhost> X-Sender: brett@localhost X-Mailer: QUALCOMM Windows Eudora Pro Version 4.2.2 Date: Tue, 25 Jan 2000 11:34:14 -0700 To: Warner Losh From: Brett Glass Subject: Re: Merged patches Cc: security@FreeBSD.ORG In-Reply-To: <200001251722.KAA04527@harmony.village.org> References: <4.2.2.20000125095042.01a5aba0@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Sender: owner-freebsd-security@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org At 10:22 AM 1/25/2000 , Warner Losh wrote: >By what code paths is this possible? Please be specific. I'd be glad to submit a patch for this. Here's the rationale: Go to the /sys/netinet directory and execute the command grep IN_MULTICAST * You'll see lots and LOTS of tests of the source addresses of packets to see if they're multicast addresses. But the tests are scattered all over; there's no central location where the test occurs, and it's not used to prevent packets from entering the TCP portion of the stack (at least until things are patched). There's a strong argument for conducting a single test for a multicast source address and setting a M_SRC_MCAST flag. I'd want others' opinion on where the best place is to do this, but it should certainly be done before the IP layer has a chance to do any of the individual tests we see in that grep. >: Also, in at least one place (maybe more), the code does >: multiple tests of the TCP option flags in succession. >: Several tests of this kind should generally be merged >: into a switch for speed (the many conditional jumps >: cause pipeline stalls on many processors, especially >: older ones) and readability. > >It does? If so, it certainly doesn't ADD them. It's a cumulative effect. In the diff, for example, I see: @ -999,7 +995,7 @@ if (thflags & TH_RST) goto drop; if (thflags & TH_ACK) - goto dropwithreset; + goto maybedropwithreset; if ((thflags & TH_SYN) == 0) goto drop; if (th->th_dport == th->th_sport) { which means that the tests are starting to pile up. IIRC there were a few other places where the tests were either stacked or might as well have been (they were interspersed with other code). >: In short, I'd only go with this patch as-is if my >: purpose were to minimize the changes made before >: release. If this were the goal, I'd go back to the >: code immediately thereafter and try to tackle some >: of the inefficiencies and holes in this key input >: path more aggressively. > >Yes. That's exactly the goal. In that case, you're doing the right thing. I'd ultimately like to see (and would be willing to take time to participate in) a code review of the entire file, though. This is SUCH an important routine.... It deserves the best optimization that can be done. >Like I said in my initial mail, I may remove the ICMP_BANDLIM option >as an option, but bump the rate limiter to 1000. But that's about as >far as I'd be willing to go at this time. I'll bet Matt will say that any deviation from protocol should be optional. ;-) I'd sure like to see rate limiting on RSTs separated from rate limiting on ICMP, though. After all, the ICMP packets are a side effect of the DoS we're discussing here; they occur AFTER we generate too many RSTs. The fact that some RSTs were dropped when you turned on ICMP_BANDLIM in the original code seems to me to be somewhat of a lucky accident. --Brett To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-security" in the body of the message