From bd597082d2b34040c4b9289035bf89380bd97596 Mon Sep 17 00:00:00 2001 From: Steve Karg Date: Fri, 8 Sep 2023 14:58:04 -0500 Subject: [PATCH] Fix ReinitializeDevice service handling of optional password (#487) * Fix ReinitializeDevice service handling of optional password * Improve ReinitializeDevice service unit testing --------- Co-authored-by: Steve Karg --- CHANGELOG.md | 16 +++++ src/bacnet/basic/object/device.c | 14 +++- src/bacnet/basic/service/h_rd.c | 12 ++-- src/bacnet/rd.c | 114 +++++++++++++++++++----------- test/bacnet/rd/src/main.c | 116 +++++++++++++++++++++---------- 5 files changed, 188 insertions(+), 84 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b6a74d2..8dd5d022 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,10 +16,26 @@ The git repositories are hosted at the following sites: ### Security +- Added or updated secure the BACnet primitive value decoders +named bacnet_x_decode(), bacnet_x_application_decode() and +bacnet_x_context_decode where x is one of the 13 BACnet primitive value names. +The updated API includes an APDU size to prevent over-reading of an APDU buffer. +Improved or added unit test code coverage for the primitive value decoders (#481) +- marked the insecure decoding API as 'deprecated' which is defined in +src/bacnet/basic/sys/platform.h and can be disabled during a build. (#481) +- secured decoders for BACnetTimeValue, BACnetHostNPort, BACnetTimeStamp, +BACnetAddress, and Weekly_Schedule and improved unit test code coverage. (#481) +- secured AtomicReadFile and AtomicWriteFile service decoders and +improved unit test code coverage. (#481) +- secured BACnet Error service decoder and improved unit test code coverage. (#481) +- improved test code coverage for BACnet objects and properties. (#481) +- fix ReinitializeDevice handler to clear password before decoding (#485) + ### Added ### Changed + ### Fixed ## [1.1.2] - 2023-08-18 diff --git a/src/bacnet/basic/object/device.c b/src/bacnet/basic/object/device.c index a51cebeb..1e4143ad 100644 --- a/src/bacnet/basic/object/device.c +++ b/src/bacnet/basic/object/device.c @@ -544,8 +544,18 @@ bool Device_Reinitialize(BACNET_REINITIALIZE_DEVICE_DATA *rd_data) { bool status = false; - /* Note: you could use a mix of state and password to multiple things */ - if (characterstring_ansi_same(&rd_data->password, Reinit_Password)) { + /* From 16.4.1.1.2 Password + This optional parameter shall be a CharacterString of up to + 20 characters. For those devices that require the password as a + protection, the service request shall be denied if the parameter + is absent or if the password is incorrect. For those devices that + do not require a password, this parameter shall be ignored.*/ + if (characterstring_length(&rd_data->password) > 20) { + rd_data->error_class = ERROR_CLASS_SERVICES; + rd_data->error_code = ERROR_CODE_PARAMETER_OUT_OF_RANGE; + } else if (characterstring_ansi_same(&rd_data->password, Reinit_Password)) { + /* Note: you could use a mix of state and password to + accomplish multiple things before restarting */ switch (rd_data->state) { case BACNET_REINIT_COLDSTART: case BACNET_REINIT_WARMSTART: diff --git a/src/bacnet/basic/service/h_rd.c b/src/bacnet/basic/service/h_rd.c index 2a7f0b42..0e12f5ce 100644 --- a/src/bacnet/basic/service/h_rd.c +++ b/src/bacnet/basic/service/h_rd.c @@ -69,11 +69,11 @@ void handler_reinitialize_device(uint8_t *service_request, BACNET_ADDRESS *src, BACNET_CONFIRMED_SERVICE_DATA *service_data) { - BACNET_REINITIALIZE_DEVICE_DATA rd_data; + BACNET_REINITIALIZE_DEVICE_DATA rd_data = { 0 }; int len = 0; int pdu_len = 0; - BACNET_NPDU_DATA npdu_data; - BACNET_ADDRESS my_address; + BACNET_NPDU_DATA npdu_data = { 0 }; + BACNET_ADDRESS my_address = { 0 }; /* encode the NPDU portion of the packet */ datalink_get_my_address(&my_address); @@ -98,8 +98,10 @@ void handler_reinitialize_device(uint8_t *service_request, service_request, service_len, &rd_data.state, &rd_data.password); #if PRINT_ENABLED if (len > 0) { - fprintf(stderr, "ReinitializeDevice: state=%u password=%s\n", - (unsigned)rd_data.state, characterstring_value(&rd_data.password)); + fprintf(stderr, "ReinitializeDevice: state=%u password=%*s\n", + (unsigned)rd_data.state, + (int)characterstring_length(&rd_data.password), + characterstring_value(&rd_data.password)); } else { fprintf(stderr, "ReinitializeDevice: Unable to decode request!\n"); } diff --git a/src/bacnet/rd.c b/src/bacnet/rd.c index a64d921b..10563db5 100644 --- a/src/bacnet/rd.c +++ b/src/bacnet/rd.c @@ -41,7 +41,22 @@ /** @file rd.c Encode/Decode Reinitialize Device APDUs */ #if BACNET_SVC_RD_A -/** Encode Reinitialize Device service +/** + * @brief Encode Reinitialize Device service + * + * ReinitializeDevice-Request ::= SEQUENCE { + * reinitialized-state-of-device [0] ENUMERATED { + * coldstart (0), + * warmstart (1), + * start-backup (2), + * end-backup (3), + * start-restore (4), + * end-restore (5), + * abort-restore (6), + * activate-changes (7) + * }, + * password [1] CharacterString (SIZE (1..20)) OPTIONAL + * } * * @param apdu Pointer to the APDU buffer. * @param invoke_id Invoke-Id @@ -63,17 +78,24 @@ int rd_encode_apdu(uint8_t *apdu, apdu[1] = encode_max_segs_max_apdu(0, MAX_APDU); apdu[2] = invoke_id; apdu[3] = SERVICE_CONFIRMED_REINITIALIZE_DEVICE; - apdu_len = 4; - len = encode_context_enumerated(&apdu[apdu_len], 0, state); - apdu_len += len; - /* optional password */ - if (password) { - /* Must be at least 1 character, limited to 20 characters */ - if ((password->length >= 1) && (password->length <= 20)) { - len = encode_context_character_string( - &apdu[apdu_len], 1, password); - apdu_len += len; - } + } + len = 4; + apdu_len += len; + if (apdu) { + apdu += len; + } + len = encode_context_enumerated(apdu, 0, state); + apdu_len += len; + if (apdu) { + apdu += len; + } + /* optional password */ + if (password) { + /* Must be at least 1 character, limited to 20 characters */ + if ((password->length >= 1) && (password->length <= 20)) { + len = encode_context_character_string( + apdu, 1, password); + apdu_len += len; } } @@ -81,52 +103,64 @@ int rd_encode_apdu(uint8_t *apdu, } #endif -/** Decode Reinitialize Device service +/** + * @brief Decode Reinitialize Device service + * + * ReinitializeDevice-Request ::= SEQUENCE { + * reinitialized-state-of-device [0] ENUMERATED { + * coldstart (0), + * warmstart (1), + * start-backup (2), + * end-backup (3), + * start-restore (4), + * end-restore (5), + * abort-restore (6), + * activate-changes (7) + * }, + * password [1] CharacterString (SIZE (1..20)) OPTIONAL + * } * * @param apdu Pointer to the APDU buffer. - * @param apdu_len Valid bytes in the buffer + * @param apdu_size Valid bytes in the buffer * @param state Pointer to the Reinitialization state * @param password Pointer to the pass phrase. * - * @return Bytes encoded. + * @return number of bytes decoded, or #BACNET_STATUS_ERROR if malformed */ int rd_decode_service_request(uint8_t *apdu, - unsigned apdu_len, + unsigned apdu_size, BACNET_REINITIALIZED_STATE *state, BACNET_CHARACTER_STRING *password) { - unsigned len = 0; - uint8_t tag_number = 0; - uint32_t len_value_type = 0; + int len = 0, apdu_len = 0; uint32_t value = 0; /* check for value pointers */ - if ((apdu) && (apdu_len >= 2)) { + if (apdu) { /* Tag 0: reinitializedStateOfDevice */ - if (!decode_is_context_tag(&apdu[len], 0)) { - return -1; - } - len += decode_tag_number_and_value( - &apdu[len], &tag_number, &len_value_type); - len += decode_enumerated(&apdu[len], len_value_type, &value); - if (state) { - *state = (BACNET_REINITIALIZED_STATE)value; + len = bacnet_enumerated_context_decode( + &apdu[apdu_len], apdu_size - apdu_len, 0, &value); + if (len > 0) { + if (state) { + *state = (BACNET_REINITIALIZED_STATE)value; + } + apdu_len = len; + } else { + apdu_len = BACNET_STATUS_ERROR; } /* Tag 1: password - optional */ - if (len < apdu_len) { - if (!decode_is_context_tag(&apdu[len], 1)) { - return -1; - } - len += decode_tag_number_and_value( - &apdu[len], &tag_number, &len_value_type); - if (len < apdu_len) { - if (password) { - len += decode_character_string( - &apdu[len], len_value_type, password); - } + if (apdu_len < apdu_size) { + len = bacnet_character_string_context_decode( + &apdu[apdu_len], apdu_size - apdu_len, 1, password); + if (len > 0) { + apdu_len += len; + } else { + apdu_len = BACNET_STATUS_ERROR; } + } else { + characterstring_init_ansi(password, ""); } } - return (int)len; + return apdu_len; } diff --git a/test/bacnet/rd/src/main.c b/test/bacnet/rd/src/main.c index 7826a1a2..29a68d8d 100644 --- a/test/bacnet/rd/src/main.c +++ b/test/bacnet/rd/src/main.c @@ -21,31 +21,84 @@ * @brief Test */ static int rd_decode_apdu(uint8_t *apdu, - unsigned apdu_len, + unsigned apdu_size, uint8_t *invoke_id, BACNET_REINITIALIZED_STATE *state, BACNET_CHARACTER_STRING *password) { int len = 0; - unsigned offset = 0; + int apdu_len = 0; - if (!apdu) + if (!apdu) { return -1; + } + if (apdu_size <= 4) { + return -1; + } /* optional checking - most likely was already done prior to this call */ - if (apdu[0] != PDU_TYPE_CONFIRMED_SERVICE_REQUEST) + if (apdu[0] != PDU_TYPE_CONFIRMED_SERVICE_REQUEST) { return -1; + } /* apdu[1] = encode_max_segs_max_apdu(0, MAX_APDU); */ - *invoke_id = apdu[2]; /* invoke id - filled in by net layer */ - if (apdu[3] != SERVICE_CONFIRMED_REINITIALIZE_DEVICE) + if (invoke_id) { + *invoke_id = apdu[2]; /* invoke id - filled in by net layer */ + } + if (apdu[3] != SERVICE_CONFIRMED_REINITIALIZE_DEVICE) { return -1; - offset = 4; - - if (apdu_len > offset) { + } + apdu_len = 4; + if (apdu_len < apdu_size) { len = rd_decode_service_request( - &apdu[offset], apdu_len - offset, state, password); + &apdu[apdu_len], apdu_size - apdu_len, state, password); + if (len > 0) { + apdu_len += len; + } else { + apdu_len = len; + } } - return len; + return apdu_len; +} + +static void Test_ReinitializeDevice_Service( + BACNET_REINITIALIZED_STATE state, char *password_string) +{ + uint8_t apdu[480] = { 0 }; + int len = 0; + int apdu_len = 0; + int null_len = 0; + uint8_t invoke_id = 128; + uint8_t test_invoke_id = 0; + BACNET_REINITIALIZED_STATE test_state; + BACNET_CHARACTER_STRING password; + BACNET_CHARACTER_STRING test_password; + + characterstring_init_ansi(&password, password_string); + null_len = rd_encode_apdu(NULL, invoke_id, state, &password); + len = rd_encode_apdu(&apdu[0], invoke_id, state, &password); + zassert_equal(null_len, len, "len=%d null_len=%d", len, null_len); + zassert_true(len > 0, NULL); + apdu_len = len; + + null_len = rd_decode_apdu(&apdu[0], apdu_len, NULL, NULL, NULL); + len = rd_decode_apdu( + &apdu[0], apdu_len, &test_invoke_id, &test_state, &test_password); + zassert_equal(null_len, len, "len=%d null_len=%d", len, null_len); + zassert_true(len > 0, NULL); + zassert_equal(test_invoke_id, invoke_id, NULL); + zassert_equal(test_state, state, NULL); + zassert_true(characterstring_same(&test_password, &password), NULL); + + while (apdu_len) { + apdu_len--; + if (apdu_len == 6) { + /* boundary of optional password, so becomes valid */ + continue; + } + len = rd_decode_apdu(apdu, apdu_len, NULL, NULL, NULL); + zassert_true(len < 0, "len=%d apdu_len=%d password='%s'", len, apdu_len, + password_string); + } } #if defined(CONFIG_ZTEST_NEW_API) @@ -54,28 +107,20 @@ ZTEST(rd_tests, test_ReinitializeDevice) static void test_ReinitializeDevice(void) #endif { - uint8_t apdu[480] = { 0 }; - int len = 0; - int apdu_len = 0; - uint8_t invoke_id = 128; - uint8_t test_invoke_id = 0; - BACNET_REINITIALIZED_STATE state; - BACNET_REINITIALIZED_STATE test_state; - BACNET_CHARACTER_STRING password; - BACNET_CHARACTER_STRING test_password; - - state = BACNET_REINIT_WARMSTART; - characterstring_init_ansi(&password, "John 3:16"); - len = rd_encode_apdu(&apdu[0], invoke_id, state, &password); - zassert_not_equal(len, 0, NULL); - apdu_len = len; - - len = rd_decode_apdu( - &apdu[0], apdu_len, &test_invoke_id, &test_state, &test_password); - zassert_not_equal(len, -1, NULL); - zassert_equal(test_invoke_id, invoke_id, NULL); - zassert_equal(test_state, state, NULL); - zassert_true(characterstring_same(&test_password, &password), NULL); + Test_ReinitializeDevice_Service(BACNET_REINIT_COLDSTART, "John 3:16"); + Test_ReinitializeDevice_Service(BACNET_REINIT_COLDSTART, ""); + Test_ReinitializeDevice_Service(BACNET_REINIT_WARMSTART, "Joshua95"); + Test_ReinitializeDevice_Service(BACNET_REINIT_WARMSTART, ""); + Test_ReinitializeDevice_Service(BACNET_REINIT_STARTBACKUP, "Mary98"); + Test_ReinitializeDevice_Service(BACNET_REINIT_STARTBACKUP, ""); + Test_ReinitializeDevice_Service(BACNET_REINIT_ENDBACKUP, "Anna99"); + Test_ReinitializeDevice_Service(BACNET_REINIT_ENDBACKUP, ""); + Test_ReinitializeDevice_Service(BACNET_REINIT_STARTRESTORE, "Chris04"); + Test_ReinitializeDevice_Service(BACNET_REINIT_STARTRESTORE, ""); + Test_ReinitializeDevice_Service(BACNET_REINIT_ENDRESTORE, "Steve66"); + Test_ReinitializeDevice_Service(BACNET_REINIT_ENDRESTORE, ""); + Test_ReinitializeDevice_Service(BACNET_REINIT_ABORTRESTORE, "Patricia66"); + Test_ReinitializeDevice_Service(BACNET_REINIT_ABORTRESTORE, ""); return; } @@ -83,15 +128,12 @@ static void test_ReinitializeDevice(void) * @} */ - #if defined(CONFIG_ZTEST_NEW_API) ZTEST_SUITE(rd_tests, NULL, NULL, NULL, NULL, NULL); #else void test_main(void) { - ztest_test_suite(rd_tests, - ztest_unit_test(test_ReinitializeDevice) - ); + ztest_test_suite(rd_tests, ztest_unit_test(test_ReinitializeDevice)); ztest_run_test_suite(rd_tests); }