| Message ID | 20240314102002.22662-3-yuxuan.luo@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Jammy,1/1] Bluetooth: Add more enc key size check | expand |
On 3/14/24 5:20 AM, Yuxuan Luo wrote: > From: Alex Lu <alex_lu@realsil.com.cn> > > When we are slave role and receives l2cap conn req when encryption has > started, we should check the enc key size to avoid KNOB attack or BLUFFS > attack. > From SIG recommendation, implementations are advised to reject > service-level connections on an encrypted baseband link with key > strengths below 7 octets. > A simple and clear way to achieve this is to place the enc key size > check in hci_cc_read_enc_key_size() > > The btmon log below shows the case that lacks enc key size check. > >> HCI Event: Connect Request (0x04) plen 10 > Address: BB:22:33:44:55:99 (OUI BB-22-33) > Class: 0x480104 > Major class: Computer (desktop, notebook, PDA, organizers) > Minor class: Desktop workstation > Capturing (Scanner, Microphone) > Telephony (Cordless telephony, Modem, Headset) > Link type: ACL (0x01) > < HCI Command: Accept Connection Request (0x01|0x0009) plen 7 > Address: BB:22:33:44:55:99 (OUI BB-22-33) > Role: Peripheral (0x01) >> HCI Event: Command Status (0x0f) plen 4 > Accept Connection Request (0x01|0x0009) ncmd 2 > Status: Success (0x00) >> HCI Event: Connect Complete (0x03) plen 11 > Status: Success (0x00) > Handle: 1 > Address: BB:22:33:44:55:99 (OUI BB-22-33) > Link type: ACL (0x01) > Encryption: Disabled (0x00) > ... > >> HCI Event: Encryption Change (0x08) plen 4 > Status: Success (0x00) > Handle: 1 Address: BB:22:33:44:55:99 (OUI BB-22-33) > Encryption: Enabled with E0 (0x01) > < HCI Command: Read Encryption Key Size (0x05|0x0008) plen 2 > Handle: 1 Address: BB:22:33:44:55:99 (OUI BB-22-33) >> HCI Event: Command Complete (0x0e) plen 7 > Read Encryption Key Size (0x05|0x0008) ncmd 2 > Status: Success (0x00) > Handle: 1 Address: BB:22:33:44:55:99 (OUI BB-22-33) > Key size: 6 > // We should check the enc key size > ... > >> ACL Data RX: Handle 1 flags 0x02 dlen 12 > L2CAP: Connection Request (0x02) ident 3 len 4 > PSM: 25 (0x0019) > Source CID: 64 > < ACL Data TX: Handle 1 flags 0x00 dlen 16 > L2CAP: Connection Response (0x03) ident 3 len 8 > Destination CID: 64 > Source CID: 64 > Result: Connection pending (0x0001) > Status: Authorization pending (0x0002) >> HCI Event: Number of Completed Packets (0x13) plen 5 > Num handles: 1 > Handle: 1 Address: BB:22:33:44:55:99 (OUI BB-22-33) > Count: 1 > #35: len 16 (25 Kb/s) > Latency: 5 msec (2-7 msec ~4 msec) > < ACL Data TX: Handle 1 flags 0x00 dlen 16 > L2CAP: Connection Response (0x03) ident 3 len 8 > Destination CID: 64 > Source CID: 64 > Result: Connection successful (0x0000) > Status: No further information available (0x0000) > > Cc: stable@vger.kernel.org > Signed-off-by: Alex Lu <alex_lu@realsil.com.cn> > Signed-off-by: Max Chou <max.chou@realtek.com> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > (backported from commit 04a342cc49a8522e99c9b3346371c329d841dcd2) > [yuxuan.luo: manually backported. Renamed status to rp_status to avoid > name conflict with the function argument "status". > ] > CVE-2023-24023 > Signed-off-by: Yuxuan Luo <yuxuan.luo@canonical.com> > --- > net/bluetooth/hci_event.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index b1eb614f2aad3..f9072872c35a5 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -3113,6 +3113,7 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status, > const struct hci_rp_read_enc_key_size *rp; > struct hci_conn *conn; > u16 handle; > + u8 rp_status = rp->status; In both the Focal and Jammy patches, `rp->status` is read before `rp` is initialized. A fix could be to set `rp_status` only after `rp` is set a few lines below. The Focal patch is also missing a CVE number and backport note. > > BT_DBG("%s status 0x%02x", hdev->name, status); > > @@ -3134,15 +3135,30 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status, > * secure approach is to then assume the key size is 0 to force a > * disconnection. > */ > - if (rp->status) { > + if (rp_status) { > bt_dev_err(hdev, "failed to read key size for handle %u", > handle); > conn->enc_key_size = 0; > } else { > conn->enc_key_size = rp->key_size; > + rp_status = 0; > + > + if (conn->enc_key_size < hdev->min_enc_key_size) { > + /* As slave role, the conn->state has been set to > + * BT_CONNECTED and l2cap conn req might not be received > + * yet, at this moment the l2cap layer almost does > + * nothing with the non-zero status. > + * So we also clear encrypt related bits, and then the > + * handler of l2cap conn req will get the right secure > + * state at a later time. > + */ > + rp_status = HCI_ERROR_AUTH_FAILURE; > + clear_bit(HCI_CONN_ENCRYPT, &conn->flags); > + clear_bit(HCI_CONN_AES_CCM, &conn->flags); > + } > } > > - hci_encrypt_cfm(conn, 0); > + hci_encrypt_cfm(conn, rp_status); > > unlock: > hci_dev_unlock(hdev);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index b1eb614f2aad3..f9072872c35a5 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -3113,6 +3113,7 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status, const struct hci_rp_read_enc_key_size *rp; struct hci_conn *conn; u16 handle; + u8 rp_status = rp->status; BT_DBG("%s status 0x%02x", hdev->name, status); @@ -3134,15 +3135,30 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status, * secure approach is to then assume the key size is 0 to force a * disconnection. */ - if (rp->status) { + if (rp_status) { bt_dev_err(hdev, "failed to read key size for handle %u", handle); conn->enc_key_size = 0; } else { conn->enc_key_size = rp->key_size; + rp_status = 0; + + if (conn->enc_key_size < hdev->min_enc_key_size) { + /* As slave role, the conn->state has been set to + * BT_CONNECTED and l2cap conn req might not be received + * yet, at this moment the l2cap layer almost does + * nothing with the non-zero status. + * So we also clear encrypt related bits, and then the + * handler of l2cap conn req will get the right secure + * state at a later time. + */ + rp_status = HCI_ERROR_AUTH_FAILURE; + clear_bit(HCI_CONN_ENCRYPT, &conn->flags); + clear_bit(HCI_CONN_AES_CCM, &conn->flags); + } } - hci_encrypt_cfm(conn, 0); + hci_encrypt_cfm(conn, rp_status); unlock: hci_dev_unlock(hdev);