Development discussion of WireGuard
 help / color / Atom feed
* [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings
@ 2020-04-27 14:46 
  2020-04-27 19:53 ` Jason
  0 siblings, 1 reply; 11+ messages in thread
From:  @ 2020-04-27 14:46 UTC (permalink / raw)


WireGuard currently only propagates ECN markings on tunnel decap according
to the old RFC3168 specification. However, the spec has since been updated
in RFC6040 to recommend slightly different decapsulation semantics. This
was implemented in the kernel as a set of common helpers for ECN
decapsulation, so let's just switch over WireGuard to using those, so it
can benefit from this enhancement and any future tweaks.

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.

Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
Reported-by: Olivier Tilmans <olivier.tilmans at nokia-bell-labs.com>
Cc: Dave Taht <dave.taht at gmail.com>
Cc: Rodney W. Grimes <ietf at gndrsh.dnsmgr.net>
Signed-off-by: Toke H?iland-J?rgensen <toke at redhat.com>
---
 drivers/net/wireguard/receive.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireguard/receive.c b/drivers/net/wireguard/receive.c
index da3b782ab7d3..f33e476ad574 100644
--- a/drivers/net/wireguard/receive.c
+++ b/drivers/net/wireguard/receive.c
@@ -393,13 +393,15 @@ static void wg_packet_consume_data_done(struct wg_peer *peer,
 		len = ntohs(ip_hdr(skb)->tot_len);
 		if (unlikely(len < sizeof(struct iphdr)))
 			goto dishonest_packet_size;
-		if (INET_ECN_is_ce(PACKET_CB(skb)->ds))
-			IP_ECN_set_ce(ip_hdr(skb));
+		if (INET_ECN_decapsulate(skb, PACKET_CB(skb)->ds,
+					 ip_hdr(skb)->tos) > 1)
+			goto ecn_decap_error;
 	} else if (skb->protocol == htons(ETH_P_IPV6)) {
 		len = ntohs(ipv6_hdr(skb)->payload_len) +
 		      sizeof(struct ipv6hdr);
-		if (INET_ECN_is_ce(PACKET_CB(skb)->ds))
-			IP6_ECN_set_ce(skb, ipv6_hdr(skb));
+		if (INET_ECN_decapsulate(skb, PACKET_CB(skb)->ds,
+					 ipv6_get_dsfield(ipv6_hdr(skb))) > 1)
+			goto ecn_decap_error;
 	} else {
 		goto dishonest_packet_type;
 	}
@@ -446,6 +448,12 @@ static void wg_packet_consume_data_done(struct wg_peer *peer,
 	++dev->stats.rx_errors;
 	++dev->stats.rx_length_errors;
 	goto packet_processed;
+ecn_decap_error:
+	net_dbg_ratelimited("%s: Non-ECT packet from peer %llu (%pISpfsc)\n",
+			    dev->name, peer->internal_id, &peer->endpoint.addr);
+	++dev->stats.rx_errors;
+	++dev->stats.rx_length_errors;
+	goto packet_processed;
 packet_processed:
 	dev_kfree_skb(skb);
 }
-- 
2.26.2



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings
  2020-04-27 14:46 [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings 
@ 2020-04-27 19:53 ` Jason
  2020-04-27 20:42   ` 
  0 siblings, 1 reply; 11+ messages in thread
From: Jason @ 2020-04-27 19:53 UTC (permalink / raw)


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 at 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
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.

I wanted to check with you: is the analysis above correct? And can you
somehow imagine the ==2 case leading to different behavior, in which
the packet isn't dropped? Or would that ruin the "[de]congestion" part
of ECN? I just want to make sure I understand the full picture before
moving in one direction or another.

> +               if (INET_ECN_decapsulate(skb, PACKET_CB(skb)->ds,
> +                                        ip_hdr(skb)->tos) > 1)
> +               if (INET_ECN_decapsulate(skb, PACKET_CB(skb)->ds,
> +                                        ipv6_get_dsfield(ipv6_hdr(skb))) > 1)

The documentation for the function says:

*  returns 0 on success
*          1 if something is broken and should be logged (!!! above)
*          2 if packet should be dropped

Would it be more clear to explicitly check for ==2 then?

> +ecn_decap_error:
> +       net_dbg_ratelimited("%s: Non-ECT packet from peer %llu (%pISpfsc)\n",
> +                           dev->name, peer->internal_id, &peer->endpoint.addr);

All the other error messages in this block are in the form of: "Packet
.* from peer %llu (%pISpfsc)\n", so better text here might be "Packet
is non-ECT from peer %llu (%pISpfsc)\n". However, do you think we
really need to log to the console for this? Does it add much in the
way of wireguard internals debugging? Can't network congestion induce
it in complicated scenarios? Maybe it'd be best just to increment
those error counters instead.

> +       ++dev->stats.rx_errors;
> +       ++dev->stats.rx_length_errors;

This should use stats.rx_frame_errors instead of length_errors, which
is also what net/ipv6/sit.c and drivers/net/geneve.c do on ECN-related
drops.

Thanks,
Jason


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net v2] wireguard: use tunnel helpers for decapsulating ECN markings
  2020-04-27 20:42   ` 
@ 2020-04-27 21:16     ` 
  2020-04-27 23:09       ` Jason
  2020-04-28  1:09     ` [PATCH net] wireguard: Use " ietf
  1 sibling, 1 reply; 11+ messages in thread
From:  @ 2020-04-27 21:16 UTC (permalink / raw)


WireGuard currently only propagates ECN markings on tunnel decap according
to the old RFC3168 specification. However, the spec has since been updated
in RFC6040 to recommend slightly different decapsulation semantics. This
was implemented in the kernel as a set of common helpers for ECN
decapsulation, so let's just switch over WireGuard to using those, so it
can benefit from this enhancement and any future tweaks.

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.

Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
Reported-by: Olivier Tilmans <olivier.tilmans at nokia-bell-labs.com>
Cc: Dave Taht <dave.taht at gmail.com>
Cc: Rodney W. Grimes <ietf at gndrsh.dnsmgr.net>
Signed-off-by: Toke H?iland-J?rgensen <toke at redhat.com>
---
v2:
  - Don't log decap errors, and make sure they are recorded as frame errors,
    not length errors.

 drivers/net/wireguard/receive.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireguard/receive.c b/drivers/net/wireguard/receive.c
index da3b782ab7d3..ad36f358c807 100644
--- a/drivers/net/wireguard/receive.c
+++ b/drivers/net/wireguard/receive.c
@@ -393,13 +393,15 @@ static void wg_packet_consume_data_done(struct wg_peer *peer,
 		len = ntohs(ip_hdr(skb)->tot_len);
 		if (unlikely(len < sizeof(struct iphdr)))
 			goto dishonest_packet_size;
-		if (INET_ECN_is_ce(PACKET_CB(skb)->ds))
-			IP_ECN_set_ce(ip_hdr(skb));
+		if (INET_ECN_decapsulate(skb, PACKET_CB(skb)->ds,
+					 ip_hdr(skb)->tos) > 1)
+			goto ecn_decap_error;
 	} else if (skb->protocol == htons(ETH_P_IPV6)) {
 		len = ntohs(ipv6_hdr(skb)->payload_len) +
 		      sizeof(struct ipv6hdr);
-		if (INET_ECN_is_ce(PACKET_CB(skb)->ds))
-			IP6_ECN_set_ce(skb, ipv6_hdr(skb));
+		if (INET_ECN_decapsulate(skb, PACKET_CB(skb)->ds,
+					 ipv6_get_dsfield(ipv6_hdr(skb))) > 1)
+			goto ecn_decap_error;
 	} else {
 		goto dishonest_packet_type;
 	}
@@ -437,6 +439,7 @@ static void wg_packet_consume_data_done(struct wg_peer *peer,
 dishonest_packet_type:
 	net_dbg_ratelimited("%s: Packet is neither ipv4 nor ipv6 from peer %llu (%pISpfsc)\n",
 			    dev->name, peer->internal_id, &peer->endpoint.addr);
+ecn_decap_error:
 	++dev->stats.rx_errors;
 	++dev->stats.rx_frame_errors;
 	goto packet_processed;
-- 
2.26.2



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net v2] wireguard: use tunnel helpers for decapsulating ECN markings
  2020-04-27 21:16     ` [PATCH net v2] wireguard: use " 
@ 2020-04-27 23:09       ` Jason
  2020-04-28  9:00         ` 
  0 siblings, 1 reply; 11+ messages in thread
From: Jason @ 2020-04-27 23:09 UTC (permalink / raw)


On Mon, Apr 27, 2020 at 3:16 PM Toke H?iland-J?rgensen <toke at redhat.com> wrote:
>
> WireGuard currently only propagates ECN markings on tunnel decap according
> to the old RFC3168 specification. However, the spec has since been updated
> in RFC6040 to recommend slightly different decapsulation semantics. This
> was implemented in the kernel as a set of common helpers for ECN
> decapsulation, so let's just switch over WireGuard to using those, so it
> can benefit from this enhancement and any future tweaks.
>
> 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.

Thanks for the details in your other email and for this v2. I've
applied this to the wireguard tree and will send things up to net
later this week with a few other things brewing there.

By the way, the original code came out of a discussion I had with Dave
Taht while I was coding this on an airplane many years ago. I read
some old RFCs, made some changes, he tested them with cake, and told
me that the behavior looked correct. And that's about as far as I've
forayed into ECN land with WireGuard. It seems like it might be
helpful (at some point) to add something to the netns.sh test to make
sure that all this machinery is actually working and continues to work
properly as things change in the future.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net v2] wireguard: use tunnel helpers for decapsulating ECN markings
  2020-04-27 23:09       ` Jason
@ 2020-04-28  9:00         ` 
  0 siblings, 0 replies; 11+ messages in thread
