linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] Bluetooth: Check for encryption key size on connect
@ 2020-09-21  7:55 Archie Pusaka
  2020-09-21 17:15 ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 5+ messages in thread
From: Archie Pusaka @ 2020-09-21  7:55 UTC (permalink / raw)
  To: linux-bluetooth, Marcel Holtmann
  Cc: CrosBT Upstreaming, Archie Pusaka, Alain Michaud,
	David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel,
	netdev

From: Archie Pusaka <apusaka@chromium.org>

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 (0x0000)
        Status: No further information available (0x0000)

////// 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 (0x0000)

Signed-off-by: Archie Pusaka <apusaka@chromium.org>
Reviewed-by: Alain Michaud <alainm@chromium.org>

---
Btw, it looks like the patch sent by Alex Lu with the title
[PATCH] Bluetooth: Fix the vulnerable issue on enc key size
also solves the exact same issue.

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

 net/bluetooth/l2cap_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ade83e224567..b4fc0ad38aaa 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4101,7 +4101,8 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
 
 	/* Check if the ACL is secure enough (if not SDP) */
 	if (psm != cpu_to_le16(L2CAP_PSM_SDP) &&
-	    !hci_conn_check_link_mode(conn->hcon)) {
+	    (!hci_conn_check_link_mode(conn->hcon) ||
+	    !l2cap_check_enc_key_size(conn->hcon))) {
 		conn->disc_reason = HCI_ERROR_AUTH_FAILURE;
 		result = L2CAP_CR_SEC_BLOCK;
 		goto response;
-- 
2.28.0.681.g6f77f65b4e-goog


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

* Re: [PATCH v2] Bluetooth: Check for encryption key size on connect
  2020-09-21  7:55 [PATCH v2] Bluetooth: Check for encryption key size on connect Archie Pusaka
@ 2020-09-21 17:15 ` Luiz Augusto von Dentz
  2020-09-22  7:47   ` Archie Pusaka
  0 siblings, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2020-09-21 17:15 UTC (permalink / raw)
  To: Archie Pusaka
  Cc: linux-bluetooth, Marcel Holtmann, CrosBT Upstreaming,
	Archie Pusaka, Alain Michaud, David S. Miller, Jakub Kicinski,
	Johan Hedberg, Linux Kernel Mailing List,
	open list:NETWORKING [GENERAL]

Hi Archie,


On Mon, Sep 21, 2020 at 12:56 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> 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 (0x0000)
>         Status: No further information available (0x0000)
>
> ////// 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 (0x0000)
>
> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> Reviewed-by: Alain Michaud <alainm@chromium.org>
>
> ---
> Btw, it looks like the patch sent by Alex Lu with the title
> [PATCH] Bluetooth: Fix the vulnerable issue on enc key size
> also solves the exact same issue.
>
> Changes in v2:
> * Add btmon trace to the commit message
>
>  net/bluetooth/l2cap_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ade83e224567..b4fc0ad38aaa 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -4101,7 +4101,8 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
>
>         /* Check if the ACL is secure enough (if not SDP) */
>         if (psm != cpu_to_le16(L2CAP_PSM_SDP) &&
> -           !hci_conn_check_link_mode(conn->hcon)) {
> +           (!hci_conn_check_link_mode(conn->hcon) ||
> +           !l2cap_check_enc_key_size(conn->hcon))) {

I wonder if we couldn't incorporate the check of key size into
hci_conn_check_link_mode, like I said in the first patch checking the
enc key size should not be specific to L2CAP.

>                 conn->disc_reason = HCI_ERROR_AUTH_FAILURE;
>                 result = L2CAP_CR_SEC_BLOCK;
>                 goto response;
> --
> 2.28.0.681.g6f77f65b4e-goog
>


--
Luiz Augusto von Dentz

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

* Re: [PATCH v2] Bluetooth: Check for encryption key size on connect
  2020-09-21 17:15 ` Luiz Augusto von Dentz
@ 2020-09-22  7:47   ` Archie Pusaka
  2020-09-22 17:03     ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 5+ messages in thread
From: Archie Pusaka @ 2020-09-22  7:47 UTC (permalink / raw)
  To: Luiz Augusto von Dentz
  Cc: linux-bluetooth, Marcel Holtmann, CrosBT Upstreaming,
	Archie Pusaka, Alain Michaud, David S. Miller, Jakub Kicinski,
	Johan Hedberg, Linux Kernel Mailing List,
	open list:NETWORKING [GENERAL]

Hi Luiz,

On Tue, 22 Sep 2020 at 01:15, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
>
> On Mon, Sep 21, 2020 at 12:56 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > 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 (0x0000)
> >         Status: No further information available (0x0000)
> >
> > ////// 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 (0x0000)
> >
> > Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> > Reviewed-by: Alain Michaud <alainm@chromium.org>
> >
> > ---
> > Btw, it looks like the patch sent by Alex Lu with the title
> > [PATCH] Bluetooth: Fix the vulnerable issue on enc key size
> > also solves the exact same issue.
> >
> > Changes in v2:
> > * Add btmon trace to the commit message
> >
> >  net/bluetooth/l2cap_core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index ade83e224567..b4fc0ad38aaa 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -4101,7 +4101,8 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
> >
> >         /* Check if the ACL is secure enough (if not SDP) */
> >         if (psm != cpu_to_le16(L2CAP_PSM_SDP) &&
> > -           !hci_conn_check_link_mode(conn->hcon)) {
> > +           (!hci_conn_check_link_mode(conn->hcon) ||
> > +           !l2cap_check_enc_key_size(conn->hcon))) {
>
> I wonder if we couldn't incorporate the check of key size into
> hci_conn_check_link_mode, like I said in the first patch checking the
> enc key size should not be specific to L2CAP.

Yes, I could move the check into hci_conn_check_link_mode.
At first look, this function is also called by AMP which I am not
familiar with. In addition, I found this patch which moves this check
outside hci_conn, so I have my doubts there.
https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=693cd8ce3f882524a5d06f7800dd8492411877b3

>
> >                 conn->disc_reason = HCI_ERROR_AUTH_FAILURE;
> >                 result = L2CAP_CR_SEC_BLOCK;
> >                 goto response;
> > --
> > 2.28.0.681.g6f77f65b4e-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie

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

* Re: [PATCH v2] Bluetooth: Check for encryption key size on connect
  2020-09-22  7:47   ` Archie Pusaka
@ 2020-09-22 17:03     ` Luiz Augusto von Dentz
  2020-09-25 12:03       ` Archie Pusaka
  0 siblings, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2020-09-22 17:03 UTC (permalink / raw)
  To: Archie Pusaka
  Cc: linux-bluetooth, Marcel Holtmann, CrosBT Upstreaming,
	Archie Pusaka, Alain Michaud, David S. Miller, Jakub Kicinski,
	Johan Hedberg, Linux Kernel Mailing List,
	open list:NETWORKING [GENERAL]

Hi Archie,

On Tue, Sep 22, 2020 at 12:48 AM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
>
> On Tue, 22 Sep 2020 at 01:15, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> >
> > On Mon, Sep 21, 2020 at 12:56 AM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > From: Archie Pusaka <apusaka@chromium.org>
> > >
> > > 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 (0x0000)
> > >         Status: No further information available (0x0000)
> > >
> > > ////// 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 (0x0000)
> > >
> > > Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> > > Reviewed-by: Alain Michaud <alainm@chromium.org>
> > >
> > > ---
> > > Btw, it looks like the patch sent by Alex Lu with the title
> > > [PATCH] Bluetooth: Fix the vulnerable issue on enc key size
> > > also solves the exact same issue.
> > >
> > > Changes in v2:
> > > * Add btmon trace to the commit message
> > >
> > >  net/bluetooth/l2cap_core.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > > index ade83e224567..b4fc0ad38aaa 100644
> > > --- a/net/bluetooth/l2cap_core.c
> > > +++ b/net/bluetooth/l2cap_core.c
> > > @@ -4101,7 +4101,8 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
> > >
> > >         /* Check if the ACL is secure enough (if not SDP) */
> > >         if (psm != cpu_to_le16(L2CAP_PSM_SDP) &&
> > > -           !hci_conn_check_link_mode(conn->hcon)) {
> > > +           (!hci_conn_check_link_mode(conn->hcon) ||
> > > +           !l2cap_check_enc_key_size(conn->hcon))) {
> >
> > I wonder if we couldn't incorporate the check of key size into
> > hci_conn_check_link_mode, like I said in the first patch checking the
> > enc key size should not be specific to L2CAP.
>
> Yes, I could move the check into hci_conn_check_link_mode.
> At first look, this function is also called by AMP which I am not
> familiar with. In addition, I found this patch which moves this check
> outside hci_conn, so I have my doubts there.
> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=693cd8ce3f882524a5d06f7800dd8492411877b3

Right, I think we can have it as part of the hci_conn_check_link_mode,
that said it is perhaps better to have it as
hci_conn_check_enc_key_size instead as it is not L2CAP expecific.
Other than that it looks good to me.

> >
> > >                 conn->disc_reason = HCI_ERROR_AUTH_FAILURE;
> > >                 result = L2CAP_CR_SEC_BLOCK;
> > >                 goto response;
> > > --
> > > 2.28.0.681.g6f77f65b4e-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Archie



-- 
Luiz Augusto von Dentz

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

* Re: [PATCH v2] Bluetooth: Check for encryption key size on connect
  2020-09-22 17:03     ` Luiz Augusto von Dentz
@ 2020-09-25 12:03       ` Archie Pusaka
  0 siblings, 0 replies; 5+ messages in thread
From: Archie Pusaka @ 2020-09-25 12:03 UTC (permalink / raw)
  To: Luiz Augusto von Dentz
  Cc: linux-bluetooth, Marcel Holtmann, CrosBT Upstreaming,
	Archie Pusaka, Alain Michaud, David S. Miller, Jakub Kicinski,
	Johan Hedberg, Linux Kernel Mailing List,
	open list:NETWORKING [GENERAL]

Hi Luiz,

On Wed, 23 Sep 2020 at 01:03, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Tue, Sep 22, 2020 at 12:48 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > Hi Luiz,
> >
> > On Tue, 22 Sep 2020 at 01:15, Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Archie,
> > >
> > >
> > > On Mon, Sep 21, 2020 at 12:56 AM Archie Pusaka <apusaka@google.com> wrote:
> > > >
> > > > From: Archie Pusaka <apusaka@chromium.org>
> > > >
> > > > 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 (0x0000)
> > > >         Status: No further information available (0x0000)
> > > >
> > > > ////// 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 (0x0000)
> > > >
> > > > Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> > > > Reviewed-by: Alain Michaud <alainm@chromium.org>
> > > >
> > > > ---
> > > > Btw, it looks like the patch sent by Alex Lu with the title
> > > > [PATCH] Bluetooth: Fix the vulnerable issue on enc key size
> > > > also solves the exact same issue.
> > > >
> > > > Changes in v2:
> > > > * Add btmon trace to the commit message
> > > >
> > > >  net/bluetooth/l2cap_core.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > > > index ade83e224567..b4fc0ad38aaa 100644
> > > > --- a/net/bluetooth/l2cap_core.c
> > > > +++ b/net/bluetooth/l2cap_core.c
> > > > @@ -4101,7 +4101,8 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
> > > >
> > > >         /* Check if the ACL is secure enough (if not SDP) */
> > > >         if (psm != cpu_to_le16(L2CAP_PSM_SDP) &&
> > > > -           !hci_conn_check_link_mode(conn->hcon)) {
> > > > +           (!hci_conn_check_link_mode(conn->hcon) ||
> > > > +           !l2cap_check_enc_key_size(conn->hcon))) {
> > >
> > > I wonder if we couldn't incorporate the check of key size into
> > > hci_conn_check_link_mode, like I said in the first patch checking the
> > > enc key size should not be specific to L2CAP.
> >
> > Yes, I could move the check into hci_conn_check_link_mode.
> > At first look, this function is also called by AMP which I am not
> > familiar with. In addition, I found this patch which moves this check
> > outside hci_conn, so I have my doubts there.
> > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=693cd8ce3f882524a5d06f7800dd8492411877b3
>
> Right, I think we can have it as part of the hci_conn_check_link_mode,
> that said it is perhaps better to have it as
> hci_conn_check_enc_key_size instead as it is not L2CAP expecific.
> Other than that it looks good to me.

Do you mean we should move l2cap_conn_check_enc_key_size to
hci_conn_check_enc_key_size? I think that is a good idea.
We also have hci_conn_check_secure which I am unsure what the purpose
is. I'll try to merge them together.

>
> > >
> > > >                 conn->disc_reason = HCI_ERROR_AUTH_FAILURE;
> > > >                 result = L2CAP_CR_SEC_BLOCK;
> > > >                 goto response;
> > > > --
> > > > 2.28.0.681.g6f77f65b4e-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > Thanks,
> > Archie
>
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie

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

end of thread, other threads:[~2020-09-25 12:03 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-21  7:55 [PATCH v2] Bluetooth: Check for encryption key size on connect Archie Pusaka
2020-09-21 17:15 ` Luiz Augusto von Dentz
2020-09-22  7:47   ` Archie Pusaka
2020-09-22 17:03     ` Luiz Augusto von Dentz
2020-09-25 12:03       ` Archie Pusaka

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).