Skip site navigation (1)Skip section navigation (2)
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 freebsd-net" 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>