fix(ble): Fix security for Bluedroid

Author: lucasssvaz

libraries/BLE/src/BLECharacteristic.cpp

@@ -582,6 +582,34 @@ void BLECharacteristic::handleGATTServerEvent(esp_gatts_cb_event_t event, esp_ga
       // we save the new value.  Next we look at the need_rsp flag which indicates whether or not we need
       // to send a response.  If we do, then we formulate a response and send it.
       if (param->write.handle == m_handle) {
+
+        // Check for authorization requirement
+        if (m_permissions & ESP_GATT_PERM_WRITE_AUTHORIZATION) {
+          bool authorized = false;
+
+          if (BLEDevice::m_securityCallbacks != nullptr) {
+            log_i("Authorization required for write operation. Checking authorization...");
+            authorized = BLEDevice::m_securityCallbacks->onAuthorizationRequest(
+              param->write.conn_id, m_handle, false
+            );
+          } else {
+            log_w("onAuthorizationRequest not implemented. Rejecting write authorization request");
+          }
+
+          if (!authorized) {
+            log_i("Write authorization rejected");
+            if (param->write.need_rsp) {
+              esp_err_t errRc = ::esp_ble_gatts_send_response(gatts_if, param->write.conn_id, param->write.trans_id, ESP_GATT_INSUF_AUTHORIZATION, nullptr);
+              if (errRc != ESP_OK) {
+                log_e("esp_ble_gatts_send_response (authorization failed): rc=%d %s", errRc, GeneralUtils::errorToString(errRc));
+              }
+            }
+            return;  // Exit early, don't process the write
+          } else {
+            log_i("Write authorization granted");
+          }
+        }
+
         if (param->write.is_prep) {
           m_value.addPart(param->write.value, param->write.len);
           m_writeEvt = true;
@@ -637,6 +665,33 @@ void BLECharacteristic::handleGATTServerEvent(esp_gatts_cb_event_t event, esp_ga
     {
       if (param->read.handle == m_handle) {
 
+        // Check for authorization requirement
+        if (m_permissions & ESP_GATT_PERM_READ_AUTHORIZATION) {
+          bool authorized = false;
+
+          if (BLEDevice::m_securityCallbacks != nullptr) {
+            log_i("Authorization required for read operation. Checking authorization...");
+            authorized = BLEDevice::m_securityCallbacks->onAuthorizationRequest(
+              param->read.conn_id, m_handle, true
+            );
+          } else {
+            log_w("onAuthorizationRequest not implemented. Rejecting read authorization request");
+          }
+
+          if (!authorized) {
+            log_i("Read authorization rejected");
+            if (param->read.need_rsp) {
+              esp_err_t errRc = ::esp_ble_gatts_send_response(gatts_if, param->read.conn_id, param->read.trans_id, ESP_GATT_INSUF_AUTHORIZATION, nullptr);
+              if (errRc != ESP_OK) {
+                log_e("esp_ble_gatts_send_response (authorization failed): rc=%d %s", errRc, GeneralUtils::errorToString(errRc));
+              }
+            }
+            return;  // Exit early, don't process the read
+          } else {
+            log_i("Read authorization granted");
+          }
+        }
+
         // Here's an interesting thing.  The read request has the option of saying whether we need a response
         // or not.  What would it "mean" to receive a read request and NOT send a response back?  That feels like
         // a very strange read.

libraries/BLE/src/BLECharacteristic.h

@@ -140,17 +140,25 @@ class BLECharacteristic {
 
 #if defined(CONFIG_BLUEDROID_ENABLED)
   static const uint32_t PROPERTY_READ = 1 << 0;
+  static const uint32_t PROPERTY_READ_ENC = 0; // Not supported by Bluedroid. Use setAccessPermissions() instead.
+  static const uint32_t PROPERTY_READ_AUTHEN = 0; // Not supported by Bluedroid. Use setAccessPermissions() instead.
+  static const uint32_t PROPERTY_READ_AUTHOR = 0; // Not supported by Bluedroid. Use setAccessPermissions() instead.
   static const uint32_t PROPERTY_WRITE = 1 << 1;
+  static const uint32_t PROPERTY_WRITE_NR = 1 << 5;
+  static const uint32_t PROPERTY_WRITE_ENC = 0; // Not supported by Bluedroid. Use setAccessPermissions() instead.
+  static const uint32_t PROPERTY_WRITE_AUTHEN = 0; // Not supported by Bluedroid. Use setAccessPermissions() instead.
+  static const uint32_t PROPERTY_WRITE_AUTHOR = 0; // Not supported by Bluedroid. Use setAccessPermissions() instead.
   static const uint32_t PROPERTY_NOTIFY = 1 << 2;
   static const uint32_t PROPERTY_BROADCAST = 1 << 3;
   static const uint32_t PROPERTY_INDICATE = 1 << 4;
-  static const uint32_t PROPERTY_WRITE_NR = 1 << 5;
 #endif
 
   /***************************************************************************
    *                     NimBLE public properties                            *
    ***************************************************************************/
 
+
+
 #if defined(CONFIG_NIMBLE_ENABLED)
   static const uint32_t PROPERTY_READ = BLE_GATT_CHR_F_READ;
   static const uint32_t PROPERTY_READ_ENC = BLE_GATT_CHR_F_READ_ENC;
@@ -161,8 +169,8 @@ class BLECharacteristic {
   static const uint32_t PROPERTY_WRITE_ENC = BLE_GATT_CHR_F_WRITE_ENC;
   static const uint32_t PROPERTY_WRITE_AUTHEN = BLE_GATT_CHR_F_WRITE_AUTHEN;
   static const uint32_t PROPERTY_WRITE_AUTHOR = BLE_GATT_CHR_F_WRITE_AUTHOR;
-  static const uint32_t PROPERTY_BROADCAST = BLE_GATT_CHR_F_BROADCAST;
   static const uint32_t PROPERTY_NOTIFY = BLE_GATT_CHR_F_NOTIFY;
+  static const uint32_t PROPERTY_BROADCAST = BLE_GATT_CHR_F_BROADCAST;
   static const uint32_t PROPERTY_INDICATE = BLE_GATT_CHR_F_INDICATE;
 #endif
 

libraries/BLE/src/BLEClient.cpp

@@ -19,6 +19,7 @@
  *                           Common includes                               *
  ***************************************************************************/
 
+#include "Arduino.h"
 #include <esp_bt.h>
 #include "BLEClient.h"
 #include "BLEUtils.h"
@@ -29,6 +30,7 @@
 #include <unordered_set>
 #include "BLEDevice.h"
 #include "esp32-hal-log.h"
+#include "BLESecurity.h"
 
 /***************************************************************************
  *                           Bluedroid includes                            *
@@ -598,11 +600,11 @@ void BLEClient::gattClientEventHandler(esp_gattc_cb_event_t event, esp_gatt_if_t
       if (errRc != ESP_OK) {
         log_e("esp_ble_gattc_send_mtu_req: rc=%d %s", errRc, GeneralUtils::errorToString(errRc));
       }
-#ifdef CONFIG_BLE_SMP_ENABLE  // Check that BLE SMP (security) is configured in make menuconfig
-      if (BLEDevice::m_securityLevel) {
-        esp_ble_set_encryption(evtParam->connect.remote_bda, BLEDevice::m_securityLevel);
+      // Set encryption on connect for BlueDroid when security is enabled
+      // This ensures security is established before any secure operations
+      if (BLESecurity::m_securityEnabled && BLESecurity::m_forceSecurity) {
+        BLESecurity::startSecurity(evtParam->connect.remote_bda);
       }
-#endif  // CONFIG_BLE_SMP_ENABLE
       break;
     }  // ESP_GATTC_CONNECT_EVT
 
@@ -1006,6 +1008,10 @@ int BLEClient::handleGAPEvent(struct ble_gap_event *event, void *arg) {
           break;
         }
 
+        if(BLESecurity::m_securityEnabled) {
+          BLESecurity::startSecurity(client->m_conn_id);
+        }
+
         // In the case of a multiconnecting device we ignore this device when
         // scanning since we are already connected to it
         // BLEDevice::addIgnored(client->m_peerAddress);
@@ -1136,8 +1142,6 @@ int BLEClient::handleGAPEvent(struct ble_gap_event *event, void *arg) {
           ble_store_util_delete_peer(&desc.peer_id_addr);
         } else if (BLEDevice::m_securityCallbacks != nullptr) {
           BLEDevice::m_securityCallbacks->onAuthenticationComplete(&desc);
-        } else {
-          client->m_pClientCallbacks->onAuthenticationComplete(&desc);
         }
       }
 
@@ -1164,52 +1168,88 @@ int BLEClient::handleGAPEvent(struct ble_gap_event *event, void *arg) {
       }
 
       if (event->passkey.params.action == BLE_SM_IOACT_DISP) {
+        // Display the passkey on this device
+        log_d("BLE_SM_IOACT_DISP");
+
         pkey.action = event->passkey.params.action;
-        pkey.passkey = BLESecurity::m_passkey;  // This is the passkey to be entered on peer
+        pkey.passkey = BLESecurity::getPassKey();  // This is the passkey to be entered on peer
+
+        if(!BLESecurity::m_passkeySet) {
+          log_w("No passkey set");
+        }
+
+        if (BLESecurity::m_staticPasskey && pkey.passkey == BLE_SM_DEFAULT_PASSKEY) {
+          log_w("*ATTENTION* Using default passkey: %06d", BLE_SM_DEFAULT_PASSKEY);
+          log_w("*ATTENTION* Please use a random passkey or set a different static passkey");
+        } else {
+          log_i("Passkey: %d", pkey.passkey);
+        }
+
+        if (BLEDevice::m_securityCallbacks != nullptr) {
+          BLEDevice::m_securityCallbacks->onPassKeyNotify(pkey.passkey);
+        }
+
         rc = ble_sm_inject_io(event->passkey.conn_handle, &pkey);
         log_d("ble_sm_inject_io result: %d", rc);
 
       } else if (event->passkey.params.action == BLE_SM_IOACT_NUMCMP) {
+        // Check if the passkey on the peer device is correct
+        log_d("BLE_SM_IOACT_NUMCMP");
+
         log_d("Passkey on device's display: %d", event->passkey.params.numcmp);
         pkey.action = event->passkey.params.action;
-        // Compatibility only - Do not use, should be removed the in future
+
         if (BLEDevice::m_securityCallbacks != nullptr) {
           pkey.numcmp_accept = BLEDevice::m_securityCallbacks->onConfirmPIN(event->passkey.params.numcmp);
-          ////////////////////////////////////////////////////
         } else {
-          pkey.numcmp_accept = client->m_pClientCallbacks->onConfirmPIN(event->passkey.params.numcmp);
+          log_e("onConfirmPIN not implemented. Rejecting connection");
+          pkey.numcmp_accept = 0;
         }
 
         rc = ble_sm_inject_io(event->passkey.conn_handle, &pkey);
         log_d("ble_sm_inject_io result: %d", rc);
 
-        //TODO: Handle out of band pairing
       } else if (event->passkey.params.action == BLE_SM_IOACT_OOB) {
+        // Out of band pairing
+        // TODO: Handle out of band pairing
+        log_w("BLE_SM_IOACT_OOB: Not implemented");
+
         static uint8_t tem_oob[16] = {0};
         pkey.action = event->passkey.params.action;
         for (int i = 0; i < 16; i++) {
           pkey.oob[i] = tem_oob[i];
         }
         rc = ble_sm_inject_io(event->passkey.conn_handle, &pkey);
         log_d("ble_sm_inject_io result: %d", rc);
-        ////////
       } else if (event->passkey.params.action == BLE_SM_IOACT_INPUT) {
-        log_d("Enter the passkey");
+        // Input passkey from peer device
+        log_d("BLE_SM_IOACT_INPUT");
+
         pkey.action = event->passkey.params.action;
+        pkey.passkey = BLESecurity::getPassKey();
+
+        if (!BLESecurity::m_passkeySet) {
+          if (BLEDevice::m_securityCallbacks != nullptr) {
+            log_i("No passkey set, getting passkey from onPassKeyRequest");
+            pkey.passkey = BLEDevice::m_securityCallbacks->onPassKeyRequest();
+          } else {
+            log_w("*ATTENTION* onPassKeyRequest not implemented and no static passkey set.");
+          }
+        }
 
-        // Compatibility only - Do not use, should be removed the in future
-        if (BLEDevice::m_securityCallbacks != nullptr) {
-          pkey.passkey = BLEDevice::m_securityCallbacks->onPassKeyRequest();
-          /////////////////////////////////////////////
+        if (BLESecurity::m_staticPasskey && pkey.passkey == BLE_SM_DEFAULT_PASSKEY) {
+          log_w("*ATTENTION* Using default passkey: %06d", BLE_SM_DEFAULT_PASSKEY);
+          log_w("*ATTENTION* Please use a random passkey or set a different static passkey");
         } else {
-          pkey.passkey = client->m_pClientCallbacks->onPassKeyRequest();
+          log_i("Passkey: %d", pkey.passkey);
         }
 
         rc = ble_sm_inject_io(event->passkey.conn_handle, &pkey);
         log_d("ble_sm_inject_io result: %d", rc);
 
       } else if (event->passkey.params.action == BLE_SM_IOACT_NONE) {
-        log_d("No passkey action required");
+        log_d("BLE_SM_IOACT_NONE");
+        log_i("No passkey action required");
       }
 
       return 0;
@@ -1255,20 +1295,6 @@ bool BLEClientCallbacks::onConnParamsUpdateRequest(BLEClient *pClient, const ble
   return true;
 }
 
-uint32_t BLEClientCallbacks::onPassKeyRequest() {
-  log_d("onPassKeyRequest: default: 123456");
-  return 123456;
-}
-
-void BLEClientCallbacks::onAuthenticationComplete(ble_gap_conn_desc *desc) {
-  log_d("onAuthenticationComplete: default");
-}
-
-bool BLEClientCallbacks::onConfirmPIN(uint32_t pin) {
-  log_d("onConfirmPIN: default: true");
-  return true;
-}
-
 #endif  // CONFIG_NIMBLE_ENABLED
 
 #endif /* CONFIG_BLUEDROID_ENABLED || CONFIG_NIMBLE_ENABLED */

libraries/BLE/src/BLEClient.h

@@ -209,9 +209,6 @@ class BLEClientCallbacks {
 
 #if defined(CONFIG_NIMBLE_ENABLED)
   virtual bool onConnParamsUpdateRequest(BLEClient *pClient, const ble_gap_upd_params *params);
-  virtual uint32_t onPassKeyRequest();
-  virtual void onAuthenticationComplete(ble_gap_conn_desc *desc);
-  virtual bool onConfirmPIN(uint32_t pin);
 #endif
 };
 

libraries/BLE/src/BLEDescriptor.h

@@ -42,6 +42,8 @@
 #include <host/ble_att.h>
 #include "BLEConnInfo.h"
 
+// Bluedroid compatibility
+// NimBLE does not support signed reads and writes
 #define ESP_GATT_PERM_READ                BLE_ATT_F_READ
 #define ESP_GATT_PERM_WRITE               BLE_ATT_F_WRITE
 #define ESP_GATT_PERM_READ_ENCRYPTED      BLE_ATT_F_READ_ENC