From 578f5071259b7d13122c5e5f0dcfca740e10bedb Mon Sep 17 00:00:00 2001 From: Steve Karg Date: Fri, 20 Feb 2026 10:30:46 -0600 Subject: [PATCH] Secure the BVLC decoders by replacing deprecated primitive and complex data decoders. (#1241) --- CHANGELOG.md | 1 + src/bacnet/datalink/bvlc.c | 123 +++++++-------------------- test/bacnet/datalink/bvlc/src/main.c | 14 +++ 3 files changed, 44 insertions(+), 94 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06b7276b..5a89bc3a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/bacnet/datalink/bvlc.c b/src/bacnet/datalink/bvlc.c index 151fd0b6..bcbd8f66 100644 --- a/src/bacnet/datalink/bvlc.c +++ b/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; - BACNET_OCTET_STRING octet_string = { 0 }; + int len = 0, apdu_len = 0; BACNET_IP_BROADCAST_DISTRIBUTION_TABLE_ENTRY *bdt_entry = NULL; - uint8_t tag_number = 0; - uint32_t len_value_type = 0; - BACNET_UNSIGNED_INTEGER unsigned_value = 0; + BACNET_HOST_N_PORT_MINIMAL host_n_port = { 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 */ - if (!decode_is_opening_tag_number(&apdu[len++], 0)) { - if (error_code) { - *error_code = ERROR_CODE_REJECT_INVALID_TAG; + len = host_n_port_minimal_context_decode( + &apdu[apdu_len], apdu_size - apdu_len, 0, error_code, &host_n_port); + if (len > 0) { + 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( - &apdu[len], &tag_number, &len_value_type); - if (tag_number != 1) { + len = bacnet_octet_string_buffer_context_decode( + &apdu[apdu_len], apdu_size - apdu_len, 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; } diff --git a/test/bacnet/datalink/bvlc/src/main.c b/test/bacnet/datalink/bvlc/src/main.c index 9dd51599..31d4f27e 100644 --- a/test/bacnet/datalink/bvlc/src/main.c +++ b/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);