Skip to content

Commit d2134dd

Browse files
committed
bluetooth: fast_pair: fix Additional Data write length validation
Reworked the write_additional_data() GATT callback in the fp_gatt_service module to validate the ATT write payload length before deriving the decoded data length from it. Previously, the function computed the decoded data length as len - FP_CRYPTO_ADDITIONAL_DATA_HEADER_LEN and used the result as the size of a stack-allocated variable-length array, without first checking that len is at least equal to the header length. A short write would underflow the unsigned subtraction, and a long write could exceed the available stack budget. Replaced the variable-length array with a fixed-size buffer of FP_STORAGE_PN_BUF_LEN bytes, sized to the worst-case Personalized Name plus the NULL terminator, and added explicit length checks: - Rejected writes whose length is less than or equal to the Additional Data header length with the BT_ATT_ERR_INVALID_ATTRIBUTE_LEN ATT error. - Rejected writes that would not fit in the decoded_data buffer with the BT_ATT_ERR_INSUFFICIENT_RESOURCES ATT error, and emitted a warning that points to the CONFIG_BT_FAST_PAIR_STORAGE_PN_LEN_MAX Kconfig option for longer Personalized Name support. Ref: NCSDK-39121 Signed-off-by: Kamil Piszczek <Kamil.Piszczek@nordicsemi.no>
1 parent 980bcd2 commit d2134dd

1 file changed

Lines changed: 26 additions & 2 deletions

File tree

subsys/bluetooth/fast_pair/fp_gatt_service.c

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -784,9 +784,13 @@ static ssize_t write_additional_data(struct bt_conn *conn,
784784
{
785785
/* The only expected Additional Data write is with Personalized Name. fp_keys module will
786786
* return an error if it receives Personalized Name store request in inappropriate state.
787+
*
788+
* The decoded payload is at most FP_STORAGE_PN_BUF_LEN - 1 bytes (the maximum
789+
* Personalized Name length), with one extra byte reserved for the NULL terminator
790+
* appended below.
787791
*/
788-
size_t data_len = len - FP_CRYPTO_ADDITIONAL_DATA_HEADER_LEN;
789-
uint8_t decoded_data[data_len + sizeof(char)];
792+
uint8_t decoded_data[FP_STORAGE_PN_BUF_LEN];
793+
size_t data_len;
790794
int err = 0;
791795
ssize_t res = len;
792796

@@ -807,6 +811,25 @@ static ssize_t write_additional_data(struct bt_conn *conn,
807811
goto finish;
808812
}
809813

814+
/* Validate the ATT write length before any arithmetic on it: a value below the
815+
* header length would underflow the unsigned data_len computation below, and a value
816+
* above the buffer capacity would overflow the decoded_data buffer.
817+
*/
818+
if (len <= FP_CRYPTO_ADDITIONAL_DATA_HEADER_LEN) {
819+
LOG_WRN("Invalid length: len=%" PRIu16 " (Additional Data)", len);
820+
res = BT_GATT_ERR(BT_ATT_ERR_INVALID_ATTRIBUTE_LEN);
821+
goto finish;
822+
}
823+
824+
if (len > FP_CRYPTO_ADDITIONAL_DATA_HEADER_LEN + sizeof(decoded_data) - 1) {
825+
LOG_WRN("Length exceeds buffer capacity: len=%" PRIu16 " (Additional Data)", len);
826+
LOG_WRN("Align the CONFIG_BT_FAST_PAIR_STORAGE_PN_LEN_MAX Kconfig to support "
827+
"longer Personalized Name");
828+
res = BT_GATT_ERR(BT_ATT_ERR_INSUFFICIENT_RESOURCES);
829+
goto finish;
830+
}
831+
832+
/* Decode the encrypted field in the Additional Data payload. */
810833
err = fp_keys_additional_data_decode(conn, decoded_data, buf, len);
811834
if (err) {
812835
LOG_WRN("Decrypt failed: err=%d (Additional Data)", err);
@@ -815,6 +838,7 @@ static ssize_t write_additional_data(struct bt_conn *conn,
815838
}
816839

817840
/* Received data is assumed to be a Personalized Name string. Terminate the string. */
841+
data_len = len - FP_CRYPTO_ADDITIONAL_DATA_HEADER_LEN;
818842
decoded_data[data_len] = '\0';
819843

820844
LOG_DBG("Received following Personalized Name: %s (Additional Data)", decoded_data);

0 commit comments

Comments
 (0)