BLE Host - validate write upon rx of prep write Before, upon receiving a prepare write request, the code was only verifying that the specified attribute existed. There are a number of checks that need to be performed in addition to that one:
1. Insufficient authorization. 2. Insufficient authentication. 3. Insufficient encryption key size. 4. Insufficient encryption (check if persisted bond exists for unencrypted peer). 5. Invalid handle. 6. Write not permitted. The host does not properly handle 3 and 4 yet (for any writes). Now the host rejects a prepare write request if any of the above issues are detected (except for 3 and 4). Project: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/commit/d6fab4dc Tree: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/tree/d6fab4dc Diff: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/diff/d6fab4dc Branch: refs/heads/sterly_refactor Commit: d6fab4dc747569d176782a056544572c0b746c72 Parents: de85f43 Author: Christopher Collins <[email protected]> Authored: Sun Aug 7 17:48:44 2016 -0700 Committer: Sterling Hughes <[email protected]> Committed: Tue Aug 9 16:05:21 2016 -0700 ---------------------------------------------------------------------- net/nimble/host/src/ble_att_svr.c | 69 ++++++++++++++---------- net/nimble/host/src/test/ble_att_svr_test.c | 62 +++++++++++++++++---- 2 files changed, 95 insertions(+), 36 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/d6fab4dc/net/nimble/host/src/ble_att_svr.c ---------------------------------------------------------------------- diff --git a/net/nimble/host/src/ble_att_svr.c b/net/nimble/host/src/ble_att_svr.c index 1b420b5..f282093 100644 --- a/net/nimble/host/src/ble_att_svr.c +++ b/net/nimble/host/src/ble_att_svr.c @@ -237,9 +237,9 @@ ble_att_svr_get_sec_state(uint16_t conn_handle, } static int -ble_att_svr_check_security(uint16_t conn_handle, int is_read, - struct ble_att_svr_entry *entry, - uint8_t *out_att_err) +ble_att_svr_check_perms(uint16_t conn_handle, int is_read, + struct ble_att_svr_entry *entry, + uint8_t *out_att_err) { struct ble_gap_sec_state sec_state; int author; @@ -248,10 +248,20 @@ ble_att_svr_check_security(uint16_t conn_handle, int is_read, int rc; if (is_read) { + if (!(entry->ha_flags & BLE_ATT_F_READ)) { + *out_att_err = BLE_ATT_ERR_READ_NOT_PERMITTED; + return BLE_HS_ENOTSUP; + } + enc = entry->ha_flags & BLE_ATT_F_READ_ENC; authen = entry->ha_flags & BLE_ATT_F_READ_AUTHEN; author = entry->ha_flags & BLE_ATT_F_READ_AUTHOR; } else { + if (!(entry->ha_flags & BLE_ATT_F_WRITE)) { + *out_att_err = BLE_ATT_ERR_WRITE_NOT_PERMITTED; + return BLE_HS_ENOTSUP; + } + enc = entry->ha_flags & BLE_ATT_F_WRITE_ENC; authen = entry->ha_flags & BLE_ATT_F_WRITE_AUTHEN; author = entry->ha_flags & BLE_ATT_F_WRITE_AUTHOR; @@ -264,6 +274,8 @@ ble_att_svr_check_security(uint16_t conn_handle, int is_read, rc = ble_att_svr_get_sec_state(conn_handle, &sec_state); if (rc != 0) { + /* Peer no longer connected. */ + *out_att_err = 0; return rc; } @@ -300,13 +312,7 @@ ble_att_svr_read(uint16_t conn_handle, att_err = 0; /* Silence gcc warning. */ if (conn_handle != BLE_HS_CONN_HANDLE_NONE) { - if (!(entry->ha_flags & BLE_ATT_F_READ)) { - att_err = BLE_ATT_ERR_READ_NOT_PERMITTED; - rc = BLE_HS_ENOTSUP; - goto err; - } - - rc = ble_att_svr_check_security(conn_handle, 1, entry, &att_err); + rc = ble_att_svr_check_perms(conn_handle, 1, entry, &att_err); if (rc != 0) { goto err; } @@ -448,13 +454,7 @@ ble_att_svr_write(uint16_t conn_handle, struct ble_att_svr_entry *entry, BLE_HS_DBG_ASSERT(!ble_hs_locked_by_cur_task()); if (conn_handle != BLE_HS_CONN_HANDLE_NONE) { - if (!(entry->ha_flags & BLE_ATT_F_WRITE)) { - att_err = BLE_ATT_ERR_WRITE_NOT_PERMITTED; - rc = BLE_HS_ENOTSUP; - goto done; - } - - rc = ble_att_svr_check_security(conn_handle, 0, entry, &att_err); + rc = ble_att_svr_check_perms(conn_handle, 0, entry, &att_err); if (rc != 0) { goto done; } @@ -2341,19 +2341,17 @@ ble_att_svr_prep_write(uint16_t conn_handle, while (!SLIST_EMPTY(prep_list)) { ble_att_svr_prep_extract(prep_list, &attr_handle, &om); + /* Attribute existence was verified during prepare-write request + * processing. + */ attr = ble_att_svr_find_by_handle(attr_handle); - *err_handle = attr_handle; - if (attr == NULL) { - rc = BLE_ATT_ERR_INVALID_HANDLE; - } else { - rc = ble_att_svr_write(conn_handle, attr, 0, &om, &att_err); - if (rc != 0) { - rc = att_err; - } - } + BLE_HS_DBG_ASSERT(attr != NULL); + + rc = ble_att_svr_write(conn_handle, attr, 0, &om, &att_err); os_mbuf_free_chain(om); if (rc != 0) { - return rc; + *err_handle = attr_handle; + return att_err; } } @@ -2400,12 +2398,29 @@ ble_att_svr_rx_prep_write(uint16_t conn_handle, struct os_mbuf **rxom) os_mbuf_adj(*rxom, BLE_ATT_PREP_WRITE_CMD_BASE_SZ); attr_entry = ble_att_svr_find_by_handle(req.bapc_handle); + + /* A prepare write request gets rejected for the following reasons: + * 1. Insufficient authorization. + * 2. Insufficient authentication. + * 3. Insufficient encryption key size (XXX: Not checked). + * 4. Insufficient encryption. + * 5. Invalid handle. + * 6. Write not permitted. + */ + + /* <5> */ if (attr_entry == NULL) { rc = BLE_HS_ENOENT; att_err = BLE_ATT_ERR_INVALID_HANDLE; goto done; } + /* <1>, <2>, <4>, <6> */ + rc = ble_att_svr_check_perms(conn_handle, 0, attr_entry, &att_err); + if (rc != 0) { + goto done; + } + prep_entry = ble_att_svr_prep_alloc(); if (prep_entry == NULL) { att_err = BLE_ATT_ERR_PREPARE_QUEUE_FULL; http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/d6fab4dc/net/nimble/host/src/test/ble_att_svr_test.c ---------------------------------------------------------------------- diff --git a/net/nimble/host/src/test/ble_att_svr_test.c b/net/nimble/host/src/test/ble_att_svr_test.c index 09e7bf9..86565e3 100644 --- a/net/nimble/host/src/test/ble_att_svr_test.c +++ b/net/nimble/host/src/test/ble_att_svr_test.c @@ -319,6 +319,15 @@ ble_att_svr_test_misc_attr_fn_w_2(uint16_t conn_handle, uint16_t attr_handle, } } +static int +ble_att_svr_test_misc_attr_fn_w_fail(uint16_t conn_handle, + uint16_t attr_handle, + uint8_t op, uint16_t offset, + struct os_mbuf **om, void *arg) +{ + return BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; +} + static void ble_att_svr_test_misc_verify_w_1(void *data, int data_len) { @@ -2071,6 +2080,7 @@ TEST_CASE(ble_att_svr_test_read_group_type) TEST_CASE(ble_att_svr_test_prep_write) { + struct ble_hs_conn *conn; uint16_t conn_handle; int i; @@ -2089,9 +2099,22 @@ TEST_CASE(ble_att_svr_test_prep_write) ble_att_svr_test_misc_register_uuid16(0x8989, HA_FLAG_PERM_RW, 2, ble_att_svr_test_misc_attr_fn_w_2); - /* Register a third attribute that is not writable. */ + /* 3: not writable. */ ble_att_svr_test_misc_register_uuid16(0xabab, BLE_ATT_F_READ, 3, ble_att_svr_test_misc_attr_fn_r_1); + /* 4: Encryption required. */ + ble_att_svr_test_misc_register_uuid16( + 0xabac, BLE_ATT_F_WRITE | BLE_ATT_F_WRITE_ENC, 4, + ble_att_svr_test_misc_attr_fn_w_1); + + /* 5: Encryption+authentication required. */ + ble_att_svr_test_misc_register_uuid16( + 0xabad, BLE_ATT_F_WRITE | BLE_ATT_F_WRITE_ENC | BLE_ATT_F_WRITE_AUTHEN, + 5, ble_att_svr_test_misc_attr_fn_w_1); + + /* 6: Write callback always fails. */ + ble_att_svr_test_misc_register_uuid16( + 0xabae, BLE_ATT_F_WRITE, 6, ble_att_svr_test_misc_attr_fn_w_fail); /*** Empty write succeeds. */ ble_att_svr_test_misc_exec_write(conn_handle, BLE_ATT_EXEC_WRITE_F_CONFIRM, @@ -2104,6 +2127,28 @@ TEST_CASE(ble_att_svr_test_prep_write) ble_att_svr_test_misc_prep_write(conn_handle, 53525, 0, data, 10, BLE_ATT_ERR_INVALID_HANDLE); + /*** Failure due to write-not-permitted. */ + ble_att_svr_test_misc_prep_write(conn_handle, 3, 0, data, 35, + BLE_ATT_ERR_WRITE_NOT_PERMITTED); + + /*** Failure due to insufficient authentication (encryption required). */ + ble_att_svr_test_misc_prep_write(conn_handle, 4, 0, data, 1, + BLE_ATT_ERR_INSUFFICIENT_AUTHEN); + + /*** Encrypt connection; ensure previous prep write now succeeds. */ + ble_hs_lock(); + conn = ble_hs_conn_find(2); + TEST_ASSERT_FATAL(conn != NULL); + conn->bhc_sec_state.encrypted = 1; + ble_hs_unlock(); + + ble_att_svr_test_misc_prep_write(conn_handle, 4, 0, data, 1, 0); + ble_att_svr_test_misc_exec_write(conn_handle, 0, 0, 0); + + /*** Failure due to insufficient authentication (not authenticated). */ + ble_att_svr_test_misc_prep_write(conn_handle, 5, 0, data, 35, + BLE_ATT_ERR_INSUFFICIENT_AUTHEN); + /*** Failure for write starting at nonzero offset. */ ble_att_svr_test_misc_prep_write(conn_handle, 1, 1, data, 10, 0); ble_att_svr_test_misc_exec_write(conn_handle, BLE_ATT_EXEC_WRITE_F_CONFIRM, @@ -2136,14 +2181,6 @@ TEST_CASE(ble_att_svr_test_prep_write) BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN, 1); ble_att_svr_test_misc_verify_w_1(NULL, 0); - /*** Failure due to attribute callback. */ - ble_att_svr_test_misc_prep_write(conn_handle, 3, 0, data, 35, 0); - ble_att_svr_test_misc_prep_write(conn_handle, 3, 35, data + 35, 43, 0); - ble_att_svr_test_misc_prep_write(conn_handle, 3, 78, data + 78, 1, 0); - ble_att_svr_test_misc_exec_write(conn_handle, BLE_ATT_EXEC_WRITE_F_CONFIRM, - BLE_ATT_ERR_WRITE_NOT_PERMITTED, 0); - ble_att_svr_test_misc_verify_w_1(NULL, 0); - /*** Successful two part write. */ ble_att_svr_test_misc_prep_write(conn_handle, 1, 0, data, 20, 0); ble_att_svr_test_misc_prep_write(conn_handle, 1, 20, data + 20, 20, 0); @@ -2188,6 +2225,13 @@ TEST_CASE(ble_att_svr_test_prep_write) 0, 0); ble_att_svr_test_misc_verify_w_1(data, 12); ble_att_svr_test_misc_verify_w_2(data, 61); + + /*** Fail due to attribute callback error. */ + ble_att_svr_test_misc_prep_write(conn_handle, 6, 0, data, 35, 0); + ble_att_svr_test_misc_prep_write(conn_handle, 6, 35, data + 35, 43, 0); + ble_att_svr_test_misc_prep_write(conn_handle, 6, 78, data + 78, 1, 0); + ble_att_svr_test_misc_exec_write(conn_handle, BLE_ATT_EXEC_WRITE_F_CONFIRM, + BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN, 6); } TEST_CASE(ble_att_svr_test_notify)