From:  @ 2020-04-28  9:00 UTC (permalink / raw)


"Jason A. Donenfeld" <Jason at zx2c4.com> writes:

> On Mon, Apr 27, 2020 at 3:16 PM Toke H?iland-J?rgensen <toke at redhat.com> wrote:
>>
>> WireGuard currently only propagates ECN markings on tunnel decap according
>> to the old RFC3168 specification. However, the spec has since been updated
>> in RFC6040 to recommend slightly different decapsulation semantics. This
>> was implemented in the kernel as a set of common helpers for ECN
>> decapsulation, so let's just switch over WireGuard to using those, so it
>> can benefit from this enhancement and any future tweaks.
>>
>> 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.
>
> Thanks for the details in your other email and for this v2. I've
> applied this to the wireguard tree and will send things up to net
> later this week with a few other things brewing there.

Thanks!

> By the way, the original code came out of a discussion I had with Dave
> Taht while I was coding this on an airplane many years ago. I read
> some old RFCs, made some changes, he tested them with cake, and told
> me that the behavior looked correct. And that's about as far as I've
> forayed into ECN land with WireGuard. It seems like it might be
> helpful (at some point) to add something to the netns.sh test to make
> sure that all this machinery is actually working and continues to work
> properly as things change in the future.

Yeah, good point. I guess I can look into that too at some point :)

