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

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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.