Secured BACnetAuthenticationFactorFormat decoder and added unit testing (#1127)

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

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);
+    len = encode_context_enumerated(apdu, 0, data->format_type);
-    if (len < 0) {
+    apdu_len += len;
-        return -1;
+    if (apdu) {
-    } else {
+        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;
-
+    if (apdu) {
-    len = bacapp_encode_authentication_factor_format(&apdu[apdu_len], aff);
+        apdu += len;
+    }
+    len = bacapp_encode_authentication_factor_format(apdu, data);
     apdu_len += len;
-
+    if (apdu) {
-    len = encode_closing_tag(&apdu[apdu_len], tag);
+        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)) {
+    /* format-type[0] BACnetAuthenticationFactorType */
-        len = decode_context_enumerated(&apdu[apdu_len], 0, &format_type);
+    len = bacnet_enumerated_context_decode(
-        if (len < 0) {
+        &apdu[apdu_len], apdu_size - apdu_len, 0, &enum_value);
-            return -1;
+    if (len > 0) {
-        } else if (format_type < AUTHENTICATION_FACTOR_MAX) {
+        apdu_len += len;
-            apdu_len += len;
+        if (enum_value > AUTHENTICATION_FACTOR_MAX) {
-            aff->format_type = (BACNET_AUTHENTICATION_FACTOR_TYPE)format_type;
+            enum_value = AUTHENTICATION_FACTOR_MAX;
-        } else {
+        }
-            /* FIXME: Maybe this should return BACNET_STATUS_REJECT */
+        format_type = (BACNET_AUTHENTICATION_FACTOR_TYPE)enum_value;
-            return -1;
+        if (data) {
+            data->format_type = format_type;
         }
     } else {
-        return -1;
+        return BACNET_STATUS_ERROR;
     }
-
+    if (format_type == AUTHENTICATION_FACTOR_CUSTOM) {
-    if (decode_is_context_tag(&apdu[apdu_len], 1)) {
+        /*  optional fields are required when Format-Type
-        len = decode_context_unsigned(&apdu[apdu_len], 1, &unsigned_value);
+            field has a value of CUSTOM. */
-        if (len < 0) {
+        /* vendor-id[1] Unsigned16 OPTIONAL */
-            return -1;
+        len = bacnet_unsigned_context_decode(
-        } else {
+            &apdu[apdu_len], apdu_size - apdu_len, 1, &unsigned_value);
-            aff->vendor_id = unsigned_value;
+        if (len > 0) {
             apdu_len += len;
-        }
+            if (unsigned_value > UINT16_MAX) {
-        if ((aff->format_type != AUTHENTICATION_FACTOR_CUSTOM) &&
+                return BACNET_STATUS_ERROR;
-            (aff->vendor_id != 0)) {
+            }
-            return -1;
+            if (data) {
-        }
+                data->vendor_id = (uint16_t)unsigned_value;
-    }
+            }
-
-    if (decode_is_context_tag(&apdu[apdu_len], 2)) {
-        len = decode_context_unsigned(&apdu[apdu_len], 2, &unsigned_value);
-        if (len < 0) {
-            return -1;
         } else {
-            aff->vendor_format = unsigned_value;
+            return BACNET_STATUS_ERROR;
-            apdu_len += len;
         }
-        if ((aff->format_type != AUTHENTICATION_FACTOR_CUSTOM) &&
+        /* vendor-format[2] Unsigned16 OPTIONAL */
-            (aff->vendor_format != 0)) {
+        len = bacnet_unsigned_context_decode(
-            return -1;
+            &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;
+    return bacnet_authentication_factor_format_decode(apdu, MAX_APDU, data);
-    int section_length;
+}
 
-    if (decode_is_opening_tag_number(&apdu[len], tag)) {
+/**
-        len++;
+ * @brief Decode a context tagged BACnetAuthenticationFactorFormat property
-        section_length =
+ *  value
-            bacapp_decode_authentication_factor_format(&apdu[len], aff);
+ * @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) {
+    if (bacnet_is_opening_tag_number(
-            len = -1;
+            &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;
+            return BACNET_STATUS_ERROR;
-            if (decode_is_closing_tag_number(&apdu[len], tag)) {
+        }
-                len++;
+        if (bacnet_is_closing_tag_number(
-            } else {
+                &apdu[apdu_len], apdu_size - apdu_len, tag, &len)) {
-                len = -1;
+            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);
 }

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,

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

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
+    )

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 <skarg@users.sourceforge.net>
+ * @date September 2024
+ * @copyright SPDX-License-Identifier: MIT
+ */
+#include <zephyr/ztest.h>
+#include <bacnet/authentication_factor_format.h>
+#include <bacnet/bacdcode.h>
+
+/**
+ * @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