Skip to content

Conversation

@sjanc
Copy link
Contributor

@sjanc sjanc commented Nov 8, 2021

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".

This is affecting L2CAP/LE/CFC/BV-25-C and L2CAP/ECFC/BV-32-C
qualification test cases.

Signed-off-by: Szymon Janc [email protected]

@github-actions github-actions bot added area: Bluetooth area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Nov 8, 2021
@sjanc sjanc added this to the v2.7.1 milestone Nov 8, 2021
Comment on lines +1047 to +1052
Copy link
Contributor

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

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

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

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

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

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

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

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

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

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 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".

This is affecting L2CAP/LE/CFC/BV-25-C and L2CAP/ECFC/BV-32-C
qualification test cases.

Signed-off-by: Szymon Janc <[email protected]>
@cfriedt cfriedt merged commit 556736d into zephyrproject-rtos:main Nov 10, 2021
@sjanc sjanc deleted the l2cap_insuf_enc branch March 31, 2023 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants