Date: Tue, 25 Jan 2000 11:34:14 -0700 From: Brett Glass <brett@lariat.org> To: Warner Losh <imp@village.org> Cc: security@FreeBSD.ORG Subject: Re: Merged patches Message-ID: <4.2.2.20000125110039.01a517c0@localhost> In-Reply-To: <200001251722.KAA04527@harmony.village.org> References: <Your message of "Tue, 25 Jan 2000 10:09:32 MST." <4.2.2.20000125095042.01a5aba0@localhost> <4.2.2.20000125095042.01a5aba0@localhost>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4.2.2.20000125110039.01a517c0>