-Toke



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings
  2020-04-28  1:09     ` [PATCH net] wireguard: Use " ietf
@ 2020-04-28  9:10       ` 
  2020-04-28 18:52         ` dave.taht
  0 siblings, 1 reply; 11+ messages in thread
From:  @ 2020-04-28  9:10 UTC (permalink / raw)


"Rodney W. Grimes" <ietf at 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 at 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 at 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.
>
> Thanks for this.
>
>> 
>> > I wanted to check with you: is the analysis above correct? And can you
>> > somehow imagine the ==2 case leading to different behavior, in which
>> > the packet isn't dropped? Or would that ruin the "[de]congestion" part
>> > of ECN? I just want to make sure I understand the full picture before
>> > moving in one direction or another.
>> 
>> So I think the logic here is supposed to be that if there are CE marks
>> on the outer header, then an AQM somewhere along the path has marked the
>> packet, which is supposed to be a congestion signal, which we want to
>> propagate all the way to the receiver (who will then echo it back to the
>> receiver). However, if the inner packet is non-ECT then we can't
>> actually propagate the ECN signal; and a drop is thus the only
>> alternative congestion signal available to us.
>
> You cannot get a CE mark on the outer packet if the inner packet is
> not ECT, as the outer packet would also be not ECT and thus not
> eligible for CE mark.  If you get the above sited condition something
> has gone *wrong*.

Yup, you're quite right. If everything is working correctly, this should
never happen. This being the internet, though, there are bound to be
cases where it will go wrong :)

>> This case shouldn't
>> actually happen that often, a middlebox has to be misconfigured to
>> CE-mark a non-ECT packet in the first place. But, well, misconfigured
>> middleboxes do exist as you're no doubt aware :)
>
> That is true, though I believe the be liberal in what you accept
> concept would say ok, someone messed up, just propogate it and
> let the end nodes deal with it, otherwise your creating a blackhole
> that could be very hard to find.

But that would lead you to ignore a congestion signal. And someone has
to go through an awful lot of trouble to set this signal; if they're
just randomly mangling bits the packet checksum will likely be wrong and
the packet would be dropped anyway. So on balance I'd tend to agree with
the RFC that the right thing to do is to propagate the congestion
signal; which in the case of a non-ECT packet means dropping it,
otherwise we'd just be contributing to the RFC-violating behaviour...

I do believe the advice in the RFC to log these cases is exactly because
of the risk of blackholes you're referring to. I discussed this a bit
with Jason and we ended up agreeing that just marking it as a framing
error should be enough for Wireguard, though...

-Toke



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings
  2020-04-28  9:10       ` 
