From acb3b0f57d47cbfe37626e0a4bb70f2a34934311 Mon Sep 17 00:00:00 2001 From: anthony-crystalpeak <108027248+anthony-crystalpeak@users.noreply.github.com> Date: Mon, 31 Jul 2023 14:50:14 -0700 Subject: [PATCH] [libbacnet]: bacapp_data_len() + bacnet_tag_number_and_value_decode() (#453) * Bugfix: bacapp_data_len() + bacnet_tag_number_and_value_decode() * Fix bacnet_tag_number_and_value_decode function. Add read buffer safe functions for opening, closing, context specific tags. Fix bacapp_data_len function changes as indicated by unit testing. * refactor bacapp_data_len --------- Co-authored-by: Steve Karg --- src/bacnet/bacapp.c | 91 +++++++++++++++++++++++---------------- src/bacnet/bacdcode.c | 80 ++++++++++++++++++++++++++++++---- src/bacnet/bacdcode.h | 7 +++ test/bacnet/wp/src/main.c | 4 +- 4 files changed, 134 insertions(+), 48 deletions(-) diff --git a/src/bacnet/bacapp.c b/src/bacnet/bacapp.c index a123541a..b8e0e8c6 100644 --- a/src/bacnet/bacapp.c +++ b/src/bacnet/bacapp.c @@ -1520,54 +1520,71 @@ int bacapp_data_len( uint8_t opening_tag_number = 0; uint8_t opening_tag_number_counter = 0; uint32_t value = 0; + bool total_len_enable = false; - if (IS_OPENING_TAG(apdu[0])) { - len = bacnet_tag_number_and_value_decode( - &apdu[apdu_len], apdu_len_max - apdu_len, &tag_number, &value); - apdu_len += len; - opening_tag_number = tag_number; - opening_tag_number_counter = 1; - while (opening_tag_number_counter) { - if (IS_OPENING_TAG(apdu[apdu_len])) { - len = bacnet_tag_number_and_value_decode(&apdu[apdu_len], - apdu_len_max - apdu_len, &tag_number, &value); - if (tag_number == opening_tag_number) { - opening_tag_number_counter++; - } - } else if (IS_CLOSING_TAG(apdu[apdu_len])) { - len = bacnet_tag_number_and_value_decode(&apdu[apdu_len], - apdu_len_max - apdu_len, &tag_number, &value); - if (tag_number == opening_tag_number) { + if (!apdu) { + return BACNET_STATUS_ERROR; + } + if (apdu_len_max <= apdu_len) { + /* error: exceeding our buffer limit */ + return BACNET_STATUS_ERROR; + } + if (!bacnet_is_opening_tag(apdu, apdu_len_max)) { + /* error: opening tag is missing */ + return BACNET_STATUS_ERROR; + } + do { + if (bacnet_is_opening_tag(apdu, apdu_len_max)) { + len = bacnet_tag_number_and_value_decode(apdu, + apdu_len_max - apdu_len, &tag_number, &value); + if (opening_tag_number_counter == 0) { + opening_tag_number = tag_number; + opening_tag_number_counter = 1; + total_len_enable = false; + } else if (tag_number == opening_tag_number) { + total_len_enable = true; + opening_tag_number_counter++; + } + } else if (bacnet_is_closing_tag(apdu, apdu_len_max)) { + len = bacnet_tag_number_and_value_decode(apdu, + apdu_len_max - apdu_len, &tag_number, &value); + if (tag_number == opening_tag_number) { + if (opening_tag_number_counter > 0) { opening_tag_number_counter--; } - } else if (IS_CONTEXT_SPECIFIC(apdu[apdu_len])) { + } + total_len_enable = true; + } else if (bacnet_is_context_specific(apdu, apdu_len_max)) { #if defined(BACAPP_TYPES_EXTRA) - /* context-specific tagged data */ - len = bacapp_decode_context_data_len( - &apdu[apdu_len], apdu_len_max - apdu_len, property); + /* context-specific tagged data */ + len = bacapp_decode_context_data_len( + apdu, apdu_len_max - apdu_len, property); + total_len_enable = true; #endif + } else { + /* application tagged data */ + len = bacapp_decode_application_data_len( + apdu, apdu_len_max - apdu_len); + total_len_enable = true; + } + if (opening_tag_number_counter > 0) { + if (len > 0) { + if (total_len_enable) { + total_len += len; + } } else { - /* application tagged data */ - len = bacapp_decode_application_data_len( - &apdu[apdu_len], apdu_len_max - apdu_len); + /* error: len is not incrementing */ + return BACNET_STATUS_ERROR; } apdu_len += len; - if (opening_tag_number_counter) { - if (len > 0) { - total_len += len; - } else { - /* error: len is not incrementing */ - total_len = BACNET_STATUS_ERROR; - break; - } - } - if ((unsigned)apdu_len > apdu_len_max) { + if (apdu_len_max <= apdu_len) { /* error: exceeding our buffer limit */ - total_len = BACNET_STATUS_ERROR; - break; + return BACNET_STATUS_ERROR; } + apdu += len; } - } + } while (opening_tag_number_counter > 0); + return total_len; } diff --git a/src/bacnet/bacdcode.c b/src/bacnet/bacdcode.c index 588b8e46..9a6f3c20 100644 --- a/src/bacnet/bacdcode.c +++ b/src/bacnet/bacdcode.c @@ -457,6 +457,27 @@ bool decode_is_opening_tag(uint8_t *apdu) return (bool)((apdu[0] & 0x07) == 6); } +/** + * @brief Returns true if an opening tag has been found. + * + * @param apdu Pointer to the tag number. + * @param apdu_size Number of bytes available to decode + * + * @return true if an opening tag has been found. + */ +bool bacnet_is_opening_tag(uint8_t *apdu, uint32_t apdu_size) +{ + bool tag = false; + + if (apdu_size > 0) { + if (IS_OPENING_TAG(apdu[0])) { + tag = true; + } + } + + return tag; +} + /** * @brief Returns if at the given pointer a * closing tag has been found. @@ -470,6 +491,49 @@ bool decode_is_closing_tag(uint8_t *apdu) return (bool)((apdu[0] & 0x07) == 7); } +/** + * @brief Returns true if a closing tag has been found. + * + * @param apdu Pointer to the tag number. + * @param apdu_size Number of bytes available to decode + * + * @return true if a closing tag has been found. + */ +bool bacnet_is_closing_tag(uint8_t *apdu, uint32_t apdu_size) +{ + bool tag = false; + + if (apdu_size > 0) { + if (IS_CLOSING_TAG(apdu[0])) { + tag = true; + } + } + + return tag; +} + +/** + * @brief Returns true if a context specific tag has been found. + * + * @param apdu Pointer to the tag number. + * @param apdu_size Number of bytes available to decode + * + * @return true if a context specific tag has been found. + */ +bool bacnet_is_context_specific(uint8_t *apdu, uint32_t apdu_size) +{ + bool tag = false; + + if (apdu_size > 0) { + if (IS_CONTEXT_SPECIFIC(apdu[0])) { + tag = true; + } + } + + return tag; +} + + /** * @brief Decodes the tag number and the value, * that the APDU pointer is addressing. @@ -549,28 +613,26 @@ int bacnet_tag_number_and_value_decode( len = bacnet_tag_number_decode(&apdu[0], apdu_len_max, tag_number); if (len > 0) { - apdu_len_max -= len; - if (IS_EXTENDED_VALUE(apdu[0])) { - /* tagged as uint32_t */ + if (IS_EXTENDED_VALUE(apdu[0]) && (apdu_len_max > len)) { + apdu_len_max -= len; if ((apdu[len] == 255) && (apdu_len_max >= 5)) { + /* tagged as uint32_t */ uint32_t value32; len++; len += decode_unsigned32(&apdu[len], &value32); if (value) { *value = value32; } - } - /* tagged as uint16_t */ - else if ((apdu[len] == 254) && (apdu_len_max >= 3)) { + } else if ((apdu[len] == 254) && (apdu_len_max >= 3)) { + /* tagged as uint16_t */ uint16_t value16; len++; len += decode_unsigned16(&apdu[len], &value16); if (value) { *value = value16; } - } - /* no tag - must be uint8_t */ - else if ((apdu[len] < 254) && (apdu_len_max >= 1)) { + } else if ((apdu[len] < 254) && (apdu_len_max >= 1)) { + /* no tag - must be uint8_t */ if (value) { *value = apdu[len]; } diff --git a/src/bacnet/bacdcode.h b/src/bacnet/bacdcode.h index 61707b37..a6851e88 100644 --- a/src/bacnet/bacdcode.h +++ b/src/bacnet/bacdcode.h @@ -61,6 +61,13 @@ extern "C" { bool context_specific, uint32_t len_value_type); +BACNET_STACK_EXPORT +bool bacnet_is_opening_tag(uint8_t *apdu, uint32_t apdu_size); +BACNET_STACK_EXPORT +bool bacnet_is_closing_tag(uint8_t *apdu, uint32_t apdu_size); +BACNET_STACK_EXPORT +bool bacnet_is_context_specific(uint8_t *apdu, uint32_t apdu_size); + /* from clause 20.2.1.3.2 Constructed Data */ /* returns the number of apdu bytes consumed */ BACNET_STACK_EXPORT diff --git a/test/bacnet/wp/src/main.c b/test/bacnet/wp/src/main.c index 97f068a8..2a6a8d45 100644 --- a/test/bacnet/wp/src/main.c +++ b/test/bacnet/wp/src/main.c @@ -61,11 +61,11 @@ static void testWritePropertyTag(BACNET_APPLICATION_DATA_VALUE *value) wpdata.application_data_len = bacapp_encode_application_data(&wpdata.application_data[0], value); len = wp_encode_apdu(&apdu[0], invoke_id, &wpdata); - zassert_not_equal(len, 0, NULL); + zassert_not_equal(len, 0, "len=%d", len); /* decode the data */ apdu_len = len; len = wp_decode_apdu(&apdu[0], apdu_len, &test_invoke_id, &test_data); - zassert_not_equal(len, -1, NULL); + zassert_true(len > 0, "len=%d", len); zassert_equal(test_data.object_type, wpdata.object_type, NULL); zassert_equal(test_data.object_instance, wpdata.object_instance, NULL); zassert_equal(test_data.object_property, wpdata.object_property, NULL);