diff --git a/CHANGELOG.md b/CHANGELOG.md index dab933f3..626a9c02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ The git repositories are hosted at the following sites: ### Security +* Secured BACnetAuthenticationFactorFormat decoding by refactoring deprecated + functions and validating with unit testing. (#1127) * Secured UnconfirmedEventNotification-Request and ConfirmedEventNotification-Request, BACnetNotificationParameters, and BACnetAuthenticationFactor decoding by refactoring deprecated diff --git a/src/bacnet/authentication_factor_format.c b/src/bacnet/authentication_factor_format.c index 48200d54..cb13ae81 100644 --- a/src/bacnet/authentication_factor_format.c +++ b/src/bacnet/authentication_factor_format.c @@ -9,132 +9,213 @@ #include "bacnet/bacdcode.h" #include "bacnet/authentication_factor_format.h" +/** + * @brief Encode BACnetAuthenticationFactorFormat data + * @param apdu - buffer to store the encoding, or NULL for length + * @param data - data to use for the encoding values + * @return number of bytes in the buffer + */ int bacapp_encode_authentication_factor_format( - uint8_t *apdu, const BACNET_AUTHENTICATION_FACTOR_FORMAT *aff) + uint8_t *apdu, const BACNET_AUTHENTICATION_FACTOR_FORMAT *data) { int len; int apdu_len = 0; - len = encode_context_enumerated(&apdu[apdu_len], 0, aff->format_type); - if (len < 0) { - return -1; - } else { + len = encode_context_enumerated(apdu, 0, data->format_type); + apdu_len += len; + if (apdu) { + apdu += len; + } + if (data->format_type == AUTHENTICATION_FACTOR_CUSTOM) { + /* optional fields are required when Format-Type + field has a value of CUSTOM. */ + len = encode_context_unsigned(apdu, 1, data->vendor_id); + apdu_len += len; + if (apdu) { + apdu += len; + } + len = encode_context_unsigned(apdu, 2, data->vendor_format); apdu_len += len; } - if (aff->format_type == AUTHENTICATION_FACTOR_CUSTOM) { - len = encode_context_unsigned(&apdu[apdu_len], 1, aff->vendor_id); - if (len < 0) { - return -1; - } else { - apdu_len += len; - } - - len = encode_context_unsigned(&apdu[apdu_len], 2, aff->vendor_format); - if (len < 0) { - return -1; - } else { - apdu_len += len; - } - } return apdu_len; } +/** + * @brief Context encode BACnetAuthenticationFactorFormat data + * @param apdu - buffer to store the encoding, or NULL for length + * @param tag - tag number used to encapsulate the data within open/close tags + * @param data - data to use for the encoding values + * @return number of bytes in the buffer + */ int bacapp_encode_context_authentication_factor_format( - uint8_t *apdu, uint8_t tag, const BACNET_AUTHENTICATION_FACTOR_FORMAT *aff) + uint8_t *apdu, uint8_t tag, const BACNET_AUTHENTICATION_FACTOR_FORMAT *data) { int len; int apdu_len = 0; - len = encode_opening_tag(&apdu[apdu_len], tag); + len = encode_opening_tag(apdu, tag); apdu_len += len; - - len = bacapp_encode_authentication_factor_format(&apdu[apdu_len], aff); + if (apdu) { + apdu += len; + } + len = bacapp_encode_authentication_factor_format(apdu, data); apdu_len += len; - - len = encode_closing_tag(&apdu[apdu_len], tag); + if (apdu) { + apdu += len; + } + len = encode_closing_tag(apdu, tag); apdu_len += len; return apdu_len; } -int bacapp_decode_authentication_factor_format( - const uint8_t *apdu, BACNET_AUTHENTICATION_FACTOR_FORMAT *aff) +/** + * @brief Decode a BACnetAuthenticationFactorFormat property value + * @details BACnetAuthenticationFactorFormat ::= SEQUENCE { + * format-type[0] BACnetAuthenticationFactorType, + * vendor-id[1] Unsigned16 OPTIONAL, + * vendor-format[2] Unsigned16 OPTIONAL + * } + * @param apdu Pointer to the buffer for decoding. + * @param apdu_size Count of valid bytes in the buffer. + * @param data Pointer to the property decoded data to be stored + * or NULL for length + * @return number of bytes decoded or BACNET_STATUS_ERROR on error. + */ +int bacnet_authentication_factor_format_decode( + const uint8_t *apdu, + size_t apdu_size, + BACNET_AUTHENTICATION_FACTOR_FORMAT *data) { int len; int apdu_len = 0; - uint32_t format_type = aff->format_type; + uint32_t enum_value = 0; BACNET_UNSIGNED_INTEGER unsigned_value = 0; + BACNET_AUTHENTICATION_FACTOR_TYPE format_type = + AUTHENTICATION_FACTOR_UNDEFINED; - if (decode_is_context_tag(&apdu[apdu_len], 0)) { - len = decode_context_enumerated(&apdu[apdu_len], 0, &format_type); - if (len < 0) { - return -1; - } else if (format_type < AUTHENTICATION_FACTOR_MAX) { - apdu_len += len; - aff->format_type = (BACNET_AUTHENTICATION_FACTOR_TYPE)format_type; - } else { - /* FIXME: Maybe this should return BACNET_STATUS_REJECT */ - return -1; + /* format-type[0] BACnetAuthenticationFactorType */ + len = bacnet_enumerated_context_decode( + &apdu[apdu_len], apdu_size - apdu_len, 0, &enum_value); + if (len > 0) { + apdu_len += len; + if (enum_value > AUTHENTICATION_FACTOR_MAX) { + enum_value = AUTHENTICATION_FACTOR_MAX; + } + format_type = (BACNET_AUTHENTICATION_FACTOR_TYPE)enum_value; + if (data) { + data->format_type = format_type; } } else { - return -1; + return BACNET_STATUS_ERROR; } - - if (decode_is_context_tag(&apdu[apdu_len], 1)) { - len = decode_context_unsigned(&apdu[apdu_len], 1, &unsigned_value); - if (len < 0) { - return -1; - } else { - aff->vendor_id = unsigned_value; + if (format_type == AUTHENTICATION_FACTOR_CUSTOM) { + /* optional fields are required when Format-Type + field has a value of CUSTOM. */ + /* vendor-id[1] Unsigned16 OPTIONAL */ + len = bacnet_unsigned_context_decode( + &apdu[apdu_len], apdu_size - apdu_len, 1, &unsigned_value); + if (len > 0) { apdu_len += len; - } - if ((aff->format_type != AUTHENTICATION_FACTOR_CUSTOM) && - (aff->vendor_id != 0)) { - return -1; - } - } - - if (decode_is_context_tag(&apdu[apdu_len], 2)) { - len = decode_context_unsigned(&apdu[apdu_len], 2, &unsigned_value); - if (len < 0) { - return -1; + if (unsigned_value > UINT16_MAX) { + return BACNET_STATUS_ERROR; + } + if (data) { + data->vendor_id = (uint16_t)unsigned_value; + } } else { - aff->vendor_format = unsigned_value; - apdu_len += len; + return BACNET_STATUS_ERROR; } - if ((aff->format_type != AUTHENTICATION_FACTOR_CUSTOM) && - (aff->vendor_format != 0)) { - return -1; + /* vendor-format[2] Unsigned16 OPTIONAL */ + len = bacnet_unsigned_context_decode( + &apdu[apdu_len], apdu_size - apdu_len, 2, &unsigned_value); + if (len > 0) { + apdu_len += len; + if (unsigned_value > UINT16_MAX) { + return BACNET_STATUS_ERROR; + } + if (data) { + data->vendor_format = (uint16_t)unsigned_value; + } + } else { + return BACNET_STATUS_ERROR; } } return apdu_len; } -int bacapp_decode_context_authentication_factor_format( - const uint8_t *apdu, uint8_t tag, BACNET_AUTHENTICATION_FACTOR_FORMAT *aff) +/** + * @brief Decode a BACnetAuthenticationFactorFormat property value + * @param apdu Pointer to the buffer for decoding. + * @param apdu_size Count of valid bytes in the buffer. + * @param data Pointer to the property decoded data to be stored + * or NULL for length + * @return Number of bytes decoded or BACNET_STATUS_ERROR on error. + * @deprecated Use bacnet_authentication_factor_format_decode() instead + */ +int bacapp_decode_authentication_factor_format( + const uint8_t *apdu, BACNET_AUTHENTICATION_FACTOR_FORMAT *data) { - int len = 0; - int section_length; + return bacnet_authentication_factor_format_decode(apdu, MAX_APDU, data); +} - if (decode_is_opening_tag_number(&apdu[len], tag)) { - len++; - section_length = - bacapp_decode_authentication_factor_format(&apdu[len], aff); +/** + * @brief Decode a context tagged BACnetAuthenticationFactorFormat property + * value + * @param apdu Pointer to the buffer for decoding. + * @param tag - tag number used to encapsulate the data within open/close tags + * @param data Pointer to the property decoded data to be stored + * or NULL for length + * @return number of bytes decoded or BACNET_STATUS_ERROR on error. + */ +int bacnet_authentication_factor_format_context_decode( + const uint8_t *apdu, + size_t apdu_size, + uint8_t tag, + BACNET_AUTHENTICATION_FACTOR_FORMAT *data) +{ + int len = 0, apdu_len = 0; - if (section_length == -1) { - len = -1; + if (bacnet_is_opening_tag_number( + &apdu[apdu_len], apdu_size - apdu_len, tag, &len)) { + apdu_len += len; + len = bacnet_authentication_factor_format_decode( + &apdu[apdu_len], apdu_size - apdu_len, data); + if (len > 0) { + apdu_len += len; } else { - len += section_length; - if (decode_is_closing_tag_number(&apdu[len], tag)) { - len++; - } else { - len = -1; - } + return BACNET_STATUS_ERROR; + } + if (bacnet_is_closing_tag_number( + &apdu[apdu_len], apdu_size - apdu_len, tag, &len)) { + apdu_len += len; + } else { + return BACNET_STATUS_ERROR; } } else { - len = -1; + return BACNET_STATUS_ERROR; } - return len; + + return apdu_len; +} + +/** + * @brief Decode a context tagged BACnetAuthenticationFactorFormat property + * value + * @param apdu Pointer to the buffer for decoding. + * @param tag - tag number used to encapsulate the data within open/close + * tags + * @param data Pointer to the property decoded data to be stored + * or NULL for length + * @return number of bytes decoded or BACNET_STATUS_ERROR on error. + * @deprecated use bacnet_authentication_factor_format_context_decode() + * instead + */ +int bacapp_decode_context_authentication_factor_format( + const uint8_t *apdu, uint8_t tag, BACNET_AUTHENTICATION_FACTOR_FORMAT *data) +{ + return bacnet_authentication_factor_format_context_decode( + apdu, MAX_APDU, tag, data); } diff --git a/src/bacnet/authentication_factor_format.h b/src/bacnet/authentication_factor_format.h index 5611b201..741d473d 100644 --- a/src/bacnet/authentication_factor_format.h +++ b/src/bacnet/authentication_factor_format.h @@ -30,9 +30,25 @@ int bacapp_encode_context_authentication_factor_format( uint8_t *apdu, uint8_t tag_number, const BACNET_AUTHENTICATION_FACTOR_FORMAT *aff); + +BACNET_STACK_EXPORT +int bacnet_authentication_factor_format_decode( + const uint8_t *apdu, + size_t apdu_size, + BACNET_AUTHENTICATION_FACTOR_FORMAT *data); +BACNET_STACK_DEPRECATED( + "Use bacnet_authentication_factor_format_decode() instead") BACNET_STACK_EXPORT int bacapp_decode_authentication_factor_format( const uint8_t *apdu, BACNET_AUTHENTICATION_FACTOR_FORMAT *aff); + +int bacnet_authentication_factor_format_context_decode( + const uint8_t *apdu, + size_t apdu_size, + uint8_t tag, + BACNET_AUTHENTICATION_FACTOR_FORMAT *data); +BACNET_STACK_DEPRECATED( + "Use bacnet_authentication_factor_format_context_decode() instead") BACNET_STACK_EXPORT int bacapp_decode_context_authentication_factor_format( const uint8_t *apdu, diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index d5845a28..6b07cb04 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -75,6 +75,7 @@ list(APPEND testdirs bacnet/access_rule bacnet/alarm_ack bacnet/arf + bacnet/authentication_factor_format bacnet/awf bacnet/bacaddr bacnet/bacapp diff --git a/test/bacnet/authentication_factor_format/CMakeLists.txt b/test/bacnet/authentication_factor_format/CMakeLists.txt new file mode 100644 index 00000000..aec16a4c --- /dev/null +++ b/test/bacnet/authentication_factor_format/CMakeLists.txt @@ -0,0 +1,68 @@ +# SPDX-License-Identifier: MIT + +cmake_minimum_required(VERSION 3.10 FATAL_ERROR) + +get_filename_component(basename ${CMAKE_CURRENT_SOURCE_DIR} NAME) +project(test_${basename} + VERSION 1.0.0 + LANGUAGES C) + + +string(REGEX REPLACE + "/test/bacnet/[a-zA-Z_/-]*$" + "/src" + SRC_DIR + ${CMAKE_CURRENT_SOURCE_DIR}) +string(REGEX REPLACE + "/test/bacnet/[a-zA-Z_/-]*$" + "/test" + TST_DIR + ${CMAKE_CURRENT_SOURCE_DIR}) +set(ZTST_DIR "${TST_DIR}/ztest/src") + +add_compile_definitions( + BIG_ENDIAN=0 + CONFIG_ZTEST=1 + MAX_APDU=50 + BACAPP_MINIMAL=1 +) + +include_directories( + ${SRC_DIR} + ${TST_DIR}/ztest/include + ) + +add_executable(${PROJECT_NAME} + # File(s) under test + ${SRC_DIR}/bacnet/authentication_factor_format.c + # Support files and stubs (pathname alphabetical) + ${SRC_DIR}/bacnet/bacaction.c + ${SRC_DIR}/bacnet/bacaddr.c + ${SRC_DIR}/bacnet/bacapp.c + ${SRC_DIR}/bacnet/bacdcode.c + ${SRC_DIR}/bacnet/bacdest.c + ${SRC_DIR}/bacnet/bacint.c + ${SRC_DIR}/bacnet/baclog.c + ${SRC_DIR}/bacnet/bacreal.c + ${SRC_DIR}/bacnet/bacstr.c + ${SRC_DIR}/bacnet/bactext.c + ${SRC_DIR}/bacnet/bacdevobjpropref.c + ${SRC_DIR}/bacnet/basic/sys/bigend.c + ${SRC_DIR}/bacnet/datetime.c + ${SRC_DIR}/bacnet/basic/sys/days.c + ${SRC_DIR}/bacnet/indtext.c + ${SRC_DIR}/bacnet/hostnport.c + ${SRC_DIR}/bacnet/lighting.c + ${SRC_DIR}/bacnet/timer_value.c + ${SRC_DIR}/bacnet/timestamp.c + ${SRC_DIR}/bacnet/weeklyschedule.c + ${SRC_DIR}/bacnet/bactimevalue.c + ${SRC_DIR}/bacnet/dailyschedule.c + ${SRC_DIR}/bacnet/calendar_entry.c + ${SRC_DIR}/bacnet/special_event.c + ${SRC_DIR}/bacnet/channel_value.c + # Test and test library files + ./src/main.c + ${ZTST_DIR}/ztest_mock.c + ${ZTST_DIR}/ztest.c + ) diff --git a/test/bacnet/authentication_factor_format/src/main.c b/test/bacnet/authentication_factor_format/src/main.c new file mode 100644 index 00000000..3657926a --- /dev/null +++ b/test/bacnet/authentication_factor_format/src/main.c @@ -0,0 +1,133 @@ +/** + * @file + * @brief Unit test for BACnetAccessRule encode and decode API + * @author Steve Karg + * @date September 2024 + * @copyright SPDX-License-Identifier: MIT + */ +#include +#include +#include + +/** + * @addtogroup bacnet_tests + * @{ + */ + +/** + * @brief Test + */ +static void test_authentication_factor_format_context( + uint8_t tag, BACNET_AUTHENTICATION_FACTOR_FORMAT *data) +{ + BACNET_AUTHENTICATION_FACTOR_FORMAT test_data = { 0 }; + uint8_t apdu[MAX_APDU]; + int apdu_len; + int test_len; + int null_len; + null_len = + bacapp_encode_context_authentication_factor_format(NULL, tag, data); + apdu_len = + bacapp_encode_context_authentication_factor_format(apdu, tag, data); + zassert_equal(null_len, apdu_len, NULL); + test_len = bacnet_authentication_factor_format_context_decode( + apdu, apdu_len, tag, &test_data); + zassert_equal( + test_len, apdu_len, "test_len=%d apdu_len=%d", test_len, apdu_len); + zassert_equal(data->format_type, test_data.format_type, "format_type"); + if (data->format_type == AUTHENTICATION_FACTOR_CUSTOM) { + zassert_equal( + data->vendor_format, test_data.vendor_format, "vendor_format"); + zassert_equal(data->vendor_id, test_data.vendor_id, "vendor_id"); + } + /* test the deprecated function */ + test_len = bacapp_decode_context_authentication_factor_format( + apdu, tag, &test_data); + zassert_equal( + test_len, apdu_len, "test_len=%d apdu_len=%d", test_len, apdu_len); + /* shrink the buffer, get some errors */ + while (apdu_len) { + apdu_len--; + test_len = bacnet_authentication_factor_format_context_decode( + apdu, apdu_len, tag, &test_data); + zassert_true( + test_len < 0, "test_len=%d apdu_len=%d", test_len, apdu_len); + } +} + +static void test_authentication_factor_format_positive( + BACNET_AUTHENTICATION_FACTOR_FORMAT *data) +{ + BACNET_AUTHENTICATION_FACTOR_FORMAT test_data = { 0 }; + uint8_t apdu[MAX_APDU]; + int apdu_len; + int test_len; + int null_len; + + null_len = bacapp_encode_authentication_factor_format(NULL, data); + apdu_len = bacapp_encode_authentication_factor_format(apdu, data); + zassert_equal(null_len, apdu_len, NULL); + test_len = + bacnet_authentication_factor_format_decode(apdu, apdu_len, &test_data); + zassert_equal( + test_len, apdu_len, "test_len=%d apdu_len=%d", test_len, apdu_len); + zassert_equal(data->format_type, test_data.format_type, "format_type"); + if (data->format_type == AUTHENTICATION_FACTOR_CUSTOM) { + zassert_equal( + data->vendor_format, test_data.vendor_format, "vendor_format"); + zassert_equal(data->vendor_id, test_data.vendor_id, "vendor_id"); + } + /* test the deprecated function */ + test_len = bacapp_decode_authentication_factor_format(apdu, &test_data); + zassert_equal( + test_len, apdu_len, "test_len=%d apdu_len=%d", test_len, apdu_len); + /* shrink the buffer, get some errors */ + while (apdu_len) { + apdu_len--; + test_len = bacnet_authentication_factor_format_decode( + apdu, apdu_len, &test_data); + zassert_true( + test_len < 0, "test_len=%d apdu_len=%d", test_len, apdu_len); + } +} + +/** + * @brief Test + */ +#if defined(CONFIG_ZTEST_NEW_API) +ZTEST(authentication_factor_format_tests, test_authentication_factor_format) +#else +static void test_authentication_factor_format(void) +#endif +{ + BACNET_AUTHENTICATION_FACTOR_FORMAT data = { 0 }; + + data.format_type = AUTHENTICATION_FACTOR_CUSTOM; + data.vendor_id = 1; + data.vendor_format = 2; + test_authentication_factor_format_positive(&data); + test_authentication_factor_format_context(1, &data); + + data.format_type = AUTHENTICATION_FACTOR_UNDEFINED; + data.vendor_id = 1; + data.vendor_format = 2; + test_authentication_factor_format_positive(&data); + test_authentication_factor_format_context(1, &data); +} + +/** + * @} + */ + +#if defined(CONFIG_ZTEST_NEW_API) +ZTEST_SUITE(authentication_factor_format_tests, NULL, NULL, NULL, NULL, NULL); +#else +void test_main(void) +{ + ztest_test_suite( + authentication_factor_format_tests, + ztest_unit_test(test_authentication_factor_format)); + + ztest_run_test_suite(authentication_factor_format_tests); +} +#endif