Re: [PATCH v3] Bluetooth: Check for encryption key size on connect

2020-10-04 Thread Archie Pusaka
[Re-sending in plain text]
Hi Marcel,

I tried Alex's patch and it works for me as well.

Thanks,
Archie


On Thu, 1 Oct 2020 at 15:14, Marcel Holtmann  wrote:
>
> Hi Archie,
>
> > When receiving connection, we only check whether the link has been
> > encrypted, but not the encryption key size of the link.
> >
> > This patch adds check for encryption key size, and reject L2CAP
> > connection which size is below the specified threshold (default 7)
> > with security block.
> >
> > Here is some btmon trace.
> > @ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 
> > 5.847722
> >Store hint: No (0x00)
> >BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
> >Key type: Unauthenticated Combination key from P-192 (0x04)
> >Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
> >PIN length: 0
> >> HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 
> >> 5.871537
> >Status: Success (0x00)
> >Handle: 256
> >Encryption: Enabled with E0 (0x01)
> > < HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 
> > 5.871609
> >Handle: 256
> >> HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 
> >> 5.872524
> >  Read Encryption Key Size (0x05|0x0008) ncmd 1
> >Status: Success (0x00)
> >Handle: 256
> >Key size: 3
> >
> > // WITHOUT PATCH //
> >> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 
> >> 5.895023
> >  L2CAP: Connection Request (0x02) ident 3 len 4
> >PSM: 4097 (0x1001)
> >Source CID: 64
> > < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 
> > 5.895213
> >  L2CAP: Connection Response (0x03) ident 3 len 8
> >Destination CID: 64
> >Source CID: 64
> >Result: Connection successful (0x)
> >Status: No further information available (0x)
> >
> > // WITH PATCH //
> >> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 
> >> 4.887024
> >  L2CAP: Connection Request (0x02) ident 3 len 4
> >PSM: 4097 (0x1001)
> >Source CID: 64
> > < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 
> > 4.887127
> >  L2CAP: Connection Response (0x03) ident 3 len 8
> >Destination CID: 0
> >Source CID: 64
> >Result: Connection refused - security block (0x0003)
> >Status: No further information available (0x)
> >
> > Signed-off-by: Archie Pusaka 
> >
> > ---
> >
> > Changes in v3:
> > * Move the check to hci_conn_check_link_mode()
> >
> > Changes in v2:
> > * Add btmon trace to the commit message
> >
> > net/bluetooth/hci_conn.c | 4 
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 9832f8445d43..89085fac797c 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn 
> > *conn)
> >   !test_bit(HCI_CONN_ENCRYPT, >flags))
> >   return 0;
> >
> > + if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
> > + conn->enc_key_size < conn->hdev->min_enc_key_size)
> > + return 0;
> > +
> >   return 1;
> > }
> 
>  I am a bit concerned since we had that check and I on purpose moved 
>  it. See commit 693cd8ce3f88 for the change where I removed and 
>  commit d5bb334a8e17 where I initially added it.
> 
>  Naively adding the check in that location caused a major regression 
>  with Bluetooth 2.0 devices. This makes me a bit reluctant to re-add 
>  it here since I restructured the whole change to check the key size 
>  a different location.
> >>>
> >>> I have tried this patch (both v2 and v3) to connect with a Bluetooth
> >>> 2.0 device, it doesn't have any connection problem.
> >>> I suppose because in the original patch (d5bb334a8e17), there is no
> >>> check for the HCI_CONN_ENCRYPT flag.
> >>
> >> while that might be the case, I am still super careful. Especially 
> >> also in conjunction with the email / patch from Alex trying to add 
> >> just another encryption key size check. If we really need them or even 
> >> both, we have to audit the whole code since I must have clearly missed 
> >> something when adding the KNOB fix.
> >>
>  Now I have to ask, are you running an 

Re: [PATCH v3] Bluetooth: Check for encryption key size on connect

2020-10-01 Thread Marcel Holtmann
Hi Archie,

> When receiving connection, we only check whether the link has been
> encrypted, but not the encryption key size of the link.
> 
> This patch adds check for encryption key size, and reject L2CAP
> connection which size is below the specified threshold (default 7)
> with security block.
> 
> Here is some btmon trace.
> @ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 
> 5.847722
>Store hint: No (0x00)
>BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
>Key type: Unauthenticated Combination key from P-192 (0x04)
>Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
>PIN length: 0
>> HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 5.871537
>Status: Success (0x00)
>Handle: 256
>Encryption: Enabled with E0 (0x01)
> < HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 
> 5.871609
>Handle: 256
>> HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 5.872524
>  Read Encryption Key Size (0x05|0x0008) ncmd 1
>Status: Success (0x00)
>Handle: 256
>Key size: 3
> 
> // WITHOUT PATCH //
>> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 5.895023
>  L2CAP: Connection Request (0x02) ident 3 len 4
>PSM: 4097 (0x1001)
>Source CID: 64
> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 
> 5.895213
>  L2CAP: Connection Response (0x03) ident 3 len 8
>Destination CID: 64
>Source CID: 64
>Result: Connection successful (0x)
>Status: No further information available (0x)
> 
> // WITH PATCH //
>> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 4.887024
>  L2CAP: Connection Request (0x02) ident 3 len 4
>PSM: 4097 (0x1001)
>Source CID: 64
> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 
> 4.887127
>  L2CAP: Connection Response (0x03) ident 3 len 8
>Destination CID: 0
>Source CID: 64
>Result: Connection refused - security block (0x0003)
>Status: No further information available (0x)
> 
> Signed-off-by: Archie Pusaka 
> 
> ---
> 
> Changes in v3:
> * Move the check to hci_conn_check_link_mode()
> 
> Changes in v2:
> * Add btmon trace to the commit message
> 
> net/bluetooth/hci_conn.c | 4 
> 1 file changed, 4 insertions(+)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 9832f8445d43..89085fac797c 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn 
> *conn)
>   !test_bit(HCI_CONN_ENCRYPT, >flags))
>   return 0;
> 
> + if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
> + conn->enc_key_size < conn->hdev->min_enc_key_size)
> + return 0;
> +
>   return 1;
> }
 
 I am a bit concerned since we had that check and I on purpose moved 
 it. See commit 693cd8ce3f88 for the change where I removed and commit 
 d5bb334a8e17 where I initially added it.
 
 Naively adding the check in that location caused a major regression 
 with Bluetooth 2.0 devices. This makes me a bit reluctant to re-add it 
 here since I restructured the whole change to check the key size a 
 different location.
