Date: Thu, 21 Dec 2000 15:36:52 -0800 From: Don Lewis <Don.Lewis@tsc.tdk.com> To: Jesper Skriver <jesper@skriver.dk>, Don Lewis <Don.Lewis@tsc.tdk.com> Cc: Kris Kennaway <kris@FreeBSD.ORG>, Poul-Henning Kamp <phk@critter.freebsd.dk>, security-officer@FreeBSD.ORG, cvs-all@FreeBSD.ORG, freebsd-net@FreeBSD.ORG Subject: Re: what to do now ? Was: cvs commit: src/sys/netinet ip_icmp.c tcp_subr.c tcp_var.h Message-ID: <200012212336.PAA27025@salsa.gv.tsc.tdk.com> In-Reply-To: <20001220155118.N81814@skriver.dk> References: <20001218182600.C1856@skriver.dk> <20001219222730.A29741@skriver.dk> <200012201046.CAA19456@salsa.gv.tsc.tdk.com> <20001220155118.N81814@skriver.dk>
next in thread | previous in thread | raw e-mail | index | archive | help
On Dec 20, 3:51pm, Jesper Skriver wrote:
} Subject: Re: what to do now ? Was: cvs commit: src/sys/netinet ip_icmp.c
} On Wed, Dec 20, 2000 at 02:46:21AM -0800, Don Lewis wrote:
}
} > } It has the following functionality.
} > }
} > } - If the sysctl net.inet.tcp.icmp_admin_prohib_like_rst == 1 (default)
} > } it enables the below.
} > } - When a ICMP administrative prohibited is recieved, it check is the
} > } IP header attached to the ICMP packet has any options set, if it has
} > } it ignores it. The reason for this is, if any options is set the extra
} > } 8 bytes is no longer the first 8 bytes from the TCP header, source/
} > } destination ports and sequence number, which we need to find the
} > } right TCP session.
} >
} > According to Stevens, we should get the first 8 bytes of the TCP header
} > even if there are options on the ICMP packet. We would have to be
} > careful to do sanity checking in this case, as well as guard against
} > unaligned accesses to the TCP header data.
}
} I'll read more on this, for now I think it's a improvement to ignore all
} packets with IP options, then we can improve it later by handling
} packets with options too.
I would expect the option-less case to be the most common, so it's ok to
defer this.
} > } @@ -714,6 +715,15 @@
} > } (lport && inp->inp_lport != lport) ||
} > } (laddr.s_addr && inp->inp_laddr.s_addr != laddr.s_addr) ||
} > } (fport && inp->inp_fport != fport)) {
} > } + inp = inp->inp_list.le_next;
} > } + continue;
} >
} > Wouldn't it be more cleaner (gets rid of the loop) and more efficient (if
} > we're getting blasted with ICMP messages) to use in_pcblookup_hash()?
}
} I didn't change the loop, but I'll have a look at this code, to see if
} we can improve it, but again, to get moving, I'd like to commit this,
} and leave this for a later improvement, ok ?
Sure.
} > } + }
} > } + /*
} > } + * If tcp_sequence is set, then skip sessions where
} > } + * the sequence number is not one of a unacknowledged
} > } + * packet.
} > } + */
} > } + if ((tcp_sequence) && (tcp_seq_vs_sess(inp, tcp_sequence) == 0)) {
} > } inp = inp->inp_list.le_next;
} > } continue;
} >
} > We should pass in an extra flag to indicate if tcp_sequence is valid, since
} > it can legally be zero.
}
} Ack, will do.
}
} > We should also bail out if the sequence check fails,
} > since it isn't possible for there to be another connection with the same
} > src/srcport/dst/dstport, so there is no sense in continuing the search.
}
} That is was we do right ?
}
} First we check if src/dst ip address and port numbers match, if not we
} bail out, so if we reach the above check we know these match, then we
} check for tcp sequence number, if this doesn't match we bail out.
If the src/dst addresses and port numbers don't match, we start the next
iteration of the loop. If the sequence numbers don't match, we want to
exit the loop. I believe the continue should be changed to a break.
I'll pretty much be off the net until the new year, so I won't be able
to perform any further reviews until then.
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200012212336.PAA27025>