Secure the BVLC decoders by replacing deprecated primitive and complex data decoders. (#1241)

CHANGELOG.md

@@ -16,6 +16,7 @@ The git repositories are hosted at the following sites:
 
 ### Security
 
+* Secured BVLC decoder by replacing deprecated primitive decoder usage. (#1241)
 * Secured decoding length underflow in wp_decode_service_request() and
   bacnet_action_command_decode() which had similar issue. (#1231)
 * Secured Schedule_Weekly_Schedule_Set() the example schedule object

src/bacnet/datalink/bvlc.c

@@ -670,129 +670,64 @@ int bvlc_broadcast_distribution_table_encode(
 /**
  * @brief Decode the Broadcast-Distribution-Table for Network Port object
  * @param apdu - the APDU buffer
- * @param apdu_len - the APDU buffer length
+ * @param apdu_size - the APDU buffer length
  * @param bdt_head - head of a BDT linked list
  * @return length of the APDU buffer decoded, or ERROR, REJECT, or ABORT
  */
 int bvlc_broadcast_distribution_table_decode(
     const uint8_t *apdu,
-    uint16_t apdu_len,
+    uint16_t apdu_size,
     BACNET_ERROR_CODE *error_code,
     BACNET_IP_BROADCAST_DISTRIBUTION_TABLE_ENTRY *bdt_head)
 {
-    int len = 0;
+    int len = 0, apdu_len = 0;
-    BACNET_OCTET_STRING octet_string = { 0 };
     BACNET_IP_BROADCAST_DISTRIBUTION_TABLE_ENTRY *bdt_entry = NULL;
-    uint8_t tag_number = 0;
+    BACNET_HOST_N_PORT_MINIMAL host_n_port = { 0 };
-    uint32_t len_value_type = 0;
-    BACNET_UNSIGNED_INTEGER unsigned_value = 0;
 
     /* default reject code */
     if (error_code) {
         *error_code = ERROR_CODE_REJECT_MISSING_REQUIRED_PARAMETER;
     }
     /* check for value pointers */
-    if ((apdu_len == 0) || (!apdu)) {
+    if ((apdu_size == 0) || (!apdu)) {
         return BACNET_STATUS_REJECT;
     }
     bdt_entry = bdt_head;
     while (bdt_entry) {
-        /* bbmd-address [0] BACnetHostNPort - opening */
+        len = host_n_port_minimal_context_decode(
-        if (!decode_is_opening_tag_number(&apdu[len++], 0)) {
+            &apdu[apdu_len], apdu_size - apdu_len, 0, error_code, &host_n_port);
-            if (error_code) {
+        if (len > 0) {
-                *error_code = ERROR_CODE_REJECT_INVALID_TAG;
+            apdu_len += len;
+            bdt_entry->dest_address.port = host_n_port.port;
+            if (host_n_port.tag == BACNET_HOST_ADDRESS_TAG_IP_ADDRESS) {
+                bdt_entry->valid = true;
+                bdt_entry->dest_address.address[0] =
+                    host_n_port.host.ip_address.address[0];
+                bdt_entry->dest_address.address[1] =
+                    host_n_port.host.ip_address.address[1];
+                bdt_entry->dest_address.address[2] =
+                    host_n_port.host.ip_address.address[2];
+                bdt_entry->dest_address.address[3] =
+                    host_n_port.host.ip_address.address[3];
+            } else {
+                bdt_entry->valid = false;
             }
-            return BACNET_STATUS_REJECT;
-        }
-        if (len > apdu_len) {
-            return BACNET_STATUS_REJECT;
-        }
-        /* host [0] BACnetHostAddress - opening */
-        if (!decode_is_opening_tag_number(&apdu[len++], 0)) {
-            if (error_code) {
-                *error_code = ERROR_CODE_REJECT_INVALID_TAG;
-            }
-            return BACNET_STATUS_REJECT;
-        }
-        if (len > apdu_len) {
-            return BACNET_STATUS_REJECT;
-        }
-        /* CHOICE - ip-address [1] OCTET STRING */
-        len += decode_tag_number_and_value(
-            &apdu[len], &tag_number, &len_value_type);
-        if (tag_number != 1) {
-            if (error_code) {
-                *error_code = ERROR_CODE_REJECT_INVALID_TAG;
-            }
-            return BACNET_STATUS_REJECT;
-        }
-        len += decode_octet_string(&apdu[len], len_value_type, &octet_string);
-        if (len > apdu_len) {
-            return BACNET_STATUS_REJECT;
-        }
-        (void)octetstring_copy_value(
-            &bdt_entry->dest_address.address[0], IP_ADDRESS_MAX, &octet_string);
-        /*  host [0] BACnetHostAddress - closing */
-        if (!decode_is_closing_tag_number(&apdu[len++], 0)) {
-            if (error_code) {
-                *error_code = ERROR_CODE_REJECT_INVALID_TAG;
-            }
-            return BACNET_STATUS_REJECT;
-        }
-        if (len > apdu_len) {
-            return BACNET_STATUS_REJECT;
-        }
-        /* port [1] Unsigned16 */
-        len += decode_tag_number_and_value(
-            &apdu[len], &tag_number, &len_value_type);
-        if (tag_number != 1) {
-            if (error_code) {
-                *error_code = ERROR_CODE_REJECT_INVALID_TAG;
-            }
-            return BACNET_STATUS_REJECT;
-        }
-        len += decode_unsigned(&apdu[len], len_value_type, &unsigned_value);
-        if (len > apdu_len) {
-            return BACNET_STATUS_REJECT;
-        }
-        if (unsigned_value <= UINT16_MAX) {
-            bdt_entry->dest_address.port = unsigned_value;
         } else {
-            if (error_code) {
-                *error_code = ERROR_CODE_REJECT_PARAMETER_OUT_OF_RANGE;
-            }
-            return BACNET_STATUS_REJECT;
-        }
-        /* bbmd-address [0] BACnetHostNPort - closing */
-        if (!decode_is_closing_tag_number(&apdu[len++], 0)) {
-            if (error_code) {
-                *error_code = ERROR_CODE_REJECT_INVALID_TAG;
-            }
-            return BACNET_STATUS_REJECT;
-        }
-        if (len > apdu_len) {
             return BACNET_STATUS_REJECT;
         }
         /* broadcast-mask [1] OCTET STRING */
-        len += decode_tag_number_and_value(
+        len = bacnet_octet_string_buffer_context_decode(
-            &apdu[len], &tag_number, &len_value_type);
+            &apdu[apdu_len], apdu_size - apdu_len, 1,
-        if (tag_number != 1) {
+            bdt_entry->broadcast_mask.address,
+            sizeof(bdt_entry->broadcast_mask.address));
+        if (len > 0) {
+            apdu_len += len;
+        } else {
             if (error_code) {
                 *error_code = ERROR_CODE_REJECT_INVALID_TAG;
             }
             return BACNET_STATUS_REJECT;
         }
-        if (len > apdu_len) {
-            return BACNET_STATUS_REJECT;
-        }
-        len += decode_octet_string(&apdu[len], len_value_type, &octet_string);
-        if (len > apdu_len) {
-            return BACNET_STATUS_REJECT;
-        }
-        (void)octetstring_copy_value(
-            &bdt_entry->broadcast_mask.address[0], IP_ADDRESS_MAX,
-            &octet_string);
-        bdt_entry->valid = true;
         /* next entry */
         bdt_entry = bdt_entry->next;
     }

test/bacnet/datalink/bvlc/src/main.c

@@ -520,6 +520,9 @@ static void test_BVLC_Broadcast_Distribution_Table_Encode(void)
         status = bvlc_broadcast_distribution_table_entry_append(
             &bdt_list[0], &bdt_entry);
         zassert_true(status, NULL);
+        status = bvlc_broadcast_distribution_table_entry_insert(
+            &bdt_list[0], &bdt_entry, i);
+        zassert_true(status, NULL);
     }
     test_count = bvlc_broadcast_distribution_table_count(&bdt_list[0]);
     if (test_count != count) {
@@ -698,6 +701,7 @@ static int test_BVLC_Foreign_Device_Table_Setup(
     uint16_t ttl_seconds = 12345;
     bool status = false;
     BACNET_IP_ADDRESS dest_address = { 0 };
+    BACNET_IP_FOREIGN_DEVICE_TABLE_ENTRY test_entry = { 0 };
 
     status = bvlc_address_from_ascii(&dest_address, "192.168.0.1");
     zassert_true(status, NULL);
@@ -712,6 +716,12 @@ static int test_BVLC_Foreign_Device_Table_Setup(
         status = bvlc_foreign_device_table_entry_add(
             fdt_list, &dest_address, ttl_seconds);
         zassert_true(status, NULL);
+        bvlc_address_copy(&test_entry.dest_address, &dest_address);
+        test_entry.ttl_seconds = ttl_seconds;
+        test_entry.ttl_seconds_remaining = ttl_seconds + 30;
+        status =
+            bvlc_foreign_device_table_entry_insert(fdt_list, &test_entry, i);
+        zassert_true(status, NULL);
     }
     test_count = bvlc_foreign_device_table_count(fdt_list);
     zassert_equal(test_count, count, NULL);
@@ -733,6 +743,7 @@ static void test_BVLC_Read_Foreign_Device_Table_Ack(void)
     uint16_t count = 0;
     uint16_t test_count = 0;
     BACNET_IP_FOREIGN_DEVICE_TABLE_ENTRY fdt_list[5] = { 0 };
+    bool status = false;
 
     count = sizeof(fdt_list) / sizeof(fdt_list[0]);
     test_count = test_BVLC_Foreign_Device_Table_Setup(fdt_list, count);
@@ -744,6 +755,9 @@ static void test_BVLC_Read_Foreign_Device_Table_Ack(void)
     zassert_equal(test_count, count, NULL);
     test_BVLC_Read_Foreign_Device_Table_Ack_Message(fdt_list);
     /* cleanup */
+    status = bvlc_foreign_device_table_entry_delete(
+        fdt_list, &fdt_list[0].dest_address);
+    zassert_true(status, NULL);
     bvlc_foreign_device_table_valid_clear(&fdt_list[0]);
     test_count = bvlc_foreign_device_table_valid_count(fdt_list);
     zassert_equal(test_count, 0, NULL);