Hello,
I'm sending same patch I've sent to bugs@ to thread [1], which got started by
Paul de Weerd few days ago. I've received few questions off-list, which made me
to shovel bit more through TCP code in OpenBSD. I hope I can better explain
impact of change introduced by patch below. I think it's worth if I'll post
answers to those off-list questions to tech@, as it is seems better place to
discuss the matter.
> Are you not undoing all the hard work by cloder and fgsch and previously
> markus -- to make icmp errors become "soft errors" rather than "hard errors",
> otherwise I can spoof icmp's and tear down your connections?
the tcp_notify() function gets called from tcp*_ctlinput() [2]. In our
particular case of ICMP error message we arrive to tcp*_ctlinput() from
icmp_input_if() [3], which handles inbound ICMP packet.
the tcp*_ctlinput() unwraps payload of ICMP error message. The payload
is header portion of packet, which triggered the ICMP error. *UNREACHABLE
ICMP errors are handled at the bottom of tcp*_ctlinput() [4]:
792
793 if (ip) {
794 th = (struct tcphdr *)((caddr_t)ip + (ip->ip_hl << 2));
795 inp = in_pcbhashlookup(&tcbtable,
796 ip->ip_dst, th->th_dport, ip->ip_src, th->th_sport,
797 rdomain);
798 if (inp) {
799 seq = ntohl(th->th_seq);
800 if (inp->inp_socket &&
801 (tp = intotcpcb(inp)) &&
802 SEQ_GEQ(seq, tp->snd_una) &&
803 SEQ_LT(seq, tp->snd_max))
804 notify(inp, errno);
805 } else if (inetctlerrmap[cmd] == EHOSTUNREACH ||
806 inetctlerrmap[cmd] == ENETUNREACH ||
807 inetctlerrmap[cmd] == EHOSTDOWN) {
808 struct sockaddr_in sin;
809
810 bzero(&sin, sizeof(sin));
811 sin.sin_len = sizeof(sin);
812 sin.sin_family = AF_INET;
813 sin.sin_port = th->th_sport;
814 sin.sin_addr = ip->ip_src;
815 syn_cache_unreach(sintosa(&sin), sa, th, rdomain);
816 }
817 } else
818 in_pcbnotifyall(&tcbtable, sa, rdomain, errno, notify);
if we could find `inp` at line 795, then ICMP message comes as a response
to packet sent by bound network client socket. If there is a matching socket
waiting for response we call tcp_notify() at 804. `notify()` is a pointer,
which
points to tcp_notify()
The else branch at 805 deals with case, where we deal with ICMP error,
which comes as a response to packet sent by listen socket. This typically
happens when we receive error to our SYN-ACK.
> Could it be that this was something we changed when the various TCP RST
> attacks were a big deal? That was in 2004/2005 IIRC. We for sure made sure
I think it's just omission in old code. The tcp_notify() has not
change sine 1995.
> that the TCP RST needs to be at the correct place. Not sure how well the
> ICMP is checked to ensure that nobody can use them for attacks (we did for
> sure made the checks more strict).
yes those checks happen in tcp_ctlinput() before tcp_notify() gets
called at lines 802, 803
>
> Your diff seems to only pass the error up if the connection is not
> established which is probably safe.
I think we should also consider to report ECONNREFUSED immediately for
sockets, which are in established state. Currently this case is handled
as 'softerror' case. The softerror is still kind of mystery. It seems
to me we just poke/wake blocked caller, which won't spot any error and
will most likely call read()/write() again. I hope tech@ will be able
to shed some light here...
> i found the same code when i read Pauls mail. I also found
> syn_cache_unreach(), which gets called with
>
> } else if (inetctlerrmap[cmd] == EHOSTUNREACH ||
> inetctlerrmap[cmd] == ENETUNREACH ||
> inetctlerrmap[cmd] == EHOSTDOWN) {
> ...
> syn_cache_unreach(sintosa(&sin), sa, th, rdomain);
>
> and contains this:
>
> /*
> * If we've retransmitted 3 times and this is our second error,
> * we remove the entry. Otherwise, we allow it to continue on.
> * This prevents us from incorrectly nuking an entry during a
> * spurious network outage.
> *
> * See tcp_notify().
> */
> if ((sc->sc_flags & SCF_UNREACH) == 0 || sc->sc_rxtshift < 3) {
> sc->sc_flags |= SCF_UNREACH;
> return;
> }
My understanding is this was just copied from tcp_notify(), which is older
code than syn cache. Perhaps the reason was to keep behavior consistent
for syn cache as well.
Thinking more about it: I would just drop the syncache entry with the first
matching ICMP unreachable message we get. Why to do so much effort for
someone, who presented us with spoofed SYN.
thanks and
regards
sashan
[1] https://marc.info/?l=openbsd-bugs&m=157218822427561&w=2
[2]
https://github.com/openbsd/src/blob/d069abbed478f14455797f3d6eedddace13a5d82/sys/netinet/tcp_subr.c#L697
[3]
https://github.com/openbsd/src/blob/d069abbed478f14455797f3d6eedddace13a5d82/sys/netinet/ip_icmp.c#L495
[4]
https://github.com/openbsd/src/blob/d069abbed478f14455797f3d6eedddace13a5d82/sys/netinet/tcp_subr.c#L789
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/netinet/tcp_subr.c b/sys/netinet/tcp_subr.c
index f1aa5b058f1..02dc41b4d45 100644
--- a/sys/netinet/tcp_subr.c
+++ b/sys/netinet/tcp_subr.c
@@ -574,9 +574,13 @@ tcp_notify(struct inpcb *inp, int error)
*/
if (tp->t_state == TCPS_ESTABLISHED &&
(error == EHOSTUNREACH || error == ENETUNREACH ||
- error == EHOSTDOWN)) {
+ error == EHOSTDOWN))
return;
- } else if (TCPS_HAVEESTABLISHED(tp->t_state) == 0 &&
+ else if (TCPS_HAVEESTABLISHED(tp->t_state) == 0 &&
+ (error == EHOSTUNREACH || error == ENETUNREACH ||
+ error == EHOSTDOWN || error == ECONNREFUSED))
+ so->so_error = error;
+ else if (TCPS_HAVEESTABLISHED(tp->t_state) == 0 &&
tp->t_rxtshift > 3 && tp->t_softerror)
so->so_error = error;
else
