Bluetooth: att: Fix EATT channel security requirements

[MariuszSkamra]

Core Vol 3, Part G, Section 5.3.2 Channel Requirements states that
"The channel shall be encrypted". It does not mention any additional
security requirements that can be specified bt higher layer profiles.
This enables link encryption requirement for EATT channel.

Signed-off-by: Mariusz Skamra mariusz.skamra@codecoup.pl

[Thalley]

The change seems OK, but I was wondering: Why do we not just ensure that the ACL is encrypted before allowing EATT channels to be established? If we cannot use EATT while not encrypted, then there's not really any reason to even establish them.

[MariuszSkamra]

The change seems OK, but I was wondering: Why do we not just ensure that the ACL is encrypted before allowing EATT channels to be established? If we cannot use EATT while not encrypted, then there's not really any reason to even establish them.

You have right. The fix would be even cleaner, as there the check won't be evaluated on every PDU. Thanks!

[Thalley]

The change seems OK, but I was wondering: Why do we not just ensure that the ACL is encrypted before allowing EATT channels to be established? If we cannot use EATT while not encrypted, then there's not really any reason to even establish them.

You have right. The fix would be even cleaner, as there the check won't be evaluated on every PDU. Thanks!

We may want to check if this causes some IOP issues: Does the core spec say anything about encryption level before establishing EATT channels? We may experience some IOP issues if we change it to that.

The auto-EATT connection we have in Zephyr would also need to be changed from when it's connected (done in bt_att_accept now), to when the ACL has been encrypted.

@hermabe Any input here?

[Thalley]

Assuming that this won't cause IOP issues, LGTM to me

[sjanc]

#AutoPTS autotest run zephyr GATT

[hermabe]

The change seems OK, but I was wondering: Why do we not just ensure that the ACL is encrypted before allowing EATT channels to be established? If we cannot use EATT while not encrypted, then there's not really any reason to even establish them.

You have right. The fix would be even cleaner, as there the check won't be evaluated on every PDU. Thanks!

We may want to check if this causes some IOP issues: Does the core spec say anything about encryption level before establishing EATT channels? We may experience some IOP issues if we change it to that.

The auto-EATT connection we have in Zephyr would also need to be changed from when it's connected (done in bt_att_accept now), to when the ACL has been encrypted.

@hermabe Any input here?

The spec is really vague on this requirement. It only says "the channel shall be encrypted". I haven't seen any special requirement that the link shall be encrypted before the channels are connected. It may be ok to connect, but not send over EATT channels without encryption (the current implementation). In that case, we may want to allow incoming requests/etc. without encryption for IOP reasons.

Requiring the link to be encrypted before the channels are established is stricter, but should be within the spec. This is also cleaner as the peer gets an auth error code in response to the connection request.

From some brief testing with my phone (Android 12) some time ago, the phone did not check the encryption requirement and allowed notifications to be received over EATT without encryption.

[MariuszSkamra]

@hermabe

The spec is really vague on this requirement. It only says "the channel shall be encrypted". I haven't seen any special requirement that the link shall be encrypted before the channels are connected. It may be ok to connect, but not send over EATT channels without encryption (the current implementation). In that case, we may want to allow incoming requests/etc. without encryption for IOP reasons.

• I agree, it is vague. However, if you allow to connect and then reject requests with ATT Insufficient Encryption, then you are in trouble because you are against the specification that does not allow such error when finding attributes:

Core 5.3 | Vol, Part F, 3.4.9 Attribute PDU response summary

The Core is strict here:
Core 5.3 | Vol 3, Part F, 3.4.3.3 ATT_FIND_BY_TYPE_VALUE_REQ

ATT_FIND_BY_TYPE_VALUE_REQ is PDU used by GATT Discover Primary Service by Service UUID procedure.

• Moreover, GATT Discover All Primary Services procedure uses ATT_READ_BY_GROUP_TYPE_REQ PDU, which can return ATT Insufficient Encryption error, but if the attribute permissions require encryption to be enabled.

but GATT Service Declaration shall not require authentication as per Core 5.3 | Vol 3, Part G, 3.1 SERVICE DEFINITION.

Requiring the link to be encrypted before the channels are established is stricter, but should be within the spec. This is also cleaner as the peer gets an auth error code in response to the connection request.

I agree and I will create an errata to Core specification with clarification request and paste link here. However, the requirements I mentioned above imply the link to be encrypted when EATT channels are established IMO.

If we allow to create EATT channels prior encryption then we fall into vague behavior that:
Attribute X can be read over ATT, but
Attribute X cannot be read over EATT, because of Insufficient Encryption (Client does not know whether it's encryption required by EATT or Attribute X permissions).

From some brief testing with my phone (Android 12) some time ago, the phone did not check the encryption requirement and allowed notifications to be received over EATT without encryption.

Yes, this will be fixed soon, as they are working on it now.

[MariuszSkamra]

Errata: https://www.bluetooth.org/errata/errata_view.cfm?errata_id=19172

[sjanc]

#AutoPTS run zephyr GATT

[codecoup-tester]

Scheduled PR ##46982 (comment) after 16:09:00.

[codecoup-tester]

AutoPTS Bot results:
Failed tests:
GATT GATT/CL/GAR/BI-42-C INCONC
GATT GATT/CL/GAR/BV-10-C FAIL
GATT GATT/CL/GAR/BV-11-C INCONC
GATT GATT/CL/GPM/BV-11-C FAIL
GATT GATT/SR/GAN/BV-02-C INCONC
GATT GATT/SR/GAN/BV-02-C_LT2 FAIL

[MariuszSkamra]

AutoPTS Bot results: Failed tests: GATT GATT/CL/GAR/BI-42-C INCONC GATT GATT/CL/GAR/BV-10-C FAIL GATT GATT/CL/GAR/BV-11-C INCONC GATT GATT/CL/GPM/BV-11-C FAIL GATT GATT/SR/GAN/BV-02-C INCONC GATT GATT/SR/GAN/BV-02-C_LT2 FAIL

@sjanc Those test cases failed in Bluetooth test session - WW26.3 as well

[MariuszSkamra]

@hermabe I proposed to change "The channel shall be encrypted." into "The channel shall be created and used on encrypted link" https://www.bluetooth.org/errata/errata_view.cfm?errata_id=19172
That's the response I got:
"However, I don't think we need to say that the connection must be encrypted before channel creation; if that's the only way of getting an encrypted channel, then it's implicit and we don't (or shouldn't) duplicate requirements in specifications."

[Thalley]

@hermabe I proposed to change "The channel shall be encrypted." into "The channel shall be created and used on encrypted link" https://www.bluetooth.org/errata/errata_view.cfm?errata_id=19172 That's the response I got: "However, I don't think we need to say that the connection must be encrypted before channel creation; if that's the only way of getting an encrypted channel, then it's implicit and we don't (or shouldn't) duplicate requirements in specifications."

Hmm, that's a shame.

w.r.t. IOP issues, then this shouldn't give any real issues: Worst case scenario is that the EATT channels are not established, and there isn't any usecase (yet) that is "EATT only".
In the case that we (Zephyr) reject at EATT request due to lacking encryption, then we'll (Zephyr) simply just attempt to create the EATT channels once the link is encrypted, so in the end the channels will probably be created.

[hermabe]

LGTM. This approach seems cleaner than connecting EATT channels that cannot be used.