[PATCH RFC] bluetooth: fix paring problems with 0cf3:0036 and certain devices

[PATCH RFC] bluetooth: fix paring problems with 0cf3:0036 and certain devices

[Andreas Kemnade]

This is clearly a RFC patch, things should be fixed in a cleaner way.
With the bluetooth usb adapter 0cf3:0036 (Atheros), there are pairing
problems in combination with certain remove devices. The cause is that
smp_resume_cb() gets called after SMP_CMD_IDENT_INFO arrives in the end 
of a SC pairing with numerical comparison. so that there are unexpected
packet messages in dmesg and although an ltk is known, bluetoothd
considers the device as unpaired and not even the ltk is not stored.

The patch makes the smp code be more tolerant with the order but that
does not feel like a safe thing to do.
Maybe the pairing result should be given back to userspace without the
irk related stuff.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 net/bluetooth/smp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index a2ddae2f37d7..8023b6e6342f 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1012,6 +1012,7 @@ static u8 smp_random(struct smp_chan *smp)
 			return SMP_UNSPECIFIED;
 
 		hci_le_start_enc(hcon, ediv, rand, stk, smp->enc_key_size);
+		smp_distribute_keys(smp);
 		hcon->enc_key_size = smp->enc_key_size;
 		set_bit(HCI_CONN_STK_ENCRYPT, &hcon->flags);
 	} else {
@@ -2837,6 +2838,7 @@ static int smp_cmd_dhkey_check(struct l2cap_conn *conn, struct sk_buff *skb)
 
 	if (hcon->out) {
 		hci_le_start_enc(hcon, 0, 0, smp->tk, smp->enc_key_size);
+		smp_distribute_keys(smp);
 		hcon->enc_key_size = smp->enc_key_size;
 	}
 
@@ -3067,7 +3069,6 @@ static void smp_resume_cb(struct l2cap_chan *chan)
 
 	cancel_delayed_work(&smp->security_timer);
 
-	smp_distribute_keys(smp);
 }
 
 static void smp_ready_cb(struct l2cap_chan *chan)
-- 
2.11.0

Re: [PATCH RFC] bluetooth: fix paring problems with 0cf3:0036 and certain devices

[Johan Hedberg]

Hi Andreas,

On Thu, Jun 14, 2018, Andreas Kemnade wrote:
> This is clearly a RFC patch, things should be fixed in a cleaner way.
> With the bluetooth usb adapter 0cf3:0036 (Atheros), there are pairing
> problems in combination with certain remove devices. The cause is that
> smp_resume_cb() gets called after SMP_CMD_IDENT_INFO arrives in the end 
> of a SC pairing with numerical comparison. so that there are unexpected
> packet messages in dmesg and although an ltk is known, bluetoothd
> considers the device as unpaired and not even the ltk is not stored.
> 
> The patch makes the smp code be more tolerant with the order but that
> does not feel like a safe thing to do.
> Maybe the pairing result should be given back to userspace without the
> irk related stuff.

This sounds like the well known race between ACL data and HCI events
that's inherent with the way the USB HCI transport is designed. So
what's happening is that to the host it looks like some packets were
delivered over an unencrypted connection even though they in reality
came over an encrypted one. Not enforcing the encrypted connection
requirement is not really a solution since it creates a security
vulnerabilty. The best option (as far as I know) is to consider
switching to some other transport (e.g. H:4 over USB where both ACL data
and HCI events share a single "pipe").

Johan

Re: [PATCH RFC] bluetooth: fix paring problems with 0cf3:0036 and certain devices

[Andreas Kemnade]

Hi Johan,

On Thu, 14 Jun 2018 22:35:00 +0200
Johan Hedberg <johan.hedberg@gmail.com> wrote:

> Hi Andreas,
> 
> On Thu, Jun 14, 2018, Andreas Kemnade wrote:
> > This is clearly a RFC patch, things should be fixed in a cleaner
> > way. With the bluetooth usb adapter 0cf3:0036 (Atheros), there are
> > pairing problems in combination with certain remove devices. The
> > cause is that smp_resume_cb() gets called after SMP_CMD_IDENT_INFO
> > arrives in the end of a SC pairing with numerical comparison. so
> > that there are unexpected packet messages in dmesg and although an
> > ltk is known, bluetoothd considers the device as unpaired and not
> > even the ltk is not stored.
> > 
> > The patch makes the smp code be more tolerant with the order but
> > that does not feel like a safe thing to do.
> > Maybe the pairing result should be given back to userspace without
> > the irk related stuff.  
> 
> This sounds like the well known race between ACL data and HCI events
> that's inherent with the way the USB HCI transport is designed. So
> what's happening is that to the host it looks like some packets were
> delivered over an unencrypted connection even though they in reality
> came over an encrypted one. 

