Skip to content

bluetooth: Fix L2CAP CoC response code if LTK is present #40177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 10, 2021
Merged

bluetooth: Fix L2CAP CoC response code if LTK is present #40177

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 39 additions & 7 deletions subsys/bluetooth/host/l2cap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "hci_core.h"
#include "conn_internal.h"
#include "l2cap_internal.h"
#include "keys.h"

#define LE_CHAN_RTX(_w) CONTAINER_OF(_w, struct bt_l2cap_le_chan, chan.rtx_work)
#define CHAN_RX(_w) CONTAINER_OF(_w, struct bt_l2cap_le_chan, rx_work)
Expand Down Expand Up @@ -1032,14 +1033,44 @@ static uint16_t l2cap_chan_accept(struct bt_conn *conn,
return BT_L2CAP_LE_SUCCESS;
}

static bool l2cap_check_security(struct bt_conn *conn,
static uint16_t l2cap_check_security(struct bt_conn *conn,
struct bt_l2cap_server *server)
{
const struct bt_keys *keys = bt_keys_find_addr(conn->id, &conn->le.dst);
bool ltk_present;

if (IS_ENABLED(CONFIG_BT_CONN_DISABLE_SECURITY)) {
return true;
return BT_L2CAP_LE_SUCCESS;
}

if (conn->sec_level >= server->sec_level) {
return BT_L2CAP_LE_SUCCESS;
}

if (conn->sec_level > BT_SECURITY_L1) {
return BT_L2CAP_LE_ERR_AUTHENTICATION;
}
Comment on lines +1050 to +1052
Copy link
Contributor

@Thalley Thalley Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I follow this statement.

If e.g. conn->sec_level is BT_SECURITY_L3 and server->sec_level is BT_SECURITY_L4 this will return with BT_L2CAP_LE_ERR_AUTHENTICATION, but couldn't it just as likely fail with BT_L2CAP_LE_ERR_KEY_SIZE?

Copy link
Contributor Author

@sjanc sjanc Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I get the code correctly, key size is verified by server user in accept callback

Copy link
Contributor

@Thalley Thalley Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then that will leave this function to assume that a specific security check is done elsewhere. It's just a matter of responding with the right error code, so it's not a huge deal, but I do think that this check could possibly (should?) be expanded to check the key in the specific case that if current security is L3 and required security is L4, that we check if the the key size is correct or not.

Copy link
Contributor Author

@sjanc sjanc Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are referring to L4 requiring 16 key size than this is check by update_sec_level(), ie connection security will not be set to L4 if keysize is not max

Copy link
Contributor

@Thalley Thalley Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am referring to that, but in the case where the ACL connection is only L3 and the L2CAP connection requires L4.

Copy link
Contributor

@Thalley Thalley Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving check from application provided accept context into this check could have potentially affect application behavior (and thus API). This is going to be backported into LTS branch and aims at fixing qualification, not reworking L2CAP API.

I'm worried we may be talking past each other. If the application supplies BT_SECURITY_L4 as the required security, why shouldn't that be checked in the same way that BT_SECURITY_L3 is?

Copy link
Contributor Author

@sjanc sjanc Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that said, I'm not sure why it was designed that way, and I'm fine with moving check into stack, just in separate PR

Copy link
Contributor Author

@sjanc sjanc Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving check from application provided accept context into this check could have potentially affect application behavior (and thus API). This is going to be backported into LTS branch and aims at fixing qualification, not reworking L2CAP API.

I'm worried we may be talking past each other. If the application supplies BT_SECURITY_L4 as the required security, why shouldn't that be checked in the same way that BT_SECURITY_L3 is?

but it is checked the same way (or maybe I'm misunderstanding what is the problem here)

Copy link
Contributor

@Thalley Thalley Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding that in a separate PR would be fine with me, but could you please add a TODO here so that we don't completely forget it?

Copy link
Contributor Author

@sjanc sjanc Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created issue for this #40216


if (keys) {
if (conn->role == BT_HCI_ROLE_CENTRAL) {
ltk_present = keys->id & (BT_KEYS_LTK_P256 | BT_KEYS_PERIPH_LTK);
} else {
ltk_present = keys->id & (BT_KEYS_LTK_P256 | BT_KEYS_LTK);
}
} else {
ltk_present = false;
}

/* If an LTK or an STK is available and encryption is required
* (LE security mode 1) but encryption is not enabled, the
* service request shall be rejected with the error code
* "Insufficient Encryption".
*/
if (ltk_present) {
return BT_L2CAP_LE_ERR_ENCRYPTION;
}

return conn->sec_level >= server->sec_level;
return BT_L2CAP_LE_ERR_AUTHENTICATION;
}

static void le_conn_req(struct bt_l2cap *l2cap, uint8_t ident,
Expand Down Expand Up @@ -1090,8 +1121,9 @@ static void le_conn_req(struct bt_l2cap *l2cap, uint8_t ident,
}

/* Check if connection has minimum required security level */
if (!l2cap_check_security(conn, server)) {
rsp->result = sys_cpu_to_le16(BT_L2CAP_LE_ERR_AUTHENTICATION);
result = l2cap_check_security(conn, server);
if (result != BT_L2CAP_LE_SUCCESS) {
rsp->result = sys_cpu_to_le16(result);
goto rsp;
}

Expand Down Expand Up @@ -1170,8 +1202,8 @@ static void le_ecred_conn_req(struct bt_l2cap *l2cap, uint8_t ident,
}

/* Check if connection has minimum required security level */
if (!l2cap_check_security(conn, server)) {
result = BT_L2CAP_LE_ERR_AUTHENTICATION;
result = l2cap_check_security(conn, server);
if (result != BT_L2CAP_LE_SUCCESS) {
goto response;
}

Expand Down