@ 2020-04-28 18:52         ` dave.taht
  2020-04-28 19:31           ` Jason
  0 siblings, 1 reply; 11+ messages in thread
From: dave.taht @ 2020-04-28 18:52 UTC (permalink / raw)


On Tue, Apr 28, 2020 at 2:10 AM Toke H?iland-J?rgensen <toke at redhat.com> wrote:
>
> "Rodney W. Grimes" <ietf at 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 at 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 at 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 this.
> >
> >>
> >> > I wanted to check with you: is the analysis above correct? And can you
> >> > somehow imagine the ==2 case leading to different behavior, in which
> >> > the packet isn't dropped? Or would that ruin the "[de]congestion" part
> >> > of ECN? I just want to make sure I understand the full picture before
> >> > moving in one direction or another.
> >>
> >> So I think the logic here is supposed to be that if there are CE marks
> >> on the outer header, then an AQM somewhere along the path has marked the
> >> packet, which is supposed to be a congestion signal, which we want to
> >> propagate all the way to the receiver (who will then echo it back to the
> >> receiver). However, if the inner packet is non-ECT then we can't
> >> actually propagate the ECN signal; and a drop is thus the only
> >> alternative congestion signal available to us.
> >
> > You cannot get a CE mark on the outer packet if the inner packet is
> > not ECT, as the outer packet would also be not ECT and thus not
> > eligible for CE mark.  If you get the above sited condition something
> > has gone *wrong*.
>
> Yup, you're quite right. If everything is working correctly, this should
> never happen. This being the internet, though, there are bound to be
> cases where it will go wrong :)
>
> >> This case shouldn't
> >> actually happen that often, a middlebox has to be misconfigured to
> >> CE-mark a non-ECT packet in the first place. But, well, misconfigured
> >> middleboxes do exist as you're no doubt aware :)
> >
> > That is true, though I believe the be liberal in what you accept
> > concept would say ok, someone messed up, just propogate it and
> > let the end nodes deal with it, otherwise your creating a blackhole
> > that could be very hard to find.
>
> But that would lead you to ignore a congestion signal. And someone has
> to go through an awful lot of trouble to set this signal; if they're
> just randomly mangling bits the packet checksum will likely be wrong and
> the packet would be dropped anyway. So on balance I'd tend to agree with
> the RFC that the right thing to do is to propagate the congestion
> signal; which in the case of a non-ECT packet means dropping it,
> otherwise we'd just be contributing to the RFC-violating behaviour...
>
> I do believe the advice in the RFC to log these cases is exactly because
> of the risk of blackholes you're referring to. I discussed this a bit
> with Jason and we ended up agreeing that just marking it as a framing
> error should be enough for Wireguard, though...
>
> -Toke
>


-- 
Make Music, Not War

Dave T?ht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-435-0729


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings
  2020-04-28 19:31           ` Jason
@ 2020-04-29  8:22             ` ietf
  0 siblings, 0 replies; 11+ messages in thread
From: ietf @ 2020-04-29  8:22 UTC (permalink / raw)