That matches what I thought. But then the question arises whether this
race also can happen the oppositie way, so that the host thinks it gets
an encrypted packet but that is really an unencrypted one? Ugly.

But interrupt endpoints are designed for correct timing, so there
might be something rotten in the adapter.

> Not enforcing the encrypted connection
> requirement is not really a solution since it creates a security
> vulnerabilty. The best option (as far as I know) is to consider
> switching to some other transport (e.g. H:4 over USB where both ACL
> data and HCI events share a single "pipe").
> 
So can we convince the bluetooth adapter to speak H4? So that we add a
quirk to use H4 for certain bluetooth adapters?

A quick grep in the kernel does not show up an easy possibility to do
so.

Maybe a fix which would be a bit better would be to not completely
ignore SMP_CMD_IDENT_ADDR_INFO (where the kernel has no doubt that it
is encrypted) without SMP_CMD_IDENT_INFO, so userspace
would not get a pairing timeout but instead a pairing without irk but
with ltk?

Regards,
Andreas

Re: [PATCH RFC] bluetooth: fix paring problems with 0cf3:0036 and certain devices

[Marcel Holtmann]

Hi Andreas,

>>> This is clearly a RFC patch, things should be fixed in a cleaner
>>> way. With the bluetooth usb adapter 0cf3:0036 (Atheros), there are
>>> pairing problems in combination with certain remove devices. The
>>> cause is that smp_resume_cb() gets called after SMP_CMD_IDENT_INFO
>>> arrives in the end of a SC pairing with numerical comparison. so
>>> that there are unexpected packet messages in dmesg and although an
>>> ltk is known, bluetoothd considers the device as unpaired and not
>>> even the ltk is not stored.
>>> 
>>> The patch makes the smp code be more tolerant with the order but
>>> that does not feel like a safe thing to do.
>>> Maybe the pairing result should be given back to userspace without
>>> the irk related stuff.  
>> 
>> This sounds like the well known race between ACL data and HCI events
>> that's inherent with the way the USB HCI transport is designed. So
>> what's happening is that to the host it looks like some packets were
>> delivered over an unencrypted connection even though they in reality
>> came over an encrypted one. 
> 
> That matches what I thought. But then the question arises whether this
> race also can happen the oppositie way, so that the host thinks it gets
> an encrypted packet but that is really an unencrypted one? Ugly.
> 
> But interrupt endpoints are designed for correct timing, so there
> might be something rotten in the adapter.
> 
>> Not enforcing the encrypted connection
>> requirement is not really a solution since it creates a security
>> vulnerabilty. The best option (as far as I know) is to consider
>> switching to some other transport (e.g. H:4 over USB where both ACL
>> data and HCI events share a single "pipe").
>> 
> So can we convince the bluetooth adapter to speak H4? So that we add a
> quirk to use H4 for certain bluetooth adapters?
> 
> A quick grep in the kernel does not show up an easy possibility to do
> so.
> 
> Maybe a fix which would be a bit better would be to not completely
> ignore SMP_CMD_IDENT_ADDR_INFO (where the kernel has no doubt that it
> is encrypted) without SMP_CMD_IDENT_INFO, so userspace
> would not get a pairing timeout but instead a pairing without irk but
> with ltk?

I doubt we can do anything about this. The firmware needs to do this correctly. Can you send a btmon trace when this happens and we can see if this is really the case. If the encryption changed signal comes after the packet, then it is the issue.

Regards

Marcel

Re: [PATCH RFC] bluetooth: fix paring problems with 0cf3:0036 and certain devices

[Andreas Kemnade]

Hi,

