Date: Tue, 19 Dec 2000 06:25:46 -0800 From: Don Lewis <Don.Lewis@tsc.tdk.com> To: Jesper Skriver <jesper@skriver.dk>, Kris Kennaway <kris@FreeBSD.ORG>, Poul-Henning Kamp <phk@critter.freebsd.dk> Cc: 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: <200012191425.GAA14731@salsa.gv.tsc.tdk.com> In-Reply-To: <20001218182600.C1856@skriver.dk> References: <20001218182600.C1856@skriver.dk>
next in thread | previous in thread | raw e-mail | index | archive | help
On Dec 18, 6:26pm, Jesper Skriver wrote:
} Subject: what to do now ? Was: cvs commit: src/sys/netinet ip_icmp.c tcp_
} Hi,
}
} I'm trying to find out what to to now regarding this.
}
} To summarize.
}
} PHK committed my original patch, this patch have the following
} functionality
} - When a ICMP administrative prohibited is recieved, it zap's
} all TCP sessions in SYN-SENT state matching the source and destination
} IP addresses and TCP port numbers in the IP header + 8 bytes from the
} ICMP packet.
} - It does not match against TCP sequence number
} - disabled by default
}
} Yesterday I summitted a new diff, with the following changes to the
} above.
}
} - Matches against the TCP sequence number in the IP header + 8 bytes
} from the ICMP packet, against the last unacknowledged packet in the
} TCP session matching the source and destination IP addresses and
} TCP port numbers, these must be equal, thus it only matches if the
} ICMP unreachable is for the last sent packet.
} This is very secure, but in reality only has effect when setting up
} the session, as it doesn't work with multiple outstanding packets,
} it does work when setting up sessions, as the window will be zero
} here.
} this could be fixed by something like (*)
} - Check for SYN-SENT state removed
} - enabled by default
}
} What I will suggest at this point, is to do one of 2 things:
}
} 1) Extend the original diff PHK committed to check for sequence number,
} and enable it by default, trivial as it's part of the second diff.
} 2) Fix the second diff with the below code.
}
} For both I'll also add a extra check if the IP header in the ICMP packet
} has options set, and if it has, don't act on it, this applies to both,
} the reason for this is, if it has options set, we'll miss some (or all)
} of the 8 bytes from the TCP header, and thus, we'll not know port and
} sequence numbers.
}
} What do you prefer ? When I know this, I'll post a new diff for review.
}
} (*) replace
}
} if (tp->snd_una != tcp_sequence) {
}
} with
}
} /*
} * First check: if sequence numbers have wrapped, don't act on this.
} * Second -"- : if the sequence number from the ICMP packet is for a
} * "old" packet, it's probably spoofed, dont't act on this.
} * Third -"- : if the sequence number from the ICMP packet is for a
} * packet from the future, it's spoofed, don't act on this.
} */
} if ((tp->snd_max < tp->snd_una) || (tcp_sequence < tp->snd_una) || \
} (tp->snd_max < tcp_sequence)) {
The sequence number comparisons should use the SEQ_xx() macros, which
handle sequence number wrapping. The sanity checks should probably be
the same as incoming RST validation, see the code in tcp_input() for
the code and matching comments. In the SYN-SENT state, this would
translate to:
if (SEQ_LEQ(tcp_sequence, tp->iss) ||
SEQ_GT(tcp_sequence, tp->snd_max)) {
/* ignore the icmp */
In the other states, RFC 793 says that the RST sanity checking is done by
comparing the sequence number of the of the incoming RST packet against
the transmit window (our outgoing acknowledgement numbers). The host
sending the RST is supposed to copy the acknowledgement number from
an incoming packet to the sequence number of the outgoing RST packet.
This presents a bit of a problem if we try to do the same thing with ICMP,
since it appears that the acknowledgement number is trimmed off the the
data that is returned in the ICMP packet.
It's been too long a day for me to figure out the security implications
of nuking non-SYN-SENT connections based on the sequence number (which
would still be better than nuking these connections without any additional
checking). If we want to do this, the test should probably be:
if (SEQ_LEQ(tcp_sequence, tp->snd_una) ||
SEQ_GT(tcp_sequence, tp->snd_max)) {
though someone needs to check this for fencepost errors.
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?200012191425.GAA14731>