> On Tue, Apr 28, 2020 at 12:52 PM Dave Taht <dave.taht at gmail.com> wrote:
> >
> > On Tue, Apr 28, 2020 at 2:10 AM Toke H?iland-J?rgensen <toke at redhat.com> wrote:
> > >
> > > "Rodney W. Grimes" <ietf at 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 at 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 at 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 at freebsd.org


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings
  2020-04-28 18:52         ` dave.taht
@ 2020-04-28 19:31           ` Jason
  2020-04-29  8:22             ` ietf
  0 siblings, 1 reply; 11+ messages in thread
From: Jason @ 2020-04-28 19:31 UTC (permalink / raw)


On Tue, Apr 28, 2020 at 12:52 PM Dave Taht <dave.taht at gmail.com> wrote:
>
> On Tue, Apr 28, 2020 at 2:10 AM Toke H?iland-J?rgensen <toke at redhat.com> wrote:
> >
> > "Rodney W. Grimes" <ietf at 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 at 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 at 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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings
  2020-04-27 20:42   ` 
  2020-04-27 21:16     ` [PATCH net v2] wireguard: use " 
@ 2020-04-28  1:09     ` ietf
  2020-04-28  9:10       ` 
  1 sibling, 1 reply; 11+ messages in thread
From: ietf @ 2020-04-28  1:09 UTC (permalink / raw)


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 at 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 at 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.

Thanks for this.

> 
> > I wanted to check with you: is the analysis above correct? And can you
> > somehow imagine the ==2 case leading to different behavior, in which
> > the packet isn't dropped? Or would that ruin the "[de]congestion" part
> > of ECN? I just want to make sure I understand the full picture before
> > moving in one direction or another.
> 
> So I think the logic here is supposed to be that if there are CE marks
> on the outer header, then an AQM somewhere along the path has marked the
> packet, which is supposed to be a congestion signal, which we want to
> propagate all the way to the receiver (who will then echo it back to the
> receiver). However, if the inner packet is non-ECT then we can't
> actually propagate the ECN signal; and a drop is thus the only
> alternative congestion signal available to us.

You cannot get a CE mark on the outer packet if the inner packet is
not ECT, as the outer packet would also be not ECT and thus not
eligible for CE mark.  If you get the above sited condition something
has gone *wrong*.


> This case shouldn't
> actually happen that often, a middlebox has to be misconfigured to
> CE-mark a non-ECT packet in the first place. But, well, misconfigured
> middleboxes do exist as you're no doubt aware :)

That is true, though I believe the be liberal in what you accept
concept would say ok, someone messed up, just propogate it and
let the end nodes deal with it, otherwise your creating a blackhole
that could be very hard to find.

> 
> >> +               if (INET_ECN_decapsulate(skb, PACKET_CB(skb)->ds,
> >> +                                        ip_hdr(skb)->tos) > 1)
> >> +               if (INET_ECN_decapsulate(skb, PACKET_CB(skb)->ds,
> >> +                                        ipv6_get_dsfield(ipv6_hdr(skb))) > 1)
> >
> > The documentation for the function says:
> >
> > *  returns 0 on success
> > *          1 if something is broken and should be logged (!!! above)
> > *          2 if packet should be dropped
> >
> > Would it be more clear to explicitly check for ==2 then?
> 
> Hmm, maybe? Other callers seem to use >1, so I figured it was better to
> be consistent with those. I won't insist on that, though, so if you'd
> rather I use a ==2 check I can certainly change it?
> 
> >> +ecn_decap_error:
> >> +       net_dbg_ratelimited("%s: Non-ECT packet from peer %llu (%pISpfsc)\n",
> >> +                           dev->name, peer->internal_id, &peer->endpoint.addr);
> >
> > All the other error messages in this block are in the form of: "Packet
> > .* from peer %llu (%pISpfsc)\n", so better text here might be "Packet
> > is non-ECT from peer %llu (%pISpfsc)\n". However, do you think we
> > really need to log to the console for this? Does it add much in the
> > way of wireguard internals debugging? Can't network congestion induce
> > it in complicated scenarios? Maybe it'd be best just to increment
> > those error counters instead.
> 
> The other callers do seem to hide the logging behind a module parameter
> specifically for this purpose. I put in this log message because the use
> of net_dbg() indicated that these were already meant for non-production
> error debugging. But if you'd rather avoid the logging that's fine by me.
> 
> >> +       ++dev->stats.rx_errors;
> >> +       ++dev->stats.rx_length_errors;
> >
> > This should use stats.rx_frame_errors instead of length_errors, which
> > is also what net/ipv6/sit.c and drivers/net/geneve.c do on ECN-related
> > drops.
> 
> Oops, that's my bad; copied the wrong error handling block it seems
> (didn't even notice they were incrementing different counters). Will
> fix.
> 
> -Toke
> 
> [0] https://lore.kernel.org/netdev/20200427141105.555251-1-toke at redhat.com/T/#u
> 
> 
> 

-- 
Rod Grimes                                                 rgrimes at freebsd.org


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings
  2020-04-27 19:53 ` Jason
@ 2020-04-27 20:42   ` 
  2020-04-27 21:16     ` [PATCH net v2] wireguard: use " 
  2020-04-28  1:09     ` [PATCH net] wireguard: Use " ietf
  0 siblings, 2 replies; 11+ messages in thread
From:  @ 2020-04-27 20:42 UTC (permalink / raw)


"Jason A. Donenfeld" <Jason at 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 at 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
> 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
(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 wanted to check with you: is the analysis above correct? And can you
> somehow imagine the ==2 case leading to different behavior, in which
> the packet isn't dropped? Or would that ruin the "[de]congestion" part
> of ECN? I just want to make sure I understand the full picture before
> moving in one direction or another.

So I think the logic here is supposed to be that if there are CE marks
on the outer header, then an AQM somewhere along the path has marked the
packet, which is supposed to be a congestion signal, which we want to
propagate all the way to the receiver (who will then echo it back to the
receiver). However, if the inner packet is non-ECT then we can't
actually propagate the ECN signal; and a drop is thus the only
alternative congestion signal available to us. This case shouldn't
actually happen that often, a middlebox has to be misconfigured to
CE-mark a non-ECT packet in the first place. But, well, misconfigured
middleboxes do exist as you're no doubt aware :)

>> +               if (INET_ECN_decapsulate(skb, PACKET_CB(skb)->ds,
>> +                                        ip_hdr(skb)->tos) > 1)
>> +               if (INET_ECN_decapsulate(skb, PACKET_CB(skb)->ds,
>> +                                        ipv6_get_dsfield(ipv6_hdr(skb))) > 1)
>
> The documentation for the function says:
>
> *  returns 0 on success
> *          1 if something is broken and should be logged (!!! above)
> *          2 if packet should be dropped
>
> Would it be more clear to explicitly check for ==2 then?

Hmm, maybe? Other callers seem to use >1, so I figured it was better to
be consistent with those. I won't insist on that, though, so if you'd
rather I use a ==2 check I can certainly change it?

>> +ecn_decap_error:
>> +       net_dbg_ratelimited("%s: Non-ECT packet from peer %llu (%pISpfsc)\n",
>> +                           dev->name, peer->internal_id, &peer->endpoint.addr);
>
> All the other error messages in this block are in the form of: "Packet
> .* from peer %llu (%pISpfsc)\n", so better text here might be "Packet
> is non-ECT from peer %llu (%pISpfsc)\n". However, do you think we
> really need to log to the console for this? Does it add much in the
> way of wireguard internals debugging? Can't network congestion induce
> it in complicated scenarios? Maybe it'd be best just to increment
> those error counters instead.

The other callers do seem to hide the logging behind a module parameter
specifically for this purpose. I put in this log message because the use
of net_dbg() indicated that these were already meant for non-production
error debugging. But if you'd rather avoid the logging that's fine by me.

>> +       ++dev->stats.rx_errors;
>> +       ++dev->stats.rx_length_errors;
>
> This should use stats.rx_frame_errors instead of length_errors, which
> is also what net/ipv6/sit.c and drivers/net/geneve.c do on ECN-related
> drops.

Oops, that's my bad; copied the wrong error handling block it seems
(didn't even notice they were incrementing different counters). Will
fix.

-Toke

[0] https://lore.kernel.org/netdev/20200427141105.555251-1-toke at redhat.com/T/#u



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-27 14:46 [PATCH net] wireguard: Use tunnel helpers for decapsulating ECN markings 
2020-04-27 19:53 ` Jason
2020-04-27 20:42   ` 
2020-04-27 21:16     ` [PATCH net v2] wireguard: use " 
2020-04-27 23:09       ` Jason
2020-04-28  9:00         ` 
2020-04-28  1:09     ` [PATCH net] wireguard: Use " ietf
2020-04-28  9:10       ` 
2020-04-28 18:52         ` dave.taht
2020-04-28 19:31           ` Jason
2020-04-29  8:22             ` ietf

Development discussion of WireGuard

Archives are clonable: git clone --mirror http://inbox.vuxu.org/wireguard

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.wireguard


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git