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 <skarg@users.sourceforge.net>

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

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 */
+    /* From 16.4.1.1.2 Password
-    if (characterstring_ansi_same(&rd_data->password, Reinit_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:

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_NPDU_DATA npdu_data = { 0 };
-    BACNET_ADDRESS my_address;
+    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",
+        fprintf(stderr, "ReinitializeDevice: state=%u password=%*s\n",
-            (unsigned)rd_data.state, characterstring_value(&rd_data.password));
+            (unsigned)rd_data.state,
+            (int)characterstring_length(&rd_data.password),
+            characterstring_value(&rd_data.password));
     } else {
         fprintf(stderr, "ReinitializeDevice: Unable to decode request!\n");
     }

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);
+    len = 4;
-        apdu_len += len;
+    apdu_len += len;
-        /* optional password */
+    if (apdu) {
-        if (password) {
+        apdu += len;
-            /* Must be at least 1 character, limited to 20 characters */
+    }
-            if ((password->length >= 1) && (password->length <= 20)) {
+    len = encode_context_enumerated(apdu, 0, state);
-                len = encode_context_character_string(
+    apdu_len += len;
-                    &apdu[apdu_len], 1, password);
+    if (apdu) {
-                apdu_len += len;
+        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;
+    int len = 0, apdu_len = 0;
-    uint8_t tag_number = 0;
-    uint32_t len_value_type = 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)) {
+        len = bacnet_enumerated_context_decode(
-            return -1;
+            &apdu[apdu_len], apdu_size - apdu_len, 0, &value);
-        }
+        if (len > 0) {
-        len += decode_tag_number_and_value(
+            if (state) {
-            &apdu[len], &tag_number, &len_value_type);
+                *state = (BACNET_REINITIALIZED_STATE)value;
-        len += decode_enumerated(&apdu[len], len_value_type, &value);
+            }
-        if (state) {
+            apdu_len = len;
-            *state = (BACNET_REINITIALIZED_STATE)value;
+        } else {
+            apdu_len = BACNET_STATUS_ERROR;
         }
         /* Tag 1: password - optional */
-        if (len < apdu_len) {
+        if (apdu_len < apdu_size) {
-            if (!decode_is_context_tag(&apdu[len], 1)) {
+            len = bacnet_character_string_context_decode(
-                return -1;
+                &apdu[apdu_len], apdu_size - apdu_len, 1, password);
-            }
+            if (len > 0) {
-            len += decode_tag_number_and_value(
+                apdu_len += len;
-                &apdu[len], &tag_number, &len_value_type);
+            } else {
-            if (len < apdu_len) {
+                apdu_len = BACNET_STATUS_ERROR;
-                if (password) {
-                    len += decode_character_string(
-                        &apdu[len], len_value_type, password);
-                }
             }
+        } else {
+            characterstring_init_ansi(password, "");
         }
     }
 
-    return (int)len;
+    return apdu_len;
 }

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 (invoke_id) {
-    if (apdu[3] != SERVICE_CONFIRMED_REINITIALIZE_DEVICE)
+        *invoke_id = apdu[2]; /* invoke id - filled in by net layer */
+    }
+    if (apdu[3] != SERVICE_CONFIRMED_REINITIALIZE_DEVICE) {
         return -1;
-    offset = 4;
+    }
-
+    apdu_len = 4;
-    if (apdu_len > offset) {
+    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 };
+    Test_ReinitializeDevice_Service(BACNET_REINIT_COLDSTART, "John 3:16");
-    int len = 0;
+    Test_ReinitializeDevice_Service(BACNET_REINIT_COLDSTART, "");
-    int apdu_len = 0;
+    Test_ReinitializeDevice_Service(BACNET_REINIT_WARMSTART, "Joshua95");
-    uint8_t invoke_id = 128;
+    Test_ReinitializeDevice_Service(BACNET_REINIT_WARMSTART, "");
-    uint8_t test_invoke_id = 0;
+    Test_ReinitializeDevice_Service(BACNET_REINIT_STARTBACKUP, "Mary98");
-    BACNET_REINITIALIZED_STATE state;
+    Test_ReinitializeDevice_Service(BACNET_REINIT_STARTBACKUP, "");
-    BACNET_REINITIALIZED_STATE test_state;
+    Test_ReinitializeDevice_Service(BACNET_REINIT_ENDBACKUP, "Anna99");
-    BACNET_CHARACTER_STRING password;
+    Test_ReinitializeDevice_Service(BACNET_REINIT_ENDBACKUP, "");
-    BACNET_CHARACTER_STRING test_password;
+    Test_ReinitializeDevice_Service(BACNET_REINIT_STARTRESTORE, "Chris04");
-
+    Test_ReinitializeDevice_Service(BACNET_REINIT_STARTRESTORE, "");
-    state = BACNET_REINIT_WARMSTART;
+    Test_ReinitializeDevice_Service(BACNET_REINIT_ENDRESTORE, "Steve66");
-    characterstring_init_ansi(&password, "John 3:16");
+    Test_ReinitializeDevice_Service(BACNET_REINIT_ENDRESTORE, "");
-    len = rd_encode_apdu(&apdu[0], invoke_id, state, &password);
+    Test_ReinitializeDevice_Service(BACNET_REINIT_ABORTRESTORE, "Patricia66");
-    zassert_not_equal(len, 0, NULL);
+    Test_ReinitializeDevice_Service(BACNET_REINIT_ABORTRESTORE, "");
-    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);
 
     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_test_suite(rd_tests, ztest_unit_test(test_ReinitializeDevice));
-     ztest_unit_test(test_ReinitializeDevice)
-     );
 
     ztest_run_test_suite(rd_tests);
 }