>>> 
>>> I have tried this patch (both v2 and v3) to connect with a Bluetooth
>>> 2.0 device, it doesn't have any connection problem.
>>> I suppose because in the original patch (d5bb334a8e17), there is no
>>> check for the HCI_CONN_ENCRYPT flag.
>> 
>> while that might be the case, I am still super careful. Especially also 
>> in conjunction with the email / patch from Alex trying to add just 
>> another encryption key size check. If we really need them or even both, 
>> we have to audit the whole code since I must have clearly missed 
>> something when adding the KNOB fix.
>> 
 Now I have to ask, are you running an upstream kernel with both 
 commits above that address KNOB vulnerability?
>>> 
>>> Actually no, I haven't heard of KNOB vulnerability before.
>>> This patch is written for qualification purposes, specifically to pass
>>> GAP/SEC/SEM/BI-05-C to BI-08-C.
>>> However, it sounds like it could also prevent some KNOB vulnerability
>>> as a bonus.
>> 
>> That part worries me since 

Re: [PATCH v3] Bluetooth: Check for encryption key size on connect

2020-09-29 Thread Archie Pusaka
Hi Marcel,

On Tue, 29 Sep 2020 at 14:21, Marcel Holtmann  wrote:
>
> Hi Archie,
>
> >>> When receiving connection, we only check whether the link has been
> >>> encrypted, but not the encryption key size of the link.
> >>>
> >>> This patch adds check for encryption key size, and reject L2CAP
> >>> connection which size is below the specified threshold (default 7)
> >>> with security block.
> >>>
> >>> Here is some btmon trace.
> >>> @ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 
> >>> 5.847722
> >>> Store hint: No (0x00)
> >>> BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
> >>> Key type: Unauthenticated Combination key from P-192 (0x04)
> >>> Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
> >>> PIN length: 0
>  HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 5.871537
> >>> Status: Success (0x00)
> >>> Handle: 256
> >>> Encryption: Enabled with E0 (0x01)
> >>> < HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 
> >>> 5.871609
> >>> Handle: 256
>  HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 5.872524
> >>>   Read Encryption Key Size (0x05|0x0008) ncmd 1
> >>> Status: Success (0x00)
> >>> Handle: 256
> >>> Key size: 3
> >>>
> >>> // WITHOUT PATCH //
>  ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 5.895023
> >>>   L2CAP: Connection Request (0x02) ident 3 len 4
> >>> PSM: 4097 (0x1001)
> >>> Source CID: 64
> >>> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 
> >>> 5.895213
> >>>   L2CAP: Connection Response (0x03) ident 3 len 8
> >>> Destination CID: 64
> >>> Source CID: 64
> >>> Result: Connection successful (0x)
> >>> Status: No further information available (0x)
> >>>
> >>> // WITH PATCH //
>  ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 4.887024
> >>>   L2CAP: Connection Request (0x02) ident 3 len 4
> >>> PSM: 4097 (0x1001)
> >>> Source CID: 64
> >>> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 
> >>> 4.887127
> >>>   L2CAP: Connection Response (0x03) ident 3 len 8
> >>> Destination CID: 0
> >>> Source CID: 64
> >>> Result: Connection refused - security block (0x0003)
> >>> Status: No further information available (0x)
> >>>
> >>> Signed-off-by: Archie Pusaka 
> >>>
> >>> ---
> >>>
> >>> Changes in v3:
> >>> * Move the check to hci_conn_check_link_mode()
> >>>
> >>> Changes in v2:
> >>> * Add btmon trace to the commit message
> >>>
> >>> net/bluetooth/hci_conn.c | 4 
> >>> 1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> >>> index 9832f8445d43..89085fac797c 100644
> >>> --- a/net/bluetooth/hci_conn.c
> >>> +++ b/net/bluetooth/hci_conn.c
> >>> @@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn 
> >>> *conn)
> >>>!test_bit(HCI_CONN_ENCRYPT, >flags))
> >>>return 0;
> >>>
> >>> + if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
> >>> + conn->enc_key_size < conn->hdev->min_enc_key_size)
> >>> + return 0;
> >>> +
> >>>return 1;
> >>> }
> >>
> >> I am a bit concerned since we had that check and I on purpose moved 
> >> it. See commit 693cd8ce3f88 for the change where I removed and commit 
> >> d5bb334a8e17 where I initially added it.
> >>
> >> Naively adding the check in that location caused a major regression 
> >> with Bluetooth 2.0 devices. This makes me a bit reluctant to re-add it 
> >> here since I restructured the whole change to check the key size a 
> >> different location.
> >
> > I have tried this patch (both v2 and v3) to connect with a Bluetooth
> > 2.0 device, it doesn't have any connection problem.
> > I suppose because in the original patch (d5bb334a8e17), there is no
> > check for the HCI_CONN_ENCRYPT flag.
> 
>  while that might be the case, I am still super careful. Especially also 
>  in conjunction with the email / patch from Alex trying to add just 
>  another encryption key size check. If we really need them or even both, 
>  we have to audit the whole code since I must have clearly missed 
>  something when adding the KNOB fix.
> 
> >> Now I have to ask, are you running an upstream kernel with both 
> >> commits above that address KNOB vulnerability?
> >
> > Actually no, I haven't heard of KNOB vulnerability before.
> > This patch is written for qualification purposes, specifically to pass
> > GAP/SEC/SEM/BI-05-C to BI-08-C.
> > However, it sounds like it could also prevent 

Re: [PATCH v3] Bluetooth: Check for encryption key size on connect

2020-09-29 Thread Marcel Holtmann
Hi Archie,

>>> When receiving connection, we only check whether the link has been
>>> encrypted, but not the encryption key size of the link.
>>> 
>>> This patch adds check for encryption key size, and reject L2CAP
>>> connection which size is below the specified threshold (default 7)
>>> with security block.
>>> 
>>> Here is some btmon trace.
>>> @ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 5.847722
>>> Store hint: No (0x00)
>>> BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
>>> Key type: Unauthenticated Combination key from P-192 (0x04)
>>> Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
>>> PIN length: 0
 HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 5.871537
>>> Status: Success (0x00)
>>> Handle: 256
>>> Encryption: Enabled with E0 (0x01)
>>> < HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 5.871609
>>> Handle: 256
 HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 5.872524
>>>   Read Encryption Key Size (0x05|0x0008) ncmd 1
>>> Status: Success (0x00)
>>> Handle: 256
>>> Key size: 3
>>> 
>>> // WITHOUT PATCH //
 ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 5.895023
>>>   L2CAP: Connection Request (0x02) ident 3 len 4
>>> PSM: 4097 (0x1001)
>>> Source CID: 64
>>> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 5.895213
>>>   L2CAP: Connection Response (0x03) ident 3 len 8
>>> Destination CID: 64
>>> Source CID: 64
>>> Result: Connection successful (0x)
>>> Status: No further information available (0x)
>>> 
>>> // WITH PATCH //
 ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 4.887024
>>>   L2CAP: Connection Request (0x02) ident 3 len 4
>>> PSM: 4097 (0x1001)
>>> Source CID: 64
>>> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 4.887127
>>>   L2CAP: Connection Response (0x03) ident 3 len 8
>>> Destination CID: 0
>>> Source CID: 64
>>> Result: Connection refused - security block (0x0003)
>>> Status: No further information available (0x)
>>> 
>>> Signed-off-by: Archie Pusaka 
>>> 
>>> ---
>>> 
>>> Changes in v3:
>>> * Move the check to hci_conn_check_link_mode()
>>> 
>>> Changes in v2:
>>> * Add btmon trace to the commit message
>>> 
>>> net/bluetooth/hci_conn.c | 4 
>>> 1 file changed, 4 insertions(+)
>>> 
>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>>> index 9832f8445d43..89085fac797c 100644
>>> --- a/net/bluetooth/hci_conn.c
>>> +++ b/net/bluetooth/hci_conn.c
>>> @@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn 
>>> *conn)
>>>!test_bit(HCI_CONN_ENCRYPT, >flags))
>>>return 0;
>>> 
>>> + if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
>>> + conn->enc_key_size < conn->hdev->min_enc_key_size)
>>> + return 0;
>>> +
>>>return 1;
>>> }
>> 
>> I am a bit concerned since we had that check and I on purpose moved it. 
>> See commit 693cd8ce3f88 for the change where I removed and commit 
>> d5bb334a8e17 where I initially added it.
>> 
>> Naively adding the check in that location caused a major regression with 
>> Bluetooth 2.0 devices. This makes me a bit reluctant to re-add it here 
>> since I restructured the whole change to check the key size a different 
>> location.
> 
> I have tried this patch (both v2 and v3) to connect with a Bluetooth
> 2.0 device, it doesn't have any connection problem.
> I suppose because in the original patch (d5bb334a8e17), there is no
> check for the HCI_CONN_ENCRYPT flag.
 
 while that might be the case, I am still super careful. Especially also in 
 conjunction with the email / patch from Alex trying to add just another 
 encryption key size check. If we really need them or even both, we have to 
 audit the whole code since I must have clearly missed something when 
 adding the KNOB fix.
 
>> Now I have to ask, are you running an upstream kernel with both commits 
>> above that address KNOB vulnerability?
> 
> Actually no, I haven't heard of KNOB vulnerability before.
> This patch is written for qualification purposes, specifically to pass
> GAP/SEC/SEM/BI-05-C to BI-08-C.
> However, it sounds like it could also prevent some KNOB vulnerability
> as a bonus.
 
 That part worries me since there should be no gaps that allows an 
 encryption key size downgrade if our side supports Read Encryption Key 
 Size.
 
 We really have to ensure that any L2CAP communication is stalled until we 
 have all 

Re: [PATCH v3] Bluetooth: Check for encryption key size on connect

2020-09-28 Thread Archie Pusaka
Hi Marcel,

On Tue, 29 Sep 2020 at 00:43, Marcel Holtmann  wrote:
>
> Hi Archie,
>
> > When receiving connection, we only check whether the link has been
> > encrypted, but not the encryption key size of the link.
> >
> > This patch adds check for encryption key size, and reject L2CAP
> > connection which size is below the specified threshold (default 7)
> > with security block.
> >
> > Here is some btmon trace.
> > @ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 5.847722
> >  Store hint: No (0x00)
> >  BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
> >  Key type: Unauthenticated Combination key from P-192 (0x04)
> >  Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
> >  PIN length: 0
> >> HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 5.871537
> >  Status: Success (0x00)
> >  Handle: 256
> >  Encryption: Enabled with E0 (0x01)
> > < HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 5.871609
> >  Handle: 256
> >> HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 5.872524
> >Read Encryption Key Size (0x05|0x0008) ncmd 1
> >  Status: Success (0x00)
> >  Handle: 256
> >  Key size: 3
> >
> > // WITHOUT PATCH //
> >> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 5.895023
> >L2CAP: Connection Request (0x02) ident 3 len 4
> >  PSM: 4097 (0x1001)
> >  Source CID: 64
> > < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 5.895213
> >L2CAP: Connection Response (0x03) ident 3 len 8
> >  Destination CID: 64
> >  Source CID: 64
> >  Result: Connection successful (0x)
> >  Status: No further information available (0x)
> >
> > // WITH PATCH //
> >> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 4.887024
> >L2CAP: Connection Request (0x02) ident 3 len 4
> >  PSM: 4097 (0x1001)
> >  Source CID: 64
> > < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 4.887127
> >L2CAP: Connection Response (0x03) ident 3 len 8
> >  Destination CID: 0
> >  Source CID: 64
> >  Result: Connection refused - security block (0x0003)
> >  Status: No further information available (0x)
> >
> > Signed-off-by: Archie Pusaka 
> >
> > ---
> >
> > Changes in v3:
> > * Move the check to hci_conn_check_link_mode()
> >
> > Changes in v2:
> > * Add btmon trace to the commit message
> >
> > net/bluetooth/hci_conn.c | 4 
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 9832f8445d43..89085fac797c 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn 
> > *conn)
> > !test_bit(HCI_CONN_ENCRYPT, >flags))
> > return 0;
> >
> > + if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
> > + conn->enc_key_size < conn->hdev->min_enc_key_size)
> > + return 0;
> > +
> > return 1;
> > }
> 
>  I am a bit concerned since we had that check and I on purpose moved it. 
>  See commit 693cd8ce3f88 for the change where I removed and commit 
>  d5bb334a8e17 where I initially added it.
> 
>  Naively adding the check in that location caused a major regression with 
>  Bluetooth 2.0 devices. This makes me a bit reluctant to re-add it here 
>  since I restructured the whole change to check the key size a different 
>  location.
> >>>
> >>> I have tried this patch (both v2 and v3) to connect with a Bluetooth
> >>> 2.0 device, it doesn't have any connection problem.
> >>> I suppose because in the original patch (d5bb334a8e17), there is no
> >>> check for the HCI_CONN_ENCRYPT flag.
> >>
> >> while that might be the case, I am still super careful. Especially also in 
> >> conjunction with the email / patch from Alex trying to add just another 
> >> encryption key size check. If we really need them or even both, we have to 
> >> audit the whole code since I must have clearly missed something when 
> >> adding the KNOB fix.
> >>
>  Now I have to ask, are you running an upstream kernel with both commits 
>  above that address KNOB vulnerability?
> >>>
> >>> Actually no, I haven't heard of KNOB vulnerability before.
> >>> This patch is written for qualification purposes, specifically to pass
> >>> GAP/SEC/SEM/BI-05-C to BI-08-C.
> >>> However, it sounds like it could also prevent some KNOB vulnerability
> >>> as a bonus.
> >>
> >> That part worries me since there should be no gaps that allows an 
> >> encryption key size downgrade if our side supports Read Encryption Key 
> >> Size.
> >>
> >> We really 

Re: [PATCH v3] Bluetooth: Check for encryption key size on connect

2020-09-28 Thread Marcel Holtmann
Hi Archie,

> When receiving connection, we only check whether the link has been
> encrypted, but not the encryption key size of the link.
> 
> This patch adds check for encryption key size, and reject L2CAP
> connection which size is below the specified threshold (default 7)
> with security block.
> 
> Here is some btmon trace.
> @ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 5.847722
>  Store hint: No (0x00)
>  BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
>  Key type: Unauthenticated Combination key from P-192 (0x04)
>  Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
>  PIN length: 0
>> HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 5.871537
>  Status: Success (0x00)
>  Handle: 256
>  Encryption: Enabled with E0 (0x01)
> < HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 5.871609
>  Handle: 256
>> HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 5.872524
>Read Encryption Key Size (0x05|0x0008) ncmd 1
>  Status: Success (0x00)
>  Handle: 256
>  Key size: 3
> 
> // WITHOUT PATCH //
>> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 5.895023
>L2CAP: Connection Request (0x02) ident 3 len 4
>  PSM: 4097 (0x1001)
>  Source CID: 64
> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 5.895213
>L2CAP: Connection Response (0x03) ident 3 len 8
>  Destination CID: 64
>  Source CID: 64
>  Result: Connection successful (0x)
>  Status: No further information available (0x)
> 
> // WITH PATCH //
>> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 4.887024
>L2CAP: Connection Request (0x02) ident 3 len 4
>  PSM: 4097 (0x1001)
>  Source CID: 64
> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 4.887127
>L2CAP: Connection Response (0x03) ident 3 len 8
>  Destination CID: 0
>  Source CID: 64
>  Result: Connection refused - security block (0x0003)
>  Status: No further information available (0x)
> 
> Signed-off-by: Archie Pusaka 
> 
> ---
> 
> Changes in v3:
> * Move the check to hci_conn_check_link_mode()
> 
> Changes in v2:
> * Add btmon trace to the commit message
> 
> net/bluetooth/hci_conn.c | 4 
> 1 file changed, 4 insertions(+)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 9832f8445d43..89085fac797c 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn *conn)
> !test_bit(HCI_CONN_ENCRYPT, >flags))
> return 0;
> 
> + if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
> + conn->enc_key_size < conn->hdev->min_enc_key_size)
> + return 0;
> +
> return 1;
> }
 
 I am a bit concerned since we had that check and I on purpose moved it. 
 See commit 693cd8ce3f88 for the change where I removed and commit 
 d5bb334a8e17 where I initially added it.
 
 Naively adding the check in that location caused a major regression with 
 Bluetooth 2.0 devices. This makes me a bit reluctant to re-add it here 
 since I restructured the whole change to check the key size a different 
 location.
>>> 
>>> I have tried this patch (both v2 and v3) to connect with a Bluetooth
>>> 2.0 device, it doesn't have any connection problem.
>>> I suppose because in the original patch (d5bb334a8e17), there is no
>>> check for the HCI_CONN_ENCRYPT flag.
>> 
>> while that might be the case, I am still super careful. Especially also in 
>> conjunction with the email / patch from Alex trying to add just another 
>> encryption key size check. If we really need them or even both, we have to 
>> audit the whole code since I must have clearly missed something when adding 
>> the KNOB fix.
>> 
 Now I have to ask, are you running an upstream kernel with both commits 
 above that address KNOB vulnerability?
>>> 
>>> Actually no, I haven't heard of KNOB vulnerability before.
>>> This patch is written for qualification purposes, specifically to pass
>>> GAP/SEC/SEM/BI-05-C to BI-08-C.
>>> However, it sounds like it could also prevent some KNOB vulnerability
>>> as a bonus.
>> 
>> That part worries me since there should be no gaps that allows an encryption 
>> key size downgrade if our side supports Read Encryption Key Size.
>> 
>> We really have to ensure that any L2CAP communication is stalled until we 
>> have all information from HCI connection setup that we need. So maybe the 
>> change Alex did would work as well, or as I mentioned put any L2CAP 
>> connection request as pending so that the validation happens in one 

Re: [PATCH v3] Bluetooth: Check for encryption key size on connect

2020-09-28 Thread Archie Pusaka
Hi Marcel,

On Sun, 27 Sep 2020 at 20:09, Marcel Holtmann  wrote:
>
> Hi Archie,
>
> >>> When receiving connection, we only check whether the link has been
> >>> encrypted, but not the encryption key size of the link.
> >>>
> >>> This patch adds check for encryption key size, and reject L2CAP
> >>> connection which size is below the specified threshold (default 7)
> >>> with security block.
> >>>
> >>> Here is some btmon trace.
> >>> @ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 5.847722
> >>>   Store hint: No (0x00)
> >>>   BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
> >>>   Key type: Unauthenticated Combination key from P-192 (0x04)
> >>>   Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
> >>>   PIN length: 0
>  HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 5.871537
> >>>   Status: Success (0x00)
> >>>   Handle: 256
> >>>   Encryption: Enabled with E0 (0x01)
> >>> < HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 5.871609
> >>>   Handle: 256
>  HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 5.872524
> >>> Read Encryption Key Size (0x05|0x0008) ncmd 1
> >>>   Status: Success (0x00)
> >>>   Handle: 256
> >>>   Key size: 3
> >>>
> >>> // WITHOUT PATCH //
>  ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 5.895023
> >>> L2CAP: Connection Request (0x02) ident 3 len 4
> >>>   PSM: 4097 (0x1001)
> >>>   Source CID: 64
> >>> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 5.895213
> >>> L2CAP: Connection Response (0x03) ident 3 len 8
> >>>   Destination CID: 64
> >>>   Source CID: 64
> >>>   Result: Connection successful (0x)
> >>>   Status: No further information available (0x)
> >>>
> >>> // WITH PATCH //
>  ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 4.887024
> >>> L2CAP: Connection Request (0x02) ident 3 len 4
> >>>   PSM: 4097 (0x1001)
> >>>   Source CID: 64
> >>> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 4.887127
> >>> L2CAP: Connection Response (0x03) ident 3 len 8
> >>>   Destination CID: 0
> >>>   Source CID: 64
> >>>   Result: Connection refused - security block (0x0003)
> >>>   Status: No further information available (0x)
> >>>
> >>> Signed-off-by: Archie Pusaka 
> >>>
> >>> ---
> >>>
> >>> Changes in v3:
> >>> * Move the check to hci_conn_check_link_mode()
> >>>
> >>> Changes in v2:
> >>> * Add btmon trace to the commit message
> >>>
> >>> net/bluetooth/hci_conn.c | 4 
> >>> 1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> >>> index 9832f8445d43..89085fac797c 100644
> >>> --- a/net/bluetooth/hci_conn.c
> >>> +++ b/net/bluetooth/hci_conn.c
> >>> @@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn *conn)
> >>>  !test_bit(HCI_CONN_ENCRYPT, >flags))
> >>>  return 0;
> >>>
> >>> + if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
> >>> + conn->enc_key_size < conn->hdev->min_enc_key_size)
> >>> + return 0;
> >>> +
> >>>  return 1;
> >>> }
> >>
> >> I am a bit concerned since we had that check and I on purpose moved it. 
> >> See commit 693cd8ce3f88 for the change where I removed and commit 
> >> d5bb334a8e17 where I initially added it.
> >>
> >> Naively adding the check in that location caused a major regression with 
> >> Bluetooth 2.0 devices. This makes me a bit reluctant to re-add it here 
> >> since I restructured the whole change to check the key size a different 
> >> location.
> >
> > I have tried this patch (both v2 and v3) to connect with a Bluetooth
> > 2.0 device, it doesn't have any connection problem.
> > I suppose because in the original patch (d5bb334a8e17), there is no
> > check for the HCI_CONN_ENCRYPT flag.
>
> while that might be the case, I am still super careful. Especially also in 
> conjunction with the email / patch from Alex trying to add just another 
> encryption key size check. If we really need them or even both, we have to 
> audit the whole code since I must have clearly missed something when adding 
> the KNOB fix.
>
> >> Now I have to ask, are you running an upstream kernel with both commits 
> >> above that address KNOB vulnerability?
> >
> > Actually no, I haven't heard of KNOB vulnerability before.
> > This patch is written for qualification purposes, specifically to pass
> > GAP/SEC/SEM/BI-05-C to BI-08-C.
> > However, it sounds like it could also prevent some KNOB vulnerability
> > as a bonus.
>
> That part worries me since there should be no gaps that allows an encryption 
> key size downgrade if our side supports Read Encryption Key Size.
>
> We really have to ensure that any L2CAP communication is stalled until we 
> have all information from HCI connection setup that we need. So maybe the 
> change Alex did would work as well, or as I mentioned put any L2CAP 
> 

Re: [PATCH v3] Bluetooth: Check for encryption key size on connect

2020-09-27 Thread Marcel Holtmann
Hi Archie,

>>> When receiving connection, we only check whether the link has been
>>> encrypted, but not the encryption key size of the link.
>>> 
>>> This patch adds check for encryption key size, and reject L2CAP
>>> connection which size is below the specified threshold (default 7)
>>> with security block.
>>> 
>>> Here is some btmon trace.
>>> @ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 5.847722
>>>   Store hint: No (0x00)
>>>   BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
>>>   Key type: Unauthenticated Combination key from P-192 (0x04)
>>>   Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
>>>   PIN length: 0
 HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 5.871537
>>>   Status: Success (0x00)
>>>   Handle: 256
>>>   Encryption: Enabled with E0 (0x01)
>>> < HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 5.871609
>>>   Handle: 256
 HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 5.872524
>>> Read Encryption Key Size (0x05|0x0008) ncmd 1
>>>   Status: Success (0x00)
>>>   Handle: 256
>>>   Key size: 3
>>> 
>>> // WITHOUT PATCH //
 ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 5.895023
>>> L2CAP: Connection Request (0x02) ident 3 len 4
>>>   PSM: 4097 (0x1001)
>>>   Source CID: 64
>>> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 5.895213
>>> L2CAP: Connection Response (0x03) ident 3 len 8
>>>   Destination CID: 64
>>>   Source CID: 64
>>>   Result: Connection successful (0x)
>>>   Status: No further information available (0x)
>>> 
>>> // WITH PATCH //
 ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 4.887024
>>> L2CAP: Connection Request (0x02) ident 3 len 4
>>>   PSM: 4097 (0x1001)
>>>   Source CID: 64
>>> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 4.887127
>>> L2CAP: Connection Response (0x03) ident 3 len 8
>>>   Destination CID: 0
>>>   Source CID: 64
>>>   Result: Connection refused - security block (0x0003)
>>>   Status: No further information available (0x)
>>> 
>>> Signed-off-by: Archie Pusaka 
>>> 
>>> ---
>>> 
>>> Changes in v3:
>>> * Move the check to hci_conn_check_link_mode()
>>> 
>>> Changes in v2:
>>> * Add btmon trace to the commit message
>>> 
>>> net/bluetooth/hci_conn.c | 4 
>>> 1 file changed, 4 insertions(+)
>>> 
>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>>> index 9832f8445d43..89085fac797c 100644
>>> --- a/net/bluetooth/hci_conn.c
>>> +++ b/net/bluetooth/hci_conn.c
>>> @@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn *conn)
>>>  !test_bit(HCI_CONN_ENCRYPT, >flags))
>>>  return 0;
>>> 
>>> + if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
>>> + conn->enc_key_size < conn->hdev->min_enc_key_size)
>>> + return 0;
>>> +
>>>  return 1;
>>> }
>> 
>> I am a bit concerned since we had that check and I on purpose moved it. See 
>> commit 693cd8ce3f88 for the change where I removed and commit d5bb334a8e17 
>> where I initially added it.
>> 
>> Naively adding the check in that location caused a major regression with 
>> Bluetooth 2.0 devices. This makes me a bit reluctant to re-add it here since 
>> I restructured the whole change to check the key size a different location.
> 
> I have tried this patch (both v2 and v3) to connect with a Bluetooth
> 2.0 device, it doesn't have any connection problem.
> I suppose because in the original patch (d5bb334a8e17), there is no
> check for the HCI_CONN_ENCRYPT flag.

while that might be the case, I am still super careful. Especially also in 
conjunction with the email / patch from Alex trying to add just another 
encryption key size check. If we really need them or even both, we have to 
audit the whole code since I must have clearly missed something when adding the 
KNOB fix.

>> Now I have to ask, are you running an upstream kernel with both commits 
>> above that address KNOB vulnerability?
> 
> Actually no, I haven't heard of KNOB vulnerability before.
> This patch is written for qualification purposes, specifically to pass
> GAP/SEC/SEM/BI-05-C to BI-08-C.
> However, it sounds like it could also prevent some KNOB vulnerability
> as a bonus.

That part worries me since there should be no gaps that allows an encryption 
key size downgrade if our side supports Read Encryption Key Size.

We really have to ensure that any L2CAP communication is stalled until we have 
all information from HCI connection setup that we need. So maybe the change 
Alex did would work as well, or as I mentioned put any L2CAP connection request 
as pending so that the validation happens in one place.

Regards

Marcel



Re: [PATCH v3] Bluetooth: Check for encryption key size on connect

2020-09-26 Thread Archie Pusaka
Hi Marcel,

On Sat, 26 Sep 2020 at 00:37, Marcel Holtmann  wrote:
>
> Hi Archie,
>
> > When receiving connection, we only check whether the link has been
> > encrypted, but not the encryption key size of the link.
> >
> > This patch adds check for encryption key size, and reject L2CAP
> > connection which size is below the specified threshold (default 7)
> > with security block.
> >
> > Here is some btmon trace.
> > @ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 5.847722
> >Store hint: No (0x00)
> >BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
> >Key type: Unauthenticated Combination key from P-192 (0x04)
> >Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
> >PIN length: 0
> >> HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 5.871537
> >Status: Success (0x00)
> >Handle: 256
> >Encryption: Enabled with E0 (0x01)
> > < HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 5.871609
> >Handle: 256
> >> HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 5.872524
> >  Read Encryption Key Size (0x05|0x0008) ncmd 1
> >Status: Success (0x00)
> >Handle: 256
> >Key size: 3
> >
> > // WITHOUT PATCH //
> >> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 5.895023
> >  L2CAP: Connection Request (0x02) ident 3 len 4
> >PSM: 4097 (0x1001)
> >Source CID: 64
> > < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 5.895213
> >  L2CAP: Connection Response (0x03) ident 3 len 8
> >Destination CID: 64
> >Source CID: 64
> >Result: Connection successful (0x)
> >Status: No further information available (0x)
> >
> > // WITH PATCH //
> >> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 4.887024
> >  L2CAP: Connection Request (0x02) ident 3 len 4
> >PSM: 4097 (0x1001)
> >Source CID: 64
> > < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 4.887127
> >  L2CAP: Connection Response (0x03) ident 3 len 8
> >Destination CID: 0
> >Source CID: 64
> >Result: Connection refused - security block (0x0003)
> >Status: No further information available (0x)
> >
> > Signed-off-by: Archie Pusaka 
> >
> > ---
> >
> > Changes in v3:
> > * Move the check to hci_conn_check_link_mode()
> >
> > Changes in v2:
> > * Add btmon trace to the commit message
> >
> > net/bluetooth/hci_conn.c | 4 
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 9832f8445d43..89085fac797c 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn *conn)
> >   !test_bit(HCI_CONN_ENCRYPT, >flags))
> >   return 0;
> >
> > + if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
> > + conn->enc_key_size < conn->hdev->min_enc_key_size)
> > + return 0;
> > +
> >   return 1;
> > }
>
> I am a bit concerned since we had that check and I on purpose moved it. See 
> commit 693cd8ce3f88 for the change where I removed and commit d5bb334a8e17 
> where I initially added it.
>
> Naively adding the check in that location caused a major regression with 
> Bluetooth 2.0 devices. This makes me a bit reluctant to re-add it here since 
> I restructured the whole change to check the key size a different location.

I have tried this patch (both v2 and v3) to connect with a Bluetooth
2.0 device, it doesn't have any connection problem.
I suppose because in the original patch (d5bb334a8e17), there is no
check for the HCI_CONN_ENCRYPT flag.

>
> Now I have to ask, are you running an upstream kernel with both commits above 
> that address KNOB vulnerability?

Actually no, I haven't heard of KNOB vulnerability before.
This patch is written for qualification purposes, specifically to pass
GAP/SEC/SEM/BI-05-C to BI-08-C.
However, it sounds like it could also prevent some KNOB vulnerability
as a bonus.
>
> Regards
>
> Marcel
>

Thanks,
Archie


Re: [PATCH v3] Bluetooth: Check for encryption key size on connect

2020-09-25 Thread Marcel Holtmann
Hi Archie,

> When receiving connection, we only check whether the link has been
> encrypted, but not the encryption key size of the link.
> 
> This patch adds check for encryption key size, and reject L2CAP
> connection which size is below the specified threshold (default 7)
> with security block.
> 
> Here is some btmon trace.
> @ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 5.847722
>Store hint: No (0x00)
>BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
>Key type: Unauthenticated Combination key from P-192 (0x04)
>Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
>PIN length: 0
>> HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 5.871537
>Status: Success (0x00)
>Handle: 256
>Encryption: Enabled with E0 (0x01)
> < HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 5.871609
>Handle: 256
>> HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 5.872524
>  Read Encryption Key Size (0x05|0x0008) ncmd 1
>Status: Success (0x00)
>Handle: 256
>Key size: 3
> 
> // WITHOUT PATCH //
>> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 5.895023
>  L2CAP: Connection Request (0x02) ident 3 len 4
>PSM: 4097 (0x1001)
>Source CID: 64
> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 5.895213
>  L2CAP: Connection Response (0x03) ident 3 len 8
>Destination CID: 64
>Source CID: 64
>Result: Connection successful (0x)
>Status: No further information available (0x)
> 
> // WITH PATCH //
>> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 4.887024
>  L2CAP: Connection Request (0x02) ident 3 len 4
>PSM: 4097 (0x1001)
>Source CID: 64
> < ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 4.887127
>  L2CAP: Connection Response (0x03) ident 3 len 8
>Destination CID: 0
>Source CID: 64
>Result: Connection refused - security block (0x0003)
>Status: No further information available (0x)
> 
> Signed-off-by: Archie Pusaka 
> 
> ---
> 
> Changes in v3:
> * Move the check to hci_conn_check_link_mode()
> 
> Changes in v2:
> * Add btmon trace to the commit message
> 
> net/bluetooth/hci_conn.c | 4 
> 1 file changed, 4 insertions(+)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 9832f8445d43..89085fac797c 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn *conn)
>   !test_bit(HCI_CONN_ENCRYPT, >flags))
>   return 0;
> 
> + if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
> + conn->enc_key_size < conn->hdev->min_enc_key_size)
> + return 0;
> +
>   return 1;
> }

I am a bit concerned since we had that check and I on purpose moved it. See 
commit 693cd8ce3f88 for the change where I removed and commit d5bb334a8e17 
where I initially added it.

Naively adding the check in that location caused a major regression with 
Bluetooth 2.0 devices. This makes me a bit reluctant to re-add it here since I 
restructured the whole change to check the key size a different location.

Now I have to ask, are you running an upstream kernel with both commits above 
that address KNOB vulnerability?

Regards

Marcel



[PATCH v3] Bluetooth: Check for encryption key size on connect

2020-09-22 Thread Archie Pusaka
From: Archie Pusaka 

When receiving connection, we only check whether the link has been
encrypted, but not the encryption key size of the link.

This patch adds check for encryption key size, and reject L2CAP
connection which size is below the specified threshold (default 7)
with security block.

Here is some btmon trace.
@ MGMT Event: New Link Key (0x0009) plen 26{0x0001} [hci0] 5.847722
Store hint: No (0x00)
BR/EDR Address: 38:00:25:F7:F1:B0 (OUI 38-00-25)
Key type: Unauthenticated Combination key from P-192 (0x04)
Link key: 7bf2f68c81305d63a6b0ee2c5a7a34bc
PIN length: 0
> HCI Event: Encryption Change (0x08) plen 4#29 [hci0] 5.871537
Status: Success (0x00)
Handle: 256
Encryption: Enabled with E0 (0x01)
< HCI Command: Read Encryp... (0x05|0x0008) plen 2  #30 [hci0] 5.871609
Handle: 256
> HCI Event: Command Complete (0x0e) plen 7 #31 [hci0] 5.872524
  Read Encryption Key Size (0x05|0x0008) ncmd 1
Status: Success (0x00)
Handle: 256
Key size: 3

// WITHOUT PATCH //
> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 5.895023
  L2CAP: Connection Request (0x02) ident 3 len 4
PSM: 4097 (0x1001)
Source CID: 64
< ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 5.895213
  L2CAP: Connection Response (0x03) ident 3 len 8
Destination CID: 64
Source CID: 64
Result: Connection successful (0x)
Status: No further information available (0x)

// WITH PATCH //
> ACL Data RX: Handle 256 flags 0x02 dlen 12#42 [hci0] 4.887024
  L2CAP: Connection Request (0x02) ident 3 len 4
PSM: 4097 (0x1001)
Source CID: 64
< ACL Data TX: Handle 256 flags 0x00 dlen 16#43 [hci0] 4.887127
  L2CAP: Connection Response (0x03) ident 3 len 8
Destination CID: 0
Source CID: 64
Result: Connection refused - security block (0x0003)
Status: No further information available (0x)

Signed-off-by: Archie Pusaka 

---

Changes in v3:
* Move the check to hci_conn_check_link_mode()

Changes in v2:
* Add btmon trace to the commit message

 net/bluetooth/hci_conn.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 9832f8445d43..89085fac797c 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1348,6 +1348,10 @@ int hci_conn_check_link_mode(struct hci_conn *conn)
!test_bit(HCI_CONN_ENCRYPT, >flags))
return 0;
 
+   if (test_bit(HCI_CONN_ENCRYPT, >flags) &&
+   conn->enc_key_size < conn->hdev->min_enc_key_size)
+   return 0;
+
return 1;
 }
 
-- 
2.28.0.681.g6f77f65b4e-goog