On Fri, 15 Jun 2018 11:01:26 +0200
Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Andreas,
> 
> >>> This is clearly a RFC patch, things should be fixed in a cleaner
> >>> way. With the bluetooth usb adapter 0cf3:0036 (Atheros), there are
> >>> pairing problems in combination with certain remove devices. The
> >>> cause is that smp_resume_cb() gets called after SMP_CMD_IDENT_INFO
> >>> arrives in the end of a SC pairing with numerical comparison. so
> >>> that there are unexpected packet messages in dmesg and although an
> >>> ltk is known, bluetoothd considers the device as unpaired and not
> >>> even the ltk is not stored.
> >>> 
> >>> The patch makes the smp code be more tolerant with the order but
> >>> that does not feel like a safe thing to do.
> >>> Maybe the pairing result should be given back to userspace without
> >>> the irk related stuff.    
> >> 
> >> This sounds like the well known race between ACL data and HCI events
> >> that's inherent with the way the USB HCI transport is designed. So
> >> what's happening is that to the host it looks like some packets were
> >> delivered over an unencrypted connection even though they in reality
> >> came over an encrypted one.   
> > 
> > That matches what I thought. But then the question arises whether this
> > race also can happen the oppositie way, so that the host thinks it gets
> > an encrypted packet but that is really an unencrypted one? Ugly.
> > 
> > But interrupt endpoints are designed for correct timing, so there
> > might be something rotten in the adapter.
> >   
> >> Not enforcing the encrypted connection
> >> requirement is not really a solution since it creates a security
> >> vulnerabilty. The best option (as far as I know) is to consider
> >> switching to some other transport (e.g. H:4 over USB where both ACL
> >> data and HCI events share a single "pipe").
> >>   
> > So can we convince the bluetooth adapter to speak H4? So that we add a
> > quirk to use H4 for certain bluetooth adapters?
> > 
> > A quick grep in the kernel does not show up an easy possibility to do
> > so.
> > 
> > Maybe a fix which would be a bit better would be to not completely
> > ignore SMP_CMD_IDENT_ADDR_INFO (where the kernel has no doubt that it
> > is encrypted) without SMP_CMD_IDENT_INFO, so userspace
> > would not get a pairing timeout but instead a pairing without irk but
> > with ltk?  
> 
> I doubt we can do anything about this. The firmware needs to do this correctly. Can you send a btmon trace when this happens and we can see if this is really the case. If the encryption changed signal comes after the packet, then it is the issue.
> 
Firmware = the non-free ar3k/ramps_something? 
I also checked with wireshark via usbmon. The order stays  the same.
But I am still wondering whether the ltk should be dropped in such cases.
Here is a dump made with hcidump.


< ACL data: handle 32 flags 0x00 dlen 11
    SMP: Pairing Request (0x01)
      capability 0x04 oob 0x00 auth req 0x2d
      max key size 0x10 init key dist 0x0d resp key dist 0x0f
      Capability: KeyboardDisplay (OOB data not present)
      Authentication: Bonding (MITM Protection)
      Initiator Key Distribution:  LTK  CSRK
      Responder Key Distribution:  LTK IRK CSRK
> HCI Event: Number of Completed Packets (0x13) plen 5
    handle 32 packets 1
> ACL data: handle 32 flags 0x02 dlen 11
    SMP: Pairing Response (0x02)
      capability 0x01 oob 0x00 auth req 0x0d
      max key size 0x10 init key dist 0x01 resp key dist 0x03
      Capability: DisplayYesNo (OOB data not present)
      Authentication: Bonding (MITM Protection)
      Initiator Key Distribution:  LTK  
      Responder Key Distribution:  LTK IRK 
< ACL data: handle 32 flags 0x00 dlen 27
< ACL data: handle 32 flags 0x01 dlen 27
< ACL data: handle 32 flags 0x01 dlen 15
    SMP: Unknown (0x0c)
> HCI Event: Number of Completed Packets (0x13) plen 5
    handle 32 packets 1
> HCI Event: Number of Completed Packets (0x13) plen 5
    handle 32 packets 1
> HCI Event: Number of Completed Packets (0x13) plen 5
    handle 32 packets 1
> ACL data: handle 32 flags 0x02 dlen 27
> ACL data: handle 32 flags 0x01 dlen 27
> ACL data: handle 32 flags 0x01 dlen 15
    SMP: Unknown (0x0c)
> ACL data: handle 32 flags 0x02 dlen 21
    SMP: Pairing Confirm (0x03)
      key aaf619a4d97c2a6a21a3e7222ca7a6b0
< ACL data: handle 32 flags 0x00 dlen 21
    SMP: Pairing Random (0x04)
      random 4016a68507abba70795e3ed19457098e
> HCI Event: Number of Completed Packets (0x13) plen 5
    handle 32 packets 1
> ACL data: handle 32 flags 0x02 dlen 21
    SMP: Pairing Random (0x04)
      random 343c0995e5eee8b4818d5037109d9cff
< ACL data: handle 32 flags 0x00 dlen 21
    SMP: Unknown (0x0d)
> HCI Event: Number of Completed Packets (0x13) plen 5
    handle 32 packets 1
> ACL data: handle 32 flags 0x02 dlen 21
    SMP: Unknown (0x0d)
< HCI Command: LE Start Encryption (0x08|0x0019) plen 28
> HCI Event: Command Status (0x0f) plen 4
    LE Start Encryption (0x08|0x0019) status 0x00 ncmd 1
> ACL data: handle 32 flags 0x02 dlen 21
    SMP: Identity Information (0x08)
      IRK 13860fdc8293df86e7cc9b699632650c
> HCI Event: Encrypt Change (0x08) plen 4
    status 0x00 handle 32 encrypt 0x01
> ACL data: handle 32 flags 0x02 dlen 12
    SMP: Identity Address Information (0x09)
      bdaddr 56:1D:34:F5:24:2C (Public)

Regards,
Andreas