Merge branch 'dev/11/fp3/security-aosp-rvc-release' into int/11/fp3
* dev/11/fp3/security-aosp-rvc-release:
Fix timing attack in BTM_BleVerifySignature
Add bounds checks in btif_avrcp_audio_track.cc
Reject access to secure service authenticated from a temp bonding [3]
Reorganize the code for checking auth requirement
Reject access to secure services authenticated from temp bonding [2]
Reject access to secure service authenticated from a temp bonding [1]
Change-Id: Ie8ea2e1a134ea018dc2c33d5120f3c09916536ec
diff --git a/btif/src/btif_avrcp_audio_track.cc b/btif/src/btif_avrcp_audio_track.cc
index 5cf94e2..6b01080 100644
--- a/btif/src/btif_avrcp_audio_track.cc
+++ b/btif/src/btif_avrcp_audio_track.cc
@@ -23,6 +23,8 @@
#include <base/logging.h>
#include <utils/StrongPointer.h>
+#include <algorithm>
+
#include "bt_target.h"
#include "osi/include/log.h"
@@ -152,7 +154,7 @@
BtifAvrcpAudioTrack* trackHolder) {
size_t sampleSize = sampleSizeFor(trackHolder);
size_t i = 0;
- for (; i <= length / sampleSize; i++) {
+ for (; i < std::min(trackHolder->bufferLength, length / sampleSize); i++) {
trackHolder->buffer[i] = ((int16_t*)buffer)[i] * kScaleQ15ToFloat;
}
return i * sampleSize;
@@ -162,7 +164,7 @@
BtifAvrcpAudioTrack* trackHolder) {
size_t sampleSize = sampleSizeFor(trackHolder);
size_t i = 0;
- for (; i <= length / sampleSize; i++) {
+ for (; i < std::min(trackHolder->bufferLength, length / sampleSize); i++) {
size_t offset = i * sampleSize;
int32_t sample = *((int32_t*)(buffer + offset - 1)) & 0x00FFFFFF;
trackHolder->buffer[i] = sample * kScaleQ23ToFloat;
@@ -174,7 +176,7 @@
BtifAvrcpAudioTrack* trackHolder) {
size_t sampleSize = sampleSizeFor(trackHolder);
size_t i = 0;
- for (; i <= length / sampleSize; i++) {
+ for (; i < std::min(trackHolder->bufferLength, length / sampleSize); i++) {
trackHolder->buffer[i] = ((int32_t*)buffer)[i] * kScaleQ31ToFloat;
}
return i * sampleSize;
diff --git a/stack/Android.bp b/stack/Android.bp
index 2f3d12c..580e0f9 100644
--- a/stack/Android.bp
+++ b/stack/Android.bp
@@ -176,6 +176,7 @@
shared_libs: [
"libcutils",
"liblog",
+ "libcrypto",
],
required: [
"libldacBT_enc",
diff --git a/stack/btm/btm_ble.cc b/stack/btm/btm_ble.cc
index 06a597a..8ba8244 100644
--- a/stack/btm/btm_ble.cc
+++ b/stack/btm/btm_ble.cc
@@ -43,6 +43,7 @@
#include "log/log.h"
#include "main/shim/btm_api.h"
#include "main/shim/shim.h"
+#include "openssl/mem.h"
#include "osi/include/log.h"
#include "osi/include/osi.h"
#include "stack/crypto_toolbox/crypto_toolbox.h"
@@ -2213,7 +2214,7 @@
crypto_toolbox::aes_cmac(p_rec->ble.keys.pcsrk, p_orig, len,
BTM_CMAC_TLEN_SIZE, p_mac);
- if (memcmp(p_mac, p_comp, BTM_CMAC_TLEN_SIZE) == 0) {
+ if (CRYPTO_memcmp(p_mac, p_comp, BTM_CMAC_TLEN_SIZE) == 0) {
btm_ble_increment_sign_ctr(bd_addr, false);
verified = true;
}
diff --git a/stack/btm/btm_sec.cc b/stack/btm/btm_sec.cc
index ec120f6..2e40e02 100644
--- a/stack/btm/btm_sec.cc
+++ b/stack/btm/btm_sec.cc
@@ -106,7 +106,7 @@
uint32_t mx_proto_id,
uint32_t mx_chan_id);
-static bool btm_dev_authenticated(tBTM_SEC_DEV_REC* p_dev_rec);
+static bool btm_dev_authenticated(const tBTM_SEC_DEV_REC* p_dev_rec);
static bool btm_dev_encrypted(tBTM_SEC_DEV_REC* p_dev_rec);
static bool btm_dev_authorized(tBTM_SEC_DEV_REC* p_dev_rec);
static bool btm_serv_trusted(tBTM_SEC_DEV_REC* p_dev_rec,
@@ -148,7 +148,7 @@
* Returns bool true or false
*
******************************************************************************/
-static bool btm_dev_authenticated(tBTM_SEC_DEV_REC* p_dev_rec) {
+static bool btm_dev_authenticated(const tBTM_SEC_DEV_REC* p_dev_rec) {
if (p_dev_rec->sec_flags & BTM_SEC_AUTHENTICATED) {
return (true);
}
@@ -224,6 +224,25 @@
/*******************************************************************************
*
+ * Function access_secure_service_from_temp_bond
+ *
+ * Description a utility function to test whether an access to
+ * secure service from temp bonding is happening
+ *
+ * Returns true if the aforementioned condition holds,
+ * false otherwise
+ *
+ ******************************************************************************/
+static bool access_secure_service_from_temp_bond(const tBTM_SEC_DEV_REC* p_dev_rec,
+ bool locally_initiated,
+ uint16_t security_req) {
+ return !locally_initiated && (security_req & BTM_SEC_IN_AUTHENTICATE) &&
+ btm_dev_authenticated(p_dev_rec) &&
+ p_dev_rec->bond_type == BOND_TYPE_TEMPORARY;
+}
+
+/*******************************************************************************
+ *
* Function BTM_SecRegister
*
* Description Application manager calls this function to register for
@@ -1863,9 +1882,13 @@
}
if (rc == BTM_SUCCESS) {
+ if (access_secure_service_from_temp_bond(p_dev_rec, is_originator, security_required)) {
+ LOG_ERROR(LOG_TAG, "Trying to access a secure service from a temp bonding, rejecting");
+ rc = BTM_FAILED_ON_SECURITY;
+ }
if (p_callback)
- (*p_callback)(&bd_addr, transport, (void*)p_ref_data, BTM_SUCCESS);
- return (BTM_SUCCESS);
+ (*p_callback)(&bd_addr, transport, (void*)p_ref_data, rc);
+ return (rc);
}
}
@@ -1882,15 +1905,15 @@
btm_cb.security_mode == BTM_SEC_MODE_SC) {
if (BTM_SEC_IS_SM4(p_dev_rec->sm4)) {
if (is_originator) {
- /* SM4 to SM4 -> always authenticate & encrypt */
- security_required |= (BTM_SEC_OUT_AUTHENTICATE | BTM_SEC_OUT_ENCRYPT);
+ /* SM4 to SM4 -> always encrypt */
+ security_required |= BTM_SEC_OUT_ENCRYPT;
} else /* acceptor */
{
/* SM4 to SM4: the acceptor needs to make sure the authentication is
* already done */
chk_acp_auth_done = true;
- /* SM4 to SM4 -> always authenticate & encrypt */
- security_required |= (BTM_SEC_IN_AUTHENTICATE | BTM_SEC_IN_ENCRYPT);
+ /* SM4 to SM4 -> always encrypt */
+ security_required |= BTM_SEC_IN_ENCRYPT;
}
} else if (!(BTM_SM4_KNOWN & p_dev_rec->sm4)) {
/* the remote features are not known yet */
@@ -2188,6 +2211,11 @@
mx_chan_id, p_callback, p_ref_data);
} else /* rc == BTM_SUCCESS */
{
+ if (access_secure_service_from_temp_bond(p_dev_rec,
+ is_originator, security_required)) {
+ LOG_ERROR(LOG_TAG, "Trying to access a secure rfcomm service from a temp bonding, reject");
+ rc = BTM_FAILED_ON_SECURITY;
+ }
/* access granted */
if (p_callback) {
(*p_callback)(&bd_addr, transport, p_ref_data, (uint8_t)rc);
@@ -4845,46 +4873,65 @@
/* If connection is not authenticated and authentication is required */
/* start authentication and return PENDING to the caller */
- if ((((!(p_dev_rec->sec_flags & BTM_SEC_AUTHENTICATED)) &&
- ((p_dev_rec->is_originator &&
- (p_dev_rec->security_required & BTM_SEC_OUT_AUTHENTICATE)) ||
- (!p_dev_rec->is_originator &&
- (p_dev_rec->security_required & BTM_SEC_IN_AUTHENTICATE)))) ||
- (!(p_dev_rec->sec_flags & BTM_SEC_16_DIGIT_PIN_AUTHED) &&
- (!p_dev_rec->is_originator &&
- (p_dev_rec->security_required & BTM_SEC_IN_MIN_16_DIGIT_PIN)))) &&
- (p_dev_rec->hci_handle != BTM_SEC_INVALID_HANDLE)) {
-/*
- * We rely on BTM_SEC_16_DIGIT_PIN_AUTHED being set if MITM is in use,
- * as 16 DIGIT is only needed if MITM is not used. Unfortunately, the
- * BTM_SEC_AUTHENTICATED is used for both MITM and non-MITM
- * authenticated connections, hence we cannot distinguish here.
- */
+ if (p_dev_rec->hci_handle != HCI_INVALID_HANDLE) {
+ bool start_auth = false;
- BTM_TRACE_EVENT("Security Manager: Start authentication");
-
- /*
- * If we do have a link-key, but we end up here because we need an
- * upgrade, then clear the link-key known and authenticated flag before
- * restarting authentication.
- * WARNING: If the controller has link-key, it is optional and
- * recommended for the controller to send a Link_Key_Request.
- * In case we need an upgrade, the only alternative would be to delete
- * the existing link-key. That could lead to very bad user experience
- * or even IOP issues, if a reconnect causes a new connection that
- * requires an upgrade.
- */
- if ((p_dev_rec->sec_flags & BTM_SEC_LINK_KEY_KNOWN) &&
- (!(p_dev_rec->sec_flags & BTM_SEC_16_DIGIT_PIN_AUTHED) &&
- (!p_dev_rec->is_originator &&
- (p_dev_rec->security_required & BTM_SEC_IN_MIN_16_DIGIT_PIN)))) {
- p_dev_rec->sec_flags &=
- ~(BTM_SEC_LINK_KEY_KNOWN | BTM_SEC_LINK_KEY_AUTHED |
- BTM_SEC_AUTHENTICATED);
+ // Check link status of BR/EDR
+ if (!(p_dev_rec->sec_flags & BTM_SEC_AUTHENTICATED)) {
+ if (p_dev_rec->is_originator) {
+ if (p_dev_rec->security_required & BTM_SEC_OUT_AUTHENTICATE) {
+ LOG_DEBUG(LOG_TAG, "Outgoing authentication Required");
+ start_auth = true;
+ }
+ } else {
+ if (p_dev_rec->security_required & BTM_SEC_IN_AUTHENTICATE) {
+ LOG_DEBUG(LOG_TAG, "Incoming authentication Required");
+ start_auth = true;
+ }
+ }
}
- btm_sec_start_authentication(p_dev_rec);
- return (BTM_CMD_STARTED);
+ if (!(p_dev_rec->sec_flags & BTM_SEC_16_DIGIT_PIN_AUTHED)) {
+ /*
+ * We rely on BTM_SEC_16_DIGIT_PIN_AUTHED being set if MITM is in use,
+ * as 16 DIGIT is only needed if MITM is not used. Unfortunately, the
+ * BTM_SEC_AUTHENTICATED is used for both MITM and non-MITM
+ * authenticated connections, hence we cannot distinguish here.
+ */
+ if (!p_dev_rec->is_originator) {
+ if (p_dev_rec->security_required & BTM_SEC_IN_MIN_16_DIGIT_PIN) {
+ LOG_DEBUG(LOG_TAG, "BTM_SEC_IN_MIN_16_DIGIT_PIN Required");
+ start_auth = true;
+ }
+ }
+ }
+
+ if (start_auth) {
+ LOG_DEBUG(LOG_TAG, "Security Manager: Start authentication");
+
+ /*
+ * If we do have a link-key, but we end up here because we need an
+ * upgrade, then clear the link-key known and authenticated flag before
+ * restarting authentication.
+ * WARNING: If the controller has link-key, it is optional and
+ * recommended for the controller to send a Link_Key_Request.
+ * In case we need an upgrade, the only alternative would be to delete
+ * the existing link-key. That could lead to very bad user experience
+ * or even IOP issues, if a reconnect causes a new connection that
+ * requires an upgrade.
+ */
+ if ((p_dev_rec->sec_flags & BTM_SEC_LINK_KEY_KNOWN) &&
+ (!(p_dev_rec->sec_flags & BTM_SEC_16_DIGIT_PIN_AUTHED) &&
+ (!p_dev_rec->is_originator &&
+ (p_dev_rec->security_required & BTM_SEC_IN_MIN_16_DIGIT_PIN)))) {
+ p_dev_rec->sec_flags &=
+ ~(BTM_SEC_LINK_KEY_KNOWN | BTM_SEC_LINK_KEY_AUTHED |
+ BTM_SEC_AUTHENTICATED);
+ }
+
+ btm_sec_start_authentication(p_dev_rec);
+ return (BTM_CMD_STARTED);
+ }
}
/* If connection is not encrypted and encryption is required */
@@ -4930,6 +4977,13 @@
}
}
+ if (access_secure_service_from_temp_bond(p_dev_rec,
+ p_dev_rec->is_originator,
+ p_dev_rec->security_required)) {
+ LOG_ERROR(LOG_TAG, "Trying to access a secure service from a temp bonding, rejecting");
+ return (BTM_FAILED_ON_SECURITY);
+ }
+
/* All required security procedures already established */
p_dev_rec->security_required &=
~(BTM_SEC_OUT_AUTHORIZE | BTM_SEC_IN_AUTHORIZE |