Re: [PATCH v2] Bluetooth: Check for encryption key size on connect
Hi Luiz,
On Wed, 23 Sep 2020 at 01:03, Luiz Augusto von Dentz
wrote:
>
> Hi Archie,
>
> On Tue, Sep 22, 2020 at 12:48 AM Archie Pusaka wrote:
> >
> > Hi Luiz,
> >
> > On Tue, 22 Sep 2020 at 01:15, Luiz Augusto von Dentz
> > wrote:
> > >
> > > Hi Archie,
> > >
> > >
> > > On Mon, Sep 21, 2020 at 12:56 AM Archie Pusaka wrote:
> > > >
> > > > 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
> > > > Reviewed-by: Alain Michaud
> > > >
> > > > ---
> > > > 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->di
Re: [PATCH v2] Bluetooth: Check for encryption key size on connect
Hi Archie,
On Tue, Sep 22, 2020 at 12:48 AM Archie Pusaka wrote:
>
> Hi Luiz,
>
> On Tue, 22 Sep 2020 at 01:15, Luiz Augusto von Dentz
> wrote:
> >
> > Hi Archie,
> >
> >
> > On Mon, Sep 21, 2020 at 12:56 AM Archie Pusaka wrote:
> > >
> > > 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
> > > Reviewed-by: Alain Michaud
> > >
> > > ---
> > > 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
Re: [PATCH v2] Bluetooth: Check for encryption key size on connect
Hi Luiz,
On Tue, 22 Sep 2020 at 01:15, Luiz Augusto von Dentz
wrote:
>
> Hi Archie,
>
>
> On Mon, Sep 21, 2020 at 12:56 AM Archie Pusaka wrote:
> >
> > 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
> > Reviewed-by: Alain Michaud
> >
> > ---
> > 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
Re: [PATCH v2] Bluetooth: Check for encryption key size on connect
Hi Archie,
On Mon, Sep 21, 2020 at 12:56 AM Archie Pusaka wrote:
>
> 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
> Reviewed-by: Alain Michaud
>
> ---
> 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
[PATCH v2] Bluetooth: Check for encryption key size on connect
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
Reviewed-by: Alain Michaud
---
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
