[PATCH 0/2] net: dccp: fix structure use-after-free

[PATCH 0/2] net: dccp: fix structure use-after-free

[Kleber Sacilotto de Souza]

This patchset addresses the following CVE:

CVE-2020-16119 - DCCP CCID structure use-after-free

Hadar Manor reported that by reusing a socket with an attached
dccps_hc_tx_ccid as a listener, it will be used after being released,
leading to DoS and potentially code execution.

The first patch moves the ccid timers to struct dccp_sock to avoid its
use-after-free, the second patch reverts 2677d2067731 "dccp: don't free
ccid2_hc_tx_sock struct in dccp_disconnect()" that's not needed anymore
and would cause another use-after-free.

Thadeu Lima de Souza Cascardo (2):
  dccp: ccid: move timers to struct dccp_sock
  Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()"

 include/linux/dccp.h   |  2 ++
 net/dccp/ccids/ccid2.c | 32 +++++++++++++++++++-------------
 net/dccp/ccids/ccid3.c | 30 ++++++++++++++++++++----------
 net/dccp/proto.c       |  2 ++
 4 files changed, 43 insertions(+), 23 deletions(-)

-- 
2.25.1

[PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Kleber Sacilotto de Souza]

From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
del_timer_sync can't be used is because this relies on keeping a reference
to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
during disconnect, the timer should really belong to struct dccp_sock.

This addresses CVE-2020-16119.

Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
---
 include/linux/dccp.h   |  2 ++
 net/dccp/ccids/ccid2.c | 32 +++++++++++++++++++-------------
 net/dccp/ccids/ccid3.c | 30 ++++++++++++++++++++----------
 3 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/include/linux/dccp.h b/include/linux/dccp.h
index 07e547c02fd8..504afa1a4be6 100644
--- a/include/linux/dccp.h
+++ b/include/linux/dccp.h
@@ -259,6 +259,7 @@ struct dccp_ackvec;
  * @dccps_sync_scheduled - flag which signals "send out-of-band message soon"
  * @dccps_xmitlet - tasklet scheduled by the TX CCID to dequeue data packets
  * @dccps_xmit_timer - used by the TX CCID to delay sending (rate-based pacing)
+ * @dccps_ccid_timer - used by the CCIDs
  * @dccps_syn_rtt - RTT sample from Request/Response exchange (in usecs)
  */
 struct dccp_sock {
@@ -303,6 +304,7 @@ struct dccp_sock {
 	__u8				dccps_sync_scheduled:1;
 	struct tasklet_struct		dccps_xmitlet;
 	struct timer_list		dccps_xmit_timer;
+	struct timer_list		dccps_ccid_timer;
 };
 
 static inline struct dccp_sock *dccp_sk(const struct sock *sk)
diff --git a/net/dccp/ccids/ccid2.c b/net/dccp/ccids/ccid2.c
index 3da1f77bd039..dbca1f1e2449 100644
--- a/net/dccp/ccids/ccid2.c
+++ b/net/dccp/ccids/ccid2.c
@@ -126,21 +126,26 @@ static void dccp_tasklet_schedule(struct sock *sk)
 
 static void ccid2_hc_tx_rto_expire(struct timer_list *t)
 {
-	struct ccid2_hc_tx_sock *hc = from_timer(hc, t, tx_rtotimer);
-	struct sock *sk = hc->sk;
-	const bool sender_was_blocked = ccid2_cwnd_network_limited(hc);
+	struct dccp_sock *dp = from_timer(dp, t, dccps_ccid_timer);
+	struct sock *sk = (struct sock *)dp;
+	struct ccid2_hc_tx_sock *hc;
+	bool sender_was_blocked;
 
 	bh_lock_sock(sk);
+
+	if (inet_sk_state_load(sk) == DCCP_CLOSED)
+		goto out;
+
+	hc = ccid_priv(dp->dccps_hc_tx_ccid);
+	sender_was_blocked = ccid2_cwnd_network_limited(hc);
+
 	if (sock_owned_by_user(sk)) {
-		sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + HZ / 5);
+		sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + HZ / 5);
 		goto out;
 	}
 
 	ccid2_pr_debug("RTO_EXPIRE\n");
 
-	if (sk->sk_state == DCCP_CLOSED)
-		goto out;
-
 	/* back-off timer */
 	hc->tx_rto <<= 1;
 	if (hc->tx_rto > DCCP_RTO_MAX)
@@ -166,7 +171,7 @@ static void ccid2_hc_tx_rto_expire(struct timer_list *t)
 	if (sender_was_blocked)
 		dccp_tasklet_schedule(sk);
 	/* restart backed-off timer */
-	sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + hc->tx_rto);
+	sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + hc->tx_rto);
 out:
 	bh_unlock_sock(sk);
 	sock_put(sk);
@@ -330,7 +335,7 @@ static void ccid2_hc_tx_packet_sent(struct sock *sk, unsigned int len)
 	}
 #endif
 
-	sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + hc->tx_rto);
+	sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + hc->tx_rto);
 
 #ifdef CONFIG_IP_DCCP_CCID2_DEBUG
 	do {
@@ -700,9 +705,9 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 
 	/* restart RTO timer if not all outstanding data has been acked */
 	if (hc->tx_pipe == 0)
-		sk_stop_timer(sk, &hc->tx_rtotimer);
+		sk_stop_timer(sk, &dp->dccps_ccid_timer);
 	else
-		sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + hc->tx_rto);
+		sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + hc->tx_rto);
 done:
 	/* check if incoming Acks allow pending packets to be sent */
 	if (sender_was_blocked && !ccid2_cwnd_network_limited(hc))
@@ -737,17 +742,18 @@ static int ccid2_hc_tx_init(struct ccid *ccid, struct sock *sk)
 	hc->tx_last_cong = hc->tx_lsndtime = hc->tx_cwnd_stamp = ccid2_jiffies32;
 	hc->tx_cwnd_used = 0;
 	hc->sk		 = sk;
-	timer_setup(&hc->tx_rtotimer, ccid2_hc_tx_rto_expire, 0);
+	timer_setup(&dp->dccps_ccid_timer, ccid2_hc_tx_rto_expire, 0);
 	INIT_LIST_HEAD(&hc->tx_av_chunks);
 	return 0;
 }
 
 static void ccid2_hc_tx_exit(struct sock *sk)
 {
+	struct dccp_sock *dp = dccp_sk(sk);
 	struct ccid2_hc_tx_sock *hc = ccid2_hc_tx_sk(sk);
 	int i;
 
-	sk_stop_timer(sk, &hc->tx_rtotimer);
+	sk_stop_timer(sk, &dp->dccps_ccid_timer);
 
 	for (i = 0; i < hc->tx_seqbufc; i++)
 		kfree(hc->tx_seqbuf[i]);
diff --git a/net/dccp/ccids/ccid3.c b/net/dccp/ccids/ccid3.c
index b9ee1a4a8955..685f4d046c0d 100644
--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -184,17 +184,24 @@ static inline void ccid3_hc_tx_update_win_count(struct ccid3_hc_tx_sock *hc,
 
 static void ccid3_hc_tx_no_feedback_timer(struct timer_list *t)
 {
-	struct ccid3_hc_tx_sock *hc = from_timer(hc, t, tx_no_feedback_timer);
-	struct sock *sk = hc->sk;
+	struct dccp_sock *dp = from_timer(dp, t, dccps_ccid_timer);
+	struct ccid3_hc_tx_sock *hc;
+	struct sock *sk = (struct sock *)dp;
 	unsigned long t_nfb = USEC_PER_SEC / 5;
 
 	bh_lock_sock(sk);
+
+	if (inet_sk_state_load(sk) == DCCP_CLOSED)
+		goto out;
+
 	if (sock_owned_by_user(sk)) {
 		/* Try again later. */
 		/* XXX: set some sensible MIB */
 		goto restart_timer;
 	}
 
+	hc = ccid_priv(dp->dccps_hc_tx_ccid);
+
 	ccid3_pr_debug("%s(%p, state=%s) - entry\n", dccp_role(sk), sk,
 		       ccid3_tx_state_name(hc->tx_state));
 
@@ -250,8 +257,8 @@ static void ccid3_hc_tx_no_feedback_timer(struct timer_list *t)
 		t_nfb = max(hc->tx_t_rto, 2 * hc->tx_t_ipi);
 
 restart_timer:
-	sk_reset_timer(sk, &hc->tx_no_feedback_timer,
-			   jiffies + usecs_to_jiffies(t_nfb));
+	sk_reset_timer(sk, &dp->dccps_ccid_timer,
+		       jiffies + usecs_to_jiffies(t_nfb));
 out:
 	bh_unlock_sock(sk);
 	sock_put(sk);
@@ -280,7 +287,7 @@ static int ccid3_hc_tx_send_packet(struct sock *sk, struct sk_buff *skb)
 		return -EBADMSG;
 
 	if (hc->tx_state == TFRC_SSTATE_NO_SENT) {
-		sk_reset_timer(sk, &hc->tx_no_feedback_timer, (jiffies +
+		sk_reset_timer(sk, &dp->dccps_ccid_timer, (jiffies +
 			       usecs_to_jiffies(TFRC_INITIAL_TIMEOUT)));
 		hc->tx_last_win_count	= 0;
 		hc->tx_t_last_win_count = now;
@@ -354,6 +361,7 @@ static void ccid3_hc_tx_packet_sent(struct sock *sk, unsigned int len)
 static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 {
 	struct ccid3_hc_tx_sock *hc = ccid3_hc_tx_sk(sk);
+	struct dccp_sock *dp = dccp_sk(sk);
 	struct tfrc_tx_hist_entry *acked;
 	ktime_t now;
 	unsigned long t_nfb;
@@ -420,7 +428,7 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 			       (unsigned int)(hc->tx_x >> 6));
 
 	/* unschedule no feedback timer */
-	sk_stop_timer(sk, &hc->tx_no_feedback_timer);
+	sk_stop_timer(sk, &dp->dccps_ccid_timer);
 
 	/*
 	 * As we have calculated new ipi, delta, t_nom it is possible
@@ -445,8 +453,8 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 		       "expire in %lu jiffies (%luus)\n",
 		       dccp_role(sk), sk, usecs_to_jiffies(t_nfb), t_nfb);
 
-	sk_reset_timer(sk, &hc->tx_no_feedback_timer,
-			   jiffies + usecs_to_jiffies(t_nfb));
+	sk_reset_timer(sk, &dp->dccps_ccid_timer,
+		       jiffies + usecs_to_jiffies(t_nfb));
 }
 
 static int ccid3_hc_tx_parse_options(struct sock *sk, u8 packet_type,
@@ -488,21 +496,23 @@ static int ccid3_hc_tx_parse_options(struct sock *sk, u8 packet_type,
 
 static int ccid3_hc_tx_init(struct ccid *ccid, struct sock *sk)
 {
+	struct dccp_sock *dp = dccp_sk(sk);
 	struct ccid3_hc_tx_sock *hc = ccid_priv(ccid);
 
 	hc->tx_state = TFRC_SSTATE_NO_SENT;
 	hc->tx_hist  = NULL;
 	hc->sk	     = sk;
-	timer_setup(&hc->tx_no_feedback_timer,
+	timer_setup(&dp->dccps_ccid_timer,
 		    ccid3_hc_tx_no_feedback_timer, 0);
 	return 0;
 }
 
 static void ccid3_hc_tx_exit(struct sock *sk)
 {
+	struct dccp_sock *dp = dccp_sk(sk);
 	struct ccid3_hc_tx_sock *hc = ccid3_hc_tx_sk(sk);
 
-	sk_stop_timer(sk, &hc->tx_no_feedback_timer);
+	sk_stop_timer(sk, &dp->dccps_ccid_timer);
 	tfrc_tx_hist_purge(&hc->tx_hist);
 }
 
-- 
2.25.1

[PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()"

[Kleber Sacilotto de Souza]

From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

This reverts commit 2677d20677314101293e6da0094ede7b5526d2b1.

This fixes an issue that after disconnect, dccps_hc_tx_ccid will still be
kept, allowing the socket to be reused as a listener socket, and the cloned
socket will free its dccps_hc_tx_ccid, leading to a later use after free,
when the listener socket is closed.

This addresses CVE-2020-16119.

Fixes: 2677d2067731 (dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect())
Reported-by: Hadar Manor
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
---
 net/dccp/proto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/dccp/proto.c b/net/dccp/proto.c
index 6d705d90c614..359e848dba6c 100644
--- a/net/dccp/proto.c
+++ b/net/dccp/proto.c
@@ -279,7 +279,9 @@ int dccp_disconnect(struct sock *sk, int flags)
 
 	dccp_clear_xmit_timers(sk);
 	ccid_hc_rx_delete(dp->dccps_hc_rx_ccid, sk);
+	ccid_hc_tx_delete(dp->dccps_hc_tx_ccid, sk);
 	dp->dccps_hc_rx_ccid = NULL;
+	dp->dccps_hc_tx_ccid = NULL;
 
 	__skb_queue_purge(&sk->sk_receive_queue);
 	__skb_queue_purge(&sk->sk_write_queue);
-- 
2.25.1

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Richard Sailer]

[-- Attachment #1.1: Type: text/plain, Size: 800 bytes --]

On 13/10/2020 19:18, Kleber Sacilotto de Souza wrote:
> From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> del_timer_sync can't be used is because this relies on keeping a reference
> to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> during disconnect, the timer should really belong to struct dccp_sock.
> 
> This addresses CVE-2020-16119.
> 
> Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

Acked-bd: Richard Sailer <richard_siegfried@systemli.org>

Implementation and concept looks fine to me


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()"

[Richard Sailer]

[-- Attachment #1.1: Type: text/plain, Size: 834 bytes --]

On 13/10/2020 19:18, Kleber Sacilotto de Souza wrote:
> rom: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> This reverts commit 2677d20677314101293e6da0094ede7b5526d2b1.
> 
> This fixes an issue that after disconnect, dccps_hc_tx_ccid will still be
> kept, allowing the socket to be reused as a listener socket, and the cloned
> socket will free its dccps_hc_tx_ccid, leading to a later use after free,
> when the listener socket is closed.
> 
> This addresses CVE-2020-16119.
> 
> Fixes: 2677d2067731 (dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect())
> Reported-by: Hadar Manor
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
> ---
Acked-by: Richard Sailer <richard_siegfried@systemli.org>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()"

[Jakub Kicinski]

On Tue, 13 Oct 2020 19:18:49 +0200 Kleber Sacilotto de Souza wrote:
> From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> This reverts commit 2677d20677314101293e6da0094ede7b5526d2b1.
> 
> This fixes an issue that after disconnect, dccps_hc_tx_ccid will still be
> kept, allowing the socket to be reused as a listener socket, and the cloned
> socket will free its dccps_hc_tx_ccid, leading to a later use after free,
> when the listener socket is closed.
> 
> This addresses CVE-2020-16119.
> 
> Fixes: 2677d2067731 (dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect())
> Reported-by: Hadar Manor

Does this person has an email address?

> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Jakub Kicinski]

On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:
> From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> del_timer_sync can't be used is because this relies on keeping a reference
> to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> during disconnect, the timer should really belong to struct dccp_sock.
> 
> This addresses CVE-2020-16119.
> 
> Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())

Presumably you chose this commit because the fix won't apply beyond it?
But it really fixes 2677d2067731 (dccp: don't free.. right?

> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

Re: [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()"

[Kleber Souza]

On 15.10.20 05:42, Jakub Kicinski wrote:
> On Tue, 13 Oct 2020 19:18:49 +0200 Kleber Sacilotto de Souza wrote:
>> From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>>
>> This reverts commit 2677d20677314101293e6da0094ede7b5526d2b1.
>>
>> This fixes an issue that after disconnect, dccps_hc_tx_ccid will still be
>> kept, allowing the socket to be reused as a listener socket, and the cloned
>> socket will free its dccps_hc_tx_ccid, leading to a later use after free,
>> when the listener socket is closed.
>>
>> This addresses CVE-2020-16119.
>>
>> Fixes: 2677d2067731 (dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect())
>> Reported-by: Hadar Manor
> 
> Does this person has an email address?

We have received this report via a private Launchpad bug and the submitter
didn't provide any public email address, so we have only their name.

> 
>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Thadeu Lima de Souza Cascardo]

On Wed, Oct 14, 2020 at 08:43:22PM -0700, Jakub Kicinski wrote:
> On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:
> > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > 
> > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> > del_timer_sync can't be used is because this relies on keeping a reference
> > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> > during disconnect, the timer should really belong to struct dccp_sock.
> > 
> > This addresses CVE-2020-16119.
> > 
> > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> 
> Presumably you chose this commit because the fix won't apply beyond it?
> But it really fixes 2677d2067731 (dccp: don't free.. right?

Well, it should also fix cases where dccps_hc_tx_ccid{,_private} has been freed
right after the timer is stopped.

So, we could add:
Fixes: 2a91aa396739 ([DCCP] CCID2: Initial CCID2 (TCP-Like) implementation)
Fixes: 7c657876b63c ([DCCP]: Initial implementation)

But I wouldn't say that this fixes 2677d2067731, unless there is argument to
say that it fixes it because it claimed to fix what is being fixed here. But
even the code that it removed was supposed to be stopping the timer, so how
could it ever fix what it was claiming to fix?

Thanks.
Cascardo.

> 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Jakub Kicinski]

On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:
> From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> del_timer_sync can't be used is because this relies on keeping a reference
> to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> during disconnect, the timer should really belong to struct dccp_sock.
> 
> This addresses CVE-2020-16119.
> 
> Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

I've been mulling over this fix.

The layering violation really doesn't sit well.

We're reusing the timer object. What if we are really unlucky, the
fires and gets blocked by a cosmic ray just as it's about to try to
lock the socket, then user manages to reconnect, and timer starts
again. Potentially with a different CCID algo altogether?

Is disconnect ever called under the BH lock?  Maybe plumb a bool
argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync()
when called from disconnect()?

Or do refcounting on ccid_priv so that the timer holds both the socket
and the priv?

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Thadeu Lima de Souza Cascardo]

On Fri, Oct 16, 2020 at 03:30:16PM -0700, Jakub Kicinski wrote:
> On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:
> > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > 
> > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> > del_timer_sync can't be used is because this relies on keeping a reference
> > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> > during disconnect, the timer should really belong to struct dccp_sock.
> > 
> > This addresses CVE-2020-16119.
> > 
> > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
> 
> I've been mulling over this fix.
> 
> The layering violation really doesn't sit well.
> 
> We're reusing the timer object. What if we are really unlucky, the
> fires and gets blocked by a cosmic ray just as it's about to try to
> lock the socket, then user manages to reconnect, and timer starts
> again. Potentially with a different CCID algo altogether?
> 
> Is disconnect ever called under the BH lock?  Maybe plumb a bool
> argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync()
> when called from disconnect()?
> 
> Or do refcounting on ccid_priv so that the timer holds both the socket
> and the priv?

Sorry about too late a response. I was on vacation, then came back and spent a
couple of days testing this further, and had to switch to other tasks.

So, while testing this, I had to resort to tricks like having a very small
expire and enqueuing on a different CPU. Then, after some minutes, I hit a UAF.
That's with or without the first of the second patch.

I also tried to refcount ccid instead of the socket, keeping the timer on the
ccid, but that still hit the UAF, and that's when I had to switch tasks. Oh,
and in the meantime, I found one or two other fixes that we should apply, will
send them shortly.

But I would argue that we should apply the revert as it addresses the CVE,
without really regressing the other UAF, as I argued. Does that make sense?

Thank you.
Cascardo.

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Jakub Kicinski]

On Mon, 9 Nov 2020 08:48:28 -0300 Thadeu Lima de Souza Cascardo wrote:
> On Fri, Oct 16, 2020 at 03:30:16PM -0700, Jakub Kicinski wrote:
> > On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:  
> > > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > 
> > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> > > del_timer_sync can't be used is because this relies on keeping a reference
> > > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> > > during disconnect, the timer should really belong to struct dccp_sock.
> > > 
> > > This addresses CVE-2020-16119.
> > > 
> > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>  
> > 
> > I've been mulling over this fix.
> > 
> > The layering violation really doesn't sit well.
> > 
> > We're reusing the timer object. What if we are really unlucky, the
> > fires and gets blocked by a cosmic ray just as it's about to try to
> > lock the socket, then user manages to reconnect, and timer starts
> > again. Potentially with a different CCID algo altogether?
> > 
> > Is disconnect ever called under the BH lock?  Maybe plumb a bool
> > argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync()
> > when called from disconnect()?
> > 
> > Or do refcounting on ccid_priv so that the timer holds both the socket
> > and the priv?  
> 
> Sorry about too late a response. I was on vacation, then came back and spent a
> couple of days testing this further, and had to switch to other tasks.
> 
> So, while testing this, I had to resort to tricks like having a very small
> expire and enqueuing on a different CPU. Then, after some minutes, I hit a UAF.
> That's with or without the first of the second patch.
> 
> I also tried to refcount ccid instead of the socket, keeping the timer on the
> ccid, but that still hit the UAF, and that's when I had to switch tasks.

Hm, not instead, as well. I think trying cancel the timer _sync from
the disconnect path would be the simplest solution, tho.

> Oh, and in the meantime, I found one or two other fixes that we
> should apply, will send them shortly.
> 
> But I would argue that we should apply the revert as it addresses the
> CVE, without really regressing the other UAF, as I argued. Does that
> make sense?

We can - it's always a little strange to go from one bug to a different
without a fix - but the justification being that while the previous UAF
required a race condition the new one is actually worst because it can 
be triggered reliably?

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Thadeu Lima de Souza Cascardo]

On Mon, Nov 09, 2020 at 09:49:38AM -0800, Jakub Kicinski wrote:
> On Mon, 9 Nov 2020 08:48:28 -0300 Thadeu Lima de Souza Cascardo wrote:
> > On Fri, Oct 16, 2020 at 03:30:16PM -0700, Jakub Kicinski wrote:
> > > On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:  
> > > > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > > 
> > > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> > > > del_timer_sync can't be used is because this relies on keeping a reference
> > > > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> > > > during disconnect, the timer should really belong to struct dccp_sock.
> > > > 
> > > > This addresses CVE-2020-16119.
> > > > 
> > > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>  
> > > 
> > > I've been mulling over this fix.
> > > 
> > > The layering violation really doesn't sit well.
> > > 
> > > We're reusing the timer object. What if we are really unlucky, the
> > > fires and gets blocked by a cosmic ray just as it's about to try to
> > > lock the socket, then user manages to reconnect, and timer starts
> > > again. Potentially with a different CCID algo altogether?
> > > 
> > > Is disconnect ever called under the BH lock?  Maybe plumb a bool
> > > argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync()
> > > when called from disconnect()?
> > > 
> > > Or do refcounting on ccid_priv so that the timer holds both the socket
> > > and the priv?  
> > 
> > Sorry about too late a response. I was on vacation, then came back and spent a
> > couple of days testing this further, and had to switch to other tasks.
> > 
> > So, while testing this, I had to resort to tricks like having a very small
> > expire and enqueuing on a different CPU. Then, after some minutes, I hit a UAF.
> > That's with or without the first of the second patch.
> > 
> > I also tried to refcount ccid instead of the socket, keeping the timer on the
> > ccid, but that still hit the UAF, and that's when I had to switch tasks.
> 
> Hm, not instead, as well. I think trying cancel the timer _sync from
> the disconnect path would be the simplest solution, tho.
> 

I don't think so. On other paths, we would still have the possibility that:

CPU1: timer expires and is about to run
CPU2: calls stop_timer (which does not stop anything) and frees ccid
CPU1: timer runs and uses freed ccid

And those paths, IIUC, may be run under a SoftIRQ on the receive path, so would
not be able to call stop_timer_sync.

> > Oh, and in the meantime, I found one or two other fixes that we
> > should apply, will send them shortly.
> > 
> > But I would argue that we should apply the revert as it addresses the
> > CVE, without really regressing the other UAF, as I argued. Does that
> > make sense?
> 
> We can - it's always a little strange to go from one bug to a different
> without a fix - but the justification being that while the previous UAF
> required a race condition the new one is actually worst because it can 
> be triggered reliably?

Well, I am arguing here that commit 2677d20677314101293e6da0094ede7b5526d2b1
("dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()") doesn't
really fix anything. Whenever ccid_hx_tx_delete is called, that UAF might
happen, because the timer might trigger right after we free the ccid struct.

And, yes, on the other hand, we can reliably launch the DoS attack that is
fixed by the revert of that commit.

Thanks.
Cascardo.

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Jakub Kicinski]

On Mon, 9 Nov 2020 18:09:09 -0300 Thadeu Lima de Souza Cascardo wrote:
> On Mon, Nov 09, 2020 at 09:49:38AM -0800, Jakub Kicinski wrote:
> > On Mon, 9 Nov 2020 08:48:28 -0300 Thadeu Lima de Souza Cascardo wrote:  
> > > On Fri, Oct 16, 2020 at 03:30:16PM -0700, Jakub Kicinski wrote:  
> > > > On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:    
> > > > > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > > > 
> > > > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> > > > > del_timer_sync can't be used is because this relies on keeping a reference
> > > > > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> > > > > during disconnect, the timer should really belong to struct dccp_sock.
> > > > > 
> > > > > This addresses CVE-2020-16119.
> > > > > 
> > > > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> > > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > > > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>    
> > > > 
> > > > I've been mulling over this fix.
> > > > 
> > > > The layering violation really doesn't sit well.
> > > > 
> > > > We're reusing the timer object. What if we are really unlucky, the
> > > > fires and gets blocked by a cosmic ray just as it's about to try to
> > > > lock the socket, then user manages to reconnect, and timer starts
> > > > again. Potentially with a different CCID algo altogether?
> > > > 
> > > > Is disconnect ever called under the BH lock?  Maybe plumb a bool
> > > > argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync()
> > > > when called from disconnect()?
> > > > 
> > > > Or do refcounting on ccid_priv so that the timer holds both the socket
> > > > and the priv?    
> > > 
> > > Sorry about too late a response. I was on vacation, then came back and spent a
> > > couple of days testing this further, and had to switch to other tasks.
> > > 
> > > So, while testing this, I had to resort to tricks like having a very small
> > > expire and enqueuing on a different CPU. Then, after some minutes, I hit a UAF.
> > > That's with or without the first of the second patch.
> > > 
> > > I also tried to refcount ccid instead of the socket, keeping the timer on the
> > > ccid, but that still hit the UAF, and that's when I had to switch tasks.  
> > 
> > Hm, not instead, as well. I think trying cancel the timer _sync from
> > the disconnect path would be the simplest solution, tho.
> 
> I don't think so. On other paths, we would still have the possibility that:
> 
> CPU1: timer expires and is about to run
> CPU2: calls stop_timer (which does not stop anything) and frees ccid
> CPU1: timer runs and uses freed ccid
> 
> And those paths, IIUC, may be run under a SoftIRQ on the receive path, so would
> not be able to call stop_timer_sync.

Which paths are those (my memory of this code is waning)? I thought
disconnect is only called from the user space side (shutdown syscall).
The only other way to terminate the connection is to close the socket,
which Eric already fixed by postponing the destruction of ccid in that
case.

> > > Oh, and in the meantime, I found one or two other fixes that we
> > > should apply, will send them shortly.
> > > 
> > > But I would argue that we should apply the revert as it addresses the
> > > CVE, without really regressing the other UAF, as I argued. Does that
> > > make sense?  
> > 
> > We can - it's always a little strange to go from one bug to a different
> > without a fix - but the justification being that while the previous UAF
> > required a race condition the new one is actually worst because it can 
> > be triggered reliably?  
> 
> Well, I am arguing here that commit 2677d20677314101293e6da0094ede7b5526d2b1
> ("dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()") doesn't
> really fix anything. Whenever ccid_hx_tx_delete is called, that UAF might
> happen, because the timer might trigger right after we free the ccid struct.
> 
> And, yes, on the other hand, we can reliably launch the DoS attack that is
> fixed by the revert of that commit.

OK.

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Thadeu Lima de Souza Cascardo]

On Mon, Nov 09, 2020 at 01:15:54PM -0800, Jakub Kicinski wrote:
> On Mon, 9 Nov 2020 18:09:09 -0300 Thadeu Lima de Souza Cascardo wrote:
> > On Mon, Nov 09, 2020 at 09:49:38AM -0800, Jakub Kicinski wrote:
> > > On Mon, 9 Nov 2020 08:48:28 -0300 Thadeu Lima de Souza Cascardo wrote:  
> > > > On Fri, Oct 16, 2020 at 03:30:16PM -0700, Jakub Kicinski wrote:  
> > > > > On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:    
> > > > > > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > > > > 
> > > > > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> > > > > > del_timer_sync can't be used is because this relies on keeping a reference
> > > > > > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> > > > > > during disconnect, the timer should really belong to struct dccp_sock.
> > > > > > 
> > > > > > This addresses CVE-2020-16119.
> > > > > > 
> > > > > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> > > > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > > > > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>    
> > > > > 
> > > > > I've been mulling over this fix.
> > > > > 
> > > > > The layering violation really doesn't sit well.
> > > > > 
> > > > > We're reusing the timer object. What if we are really unlucky, the
> > > > > fires and gets blocked by a cosmic ray just as it's about to try to
> > > > > lock the socket, then user manages to reconnect, and timer starts
> > > > > again. Potentially with a different CCID algo altogether?
> > > > > 
> > > > > Is disconnect ever called under the BH lock?  Maybe plumb a bool
> > > > > argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync()
> > > > > when called from disconnect()?
> > > > > 
> > > > > Or do refcounting on ccid_priv so that the timer holds both the socket
> > > > > and the priv?    
> > > > 
> > > > Sorry about too late a response. I was on vacation, then came back and spent a
> > > > couple of days testing this further, and had to switch to other tasks.
> > > > 
> > > > So, while testing this, I had to resort to tricks like having a very small
> > > > expire and enqueuing on a different CPU. Then, after some minutes, I hit a UAF.
> > > > That's with or without the first of the second patch.
> > > > 
> > > > I also tried to refcount ccid instead of the socket, keeping the timer on the
> > > > ccid, but that still hit the UAF, and that's when I had to switch tasks.  
> > > 
> > > Hm, not instead, as well. I think trying cancel the timer _sync from
> > > the disconnect path would be the simplest solution, tho.
> > 
> > I don't think so. On other paths, we would still have the possibility that:
> > 
> > CPU1: timer expires and is about to run
> > CPU2: calls stop_timer (which does not stop anything) and frees ccid
> > CPU1: timer runs and uses freed ccid
> > 
> > And those paths, IIUC, may be run under a SoftIRQ on the receive path, so would
> > not be able to call stop_timer_sync.
> 
> Which paths are those (my memory of this code is waning)? I thought
> disconnect is only called from the user space side (shutdown syscall).
> The only other way to terminate the connection is to close the socket,
> which Eric already fixed by postponing the destruction of ccid in that
> case.

dccp_v4_do_rcv -> dccp_rcv_established -> dccp_parse_options ->
	dccp_feat_parse_options -> dccp_feat_handle_nn_established ->
	dccp_feat_activate -> __dccp_feat_activate -> dccp_hdlr_ccid ->
	ccid_hc_tx_delete

> 
> > > > Oh, and in the meantime, I found one or two other fixes that we
> > > > should apply, will send them shortly.
> > > > 
> > > > But I would argue that we should apply the revert as it addresses the
> > > > CVE, without really regressing the other UAF, as I argued. Does that
> > > > make sense?  
> > > 
> > > We can - it's always a little strange to go from one bug to a different
> > > without a fix - but the justification being that while the previous UAF
> > > required a race condition the new one is actually worst because it can 
> > > be triggered reliably?  
> > 
> > Well, I am arguing here that commit 2677d20677314101293e6da0094ede7b5526d2b1
> > ("dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()") doesn't
> > really fix anything. Whenever ccid_hx_tx_delete is called, that UAF might
> > happen, because the timer might trigger right after we free the ccid struct.
> > 
> > And, yes, on the other hand, we can reliably launch the DoS attack that is
> > fixed by the revert of that commit.
> 
> OK.
> 

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Jakub Kicinski]

On Mon, 9 Nov 2020 18:31:34 -0300 Thadeu Lima de Souza Cascardo wrote:
> > Which paths are those (my memory of this code is waning)? I thought
> > disconnect is only called from the user space side (shutdown syscall).
> > The only other way to terminate the connection is to close the socket,
> > which Eric already fixed by postponing the destruction of ccid in that
> > case.  
> 
> dccp_v4_do_rcv -> dccp_rcv_established -> dccp_parse_options ->
> 	dccp_feat_parse_options -> dccp_feat_handle_nn_established ->
> 	dccp_feat_activate -> __dccp_feat_activate -> dccp_hdlr_ccid ->
> 	ccid_hc_tx_delete

Well, that's not a disconnect path.

There should be no CCID on a disconnected socket, tho, right? Otherwise
if we can switch from one active CCID to another then reusing a single
timer in struct dccp_sock for both is definitely not safe as I
explained in my initial email.

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Thadeu Lima de Souza Cascardo]

On Mon, Nov 09, 2020 at 02:15:53PM -0800, Jakub Kicinski wrote:
> On Mon, 9 Nov 2020 18:31:34 -0300 Thadeu Lima de Souza Cascardo wrote:
> > > Which paths are those (my memory of this code is waning)? I thought
> > > disconnect is only called from the user space side (shutdown syscall).
> > > The only other way to terminate the connection is to close the socket,
> > > which Eric already fixed by postponing the destruction of ccid in that
> > > case.  
> > 
> > dccp_v4_do_rcv -> dccp_rcv_established -> dccp_parse_options ->
> > 	dccp_feat_parse_options -> dccp_feat_handle_nn_established ->
> > 	dccp_feat_activate -> __dccp_feat_activate -> dccp_hdlr_ccid ->
> > 	ccid_hc_tx_delete
> 
> Well, that's not a disconnect path.
> 
> There should be no CCID on a disconnected socket, tho, right? Otherwise
> if we can switch from one active CCID to another then reusing a single
> timer in struct dccp_sock for both is definitely not safe as I
> explained in my initial email.

Yeah, I agree with your initial email. The patch I submitted for that fix needs
rework, which is what I tried and failed so far. I need to get back to some
testing of my latest fix and find out what needs fixing there.

But I am also saying that simply doing a del_timer_sync on disconnect paths
won't do, because there are non-disconnect paths where there is a CCID that we
will remove and replace and that will still trigger a timer UAF.

So I have been working on a fix that involves a refcnt on ccid itself. But I
want to test that it really fixes the problem and I have spent most of the time
finding out a way to trigger the timer in a race with the disconnect path.

And that same test has showed me that this timer UAF will happen regardless of
commit 2677d20677314101293e6da0094ede7b5526d2b1, which led me into stating that
reverting it should be done in any case.

I think I can find some time this week to work a little further on the fix for
the time UAF.

Thanks.
Cascardo.

Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock

[Jakub Kicinski]

On Tue, 10 Nov 2020 08:19:32 -0300 Thadeu Lima de Souza Cascardo wrote:
> Yeah, I agree with your initial email. The patch I submitted for that fix needs
> rework, which is what I tried and failed so far. I need to get back to some
> testing of my latest fix and find out what needs fixing there.
> 
> But I am also saying that simply doing a del_timer_sync on disconnect paths
> won't do, because there are non-disconnect paths where there is a CCID that we
> will remove and replace and that will still trigger a timer UAF.
> 
> So I have been working on a fix that involves a refcnt on ccid itself. But I
> want to test that it really fixes the problem and I have spent most of the time
> finding out a way to trigger the timer in a race with the disconnect path.

Sounds good, thanks a lot for working on this!

> And that same test has showed me that this timer UAF will happen regardless of
> commit 2677d20677314101293e6da0094ede7b5526d2b1, which led me into stating that
> reverting it should be done in any case.
> 
> I think I can find some time this week to work a little further on the fix for
> the time UAF.