Re: [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings

["Rodney W. Grimes" <ietf@gndrsh.dnsmgr.net>]

> On Tue, Apr 28, 2020 at 12:52 PM Dave Taht <dave.taht@gmail.com> wrote:
> >
> > On Tue, Apr 28, 2020 at 2:10 AM Toke H?iland-J?rgensen <toke@redhat.com> wrote:
> > >
> > > "Rodney W. Grimes" <ietf@gndrsh.dnsmgr.net> writes:
> > >
> > > > Replying to a single issue I am reading, and really hope I
> > > > am miss understanding.  I am neither a wireguard or linux
> > > > user so I may be miss understanding what is said.
> > > >
> > > > Inline at {RWG}
> > > >
> > > >> "Jason A. Donenfeld" <Jason@zx2c4.com> writes:
> > > >>
> > > >> > Hey Toke,
> > > >> >
> > > >> > Thanks for fixing this. I wasn't aware there was a newer ECN RFC. A
> > > >> > few comments below:
> > > >> >
> > > >> > On Mon, Apr 27, 2020 at 8:47 AM Toke H?iland-J?rgensen <toke@redhat.com> wrote:
> > > >> >> RFC6040 also recommends dropping packets on certain combinations of
> > > >> >> erroneous code points on the inner and outer packet headers which shouldn't
> > > >> >> appear in normal operation. The helper signals this by a return value > 1,
> > > >> >> so also add a handler for this case.
> > > >> >
> > > >> > This worries me. In the old implementation, we propagate some outer
> > > >> > header data to the inner header, which is technically an authenticity
> > > >> > violation, but minor enough that we let it slide. This patch here
> > > >> > seems to make that violation a bit worse: namely, we're now changing
> > > >> > the behavior based on a combination of outer header + inner header. An
> > > >> > attacker can manipulate the outer header (set it to CE) in order to
> > > >> > learn whether the inner header was CE or not, based on whether or not
> > > >> > the packet gets dropped, which is often observable. That's some form
> > > >
> > > > Why is anyone dropping on decap over the CE bit?  It should be passed
> > > > on, not lead to a packet drop.  If the outer header is CE on an inner
> > > > header of CE it should just continue to be a CE, dropping it is actually
> > > > breaking the purpose of the CE codepoint, to signal congestion before
> > > > having to cause a packet loss.
> > > >
> > > >> > of an oracle, which I'm not too keen on having in wireguard. On the
> > > >> > other hand, we pretty much already _explicitly leak this bit_ on tx
> > > >> > side -- in send.c:
> > > >> >
> > > >> > PACKET_CB(skb)->ds = ip_tunnel_ecn_encap(0, ip_hdr(skb), skb); // inner packet
> > > >> > ...
> > > >> > wg_socket_send_skb_to_peer(peer, skb, PACKET_CB(skb)->ds); // outer packet
> > > >> >
> > > >> > We considered that leak a-okay. But a decryption oracle seems slightly
> > > >> > worse than an explicit and intentional leak. But maybe not that much
> > > >> > worse.
> > > >>
> > > >> Well, seeing as those two bits on the outer header are already copied
> > > >> from the inner header, there's no additional leak added by this change,
> > > >> is there? An in-path observer could set CE and observe that the packet
> > > >> gets dropped, but all they would learn is that the bits were zero
> > > >
> > > > Again why is CE leading to anyone dropping?
> > > >
> > > >> (non-ECT). Which they already knew because they could just read the bits
> > > >> directly from the header.
> > > >>
> > > >> Also note, BTW, that another difference between RFC 3168 and 6040 is the
> > > >> propagation of ECT(1) from outer to inner header. That's not actually
> > > >> done correctly in Linux ATM, but I sent a separate patch to fix this[0],
> > > >> which Wireguard will also benefit from with this patch.
> >
> > I note that there is a large ISP in argentina that has been
> > mis-marking most udp & tcp traffic
> > as CE for years now and despite many attempts to get 'em to fix it,
> > when last I checked (2? 3?)
> > months back, they still were doing it.
> >
> > My impression of overall competence and veracity of multiple transit
> > and isp providers has been sorely
> > tried recently. While I support treating ect 1 and 2 properly, I am
> > inclined to start thinking that
> > ce on a non-ect encapsulated packet is something that should not be dropped.
> >
> > but, whatever is decided on that front is in the hooks in the other
> > patch above, not in wireguard,
> > and I'll make the same comment there.
> 
> Thanks for pointing this out. We're going to drop the dropping
> behavior in wireguard, especially in light of the fact that folks like
> to use wireguard for working around issues with their broken ISP or in
> other weird circumstances.

Thank you!  

-- 
Rod Grimes                                                 rgrimes@freebsd.org