> On Tue, Apr 28, 2020 at 12:52 PM Dave Taht <[email protected]> wrote: > > > > On Tue, Apr 28, 2020 at 2:10 AM Toke H?iland-J?rgensen <[email protected]> > > wrote: > > > > > > "Rodney W. Grimes" <[email protected]> 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" <[email protected]> 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 > > > >> > <[email protected]> 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 [email protected]
