dali/bacnet_stack
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bugfix/fix write property decode deprecated (#499)

Browse Source 
* fixed WriteProperty to use secure decode. Improve unit testing.

---------

Co-authored-by: Steve Karg <skarg@users.sourceforge.net>
This commit is contained in:
Steve Karg
2023-09-16 10:23:08 -05:00
committed by GitHub
parent 75286cffe4
commit b0749572f6
5 changed files with 238 additions and 129 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/bacnet/bacapp.c
+1 -1
View File
@@ -1531,7 +1531,7 @@ bool bacapp_copy(BACNET_APPLICATION_DATA_VALUE *dest_value,
* @param apdu_len_max Bytes valid in the buffer * @param apdu_len_max Bytes valid in the buffer
* @param property ID of the property to get the length for. * @param property ID of the property to get the length for.
* *
* @return Length in bytes or BACNET_STATUS_ERROR. * @return Length in bytes 0..N, or BACNET_STATUS_ERROR.
*/ */
int bacapp_data_len( int bacapp_data_len(
uint8_t *apdu, unsigned apdu_len_max, BACNET_PROPERTY_ID property) uint8_t *apdu, unsigned apdu_len_max, BACNET_PROPERTY_ID property)
src/bacnet/wp.c
+164 -97
View File
@@ -40,9 +40,22 @@
/** @file wp.c Encode/Decode BACnet Write Property APDUs */ /** @file wp.c Encode/Decode BACnet Write Property APDUs */
#if BACNET_SVC_WP_A #if BACNET_SVC_WP_A
/** Initialize the APDU for encode service. /**
* @brief Initialize the APDU for encode service.
* *
* @param apdu Pointer to the buffer. * WriteProperty-Request ::= SEQUENCE {
* object-identifier [0] BACnetObjectIdentifier,
* property-identifier [1] BACnetPropertyIdentifier,
* property-array-index [2] Unsigned OPTIONAL,
* -- used only with array datatype
* -- if omitted with an array the entire
* -- array is referenced
* property-value [3] ABSTRACT-SYNTAX.&Type,
* priority [4] Unsigned (1..16) OPTIONAL
* -– used only when property is commandable
* }
*
* @param apdu Pointer to the buffer, or NULL for length
* @param invoke_id ID of service invoked. * @param invoke_id ID of service invoked.
* @param wpdata Pointer to the write property data. * @param wpdata Pointer to the write property data.
* *
@@ -55,66 +68,96 @@ int wp_encode_apdu(
int len = 0; /* total length of the apdu, return value */ int len = 0; /* total length of the apdu, return value */
int imax = 0; /* maximum application data length */ int imax = 0; /* maximum application data length */
if (!wpdata) {
return BACNET_STATUS_ERROR;
}
if (apdu) { if (apdu) {
apdu[0] = PDU_TYPE_CONFIRMED_SERVICE_REQUEST; apdu[0] = PDU_TYPE_CONFIRMED_SERVICE_REQUEST;
apdu[1] = encode_max_segs_max_apdu(0, MAX_APDU); apdu[1] = encode_max_segs_max_apdu(0, MAX_APDU);
apdu[2] = invoke_id; apdu[2] = invoke_id;
apdu[3] = SERVICE_CONFIRMED_WRITE_PROPERTY; /* service choice */ apdu[3] = SERVICE_CONFIRMED_WRITE_PROPERTY; /* service choice */
apdu_len = 4; }
len = encode_context_object_id( len = 4;
&apdu[apdu_len], 0, wpdata->object_type, wpdata->object_instance); apdu_len += len;
if (apdu) {
apdu += len;
}
len = encode_context_object_id(
apdu, 0, wpdata->object_type, wpdata->object_instance);
apdu_len += len;
if (apdu) {
apdu += len;
}
len = encode_context_enumerated(apdu, 1, wpdata->object_property);
apdu_len += len;
if (apdu) {
apdu += len;
}
/* optional array index; ALL is -1 which is assumed when missing */
if (wpdata->array_index != BACNET_ARRAY_ALL) {
len = encode_context_unsigned(apdu, 2, wpdata->array_index);
apdu_len += len; apdu_len += len;
len = encode_context_enumerated( if (apdu) {
&apdu[apdu_len], 1, wpdata->object_property); apdu += len;
}
}
/* propertyValue */
len = encode_opening_tag(apdu, 3);
apdu_len += len;
if (apdu) {
apdu += len;
}
for (len = 0; len < wpdata->application_data_len; len++) {
if (apdu) {
*apdu = wpdata->application_data[len];
apdu += 1;
}
apdu_len++;
}
len = encode_closing_tag(apdu, 3);
apdu_len += len;
if (apdu) {
apdu += len;
}
/* optional priority - 0 if not set, 1..16 if set */
if (wpdata->priority != BACNET_NO_PRIORITY) {
len = encode_context_unsigned(apdu, 4, wpdata->priority);
apdu_len += len; apdu_len += len;
/* optional array index; ALL is -1 which is assumed when missing */
if (wpdata->array_index != BACNET_ARRAY_ALL) {
len = encode_context_unsigned(
&apdu[apdu_len], 2, wpdata->array_index);
apdu_len += len;
}
/* propertyValue */
len = encode_opening_tag(&apdu[apdu_len], 3);
apdu_len += len;
imax = wpdata->application_data_len;
if (imax > (MAX_APDU - 2 /*closing*/ - apdu_len)) {
imax = MAX_APDU - 2 - apdu_len;
}
for (len = 0; len < imax; len++) {
apdu[apdu_len + len] = wpdata->application_data[len];
}
apdu_len += imax;
len = encode_closing_tag(&apdu[apdu_len], 3);
apdu_len += len;
/* optional priority - 0 if not set, 1..16 if set */
if (wpdata->priority != BACNET_NO_PRIORITY) {
len = encode_context_unsigned(&apdu[apdu_len], 4, wpdata->priority);
apdu_len += len;
}
} }
return apdu_len; return apdu_len;
} }
#endif #endif
/** Decode the service request only /**
* @brief Decode the service request only
* *
* FIXME: there could be various error messages returned * WriteProperty-Request ::= SEQUENCE {
* using unique values less than zero. * object-identifier [0] BACnetObjectIdentifier,
* property-identifier [1] BACnetPropertyIdentifier,
* property-array-index [2] Unsigned OPTIONAL,
* -- used only with array datatype
* -- if omitted with an array the entire
* -- array is referenced
* property-value [3] ABSTRACT-SYNTAX.&Type,
* priority [4] Unsigned (1..16) OPTIONAL
* -– used only when property is commandable
* }
* *
* @param apdu Pointer to the buffer. * @param apdu Pointer to the buffer.
* @param apdu_len Valid bytes in the buffer * @param apdu_size Valid bytes in the buffer
* @param wpdata Pointer to the write property data. * @param wpdata Pointer to the write property data.
* *
* @return Bytes encoded or a negative value as error. * @return number of bytes decoded, or #BACNET_STATUS_ERROR
*/ */
int wp_decode_service_request( int wp_decode_service_request(
uint8_t *apdu, unsigned apdu_len, BACNET_WRITE_PROPERTY_DATA *wpdata) uint8_t *apdu, unsigned apdu_size, BACNET_WRITE_PROPERTY_DATA *wpdata)
{ {
int len = 0; int len = 0;
int apdu_len = 0;
int tag_len = 0; int tag_len = 0;
uint8_t tag_number = 0; uint8_t tag_number = 0;
uint32_t len_value_type = 0; uint32_t instance = 0;
BACNET_OBJECT_TYPE type = OBJECT_NONE; /* for decoding */ BACNET_OBJECT_TYPE type = OBJECT_NONE; /* for decoding */
uint32_t property = 0; /* for decoding */ uint32_t property = 0; /* for decoding */
BACNET_UNSIGNED_INTEGER unsigned_value = 0; BACNET_UNSIGNED_INTEGER unsigned_value = 0;
@@ -122,80 +165,104 @@ int wp_decode_service_request(
int imax = 0; /* max application data length */ int imax = 0; /* max application data length */
/* check for value pointers */ /* check for value pointers */
if (apdu_len && wpdata) { if (!apdu) {
/* Tag 0: Object ID */ return BACNET_STATUS_ERROR;
if (!decode_is_context_tag(&apdu[len++], 0)) { }
return -1; /* object-identifier [0] BACnetObjectIdentifier */
len = bacnet_object_id_context_decode(
&apdu[apdu_len], apdu_size - apdu_len, 0, &type, &instance);
if (len > 0) {
if (instance > BACNET_MAX_INSTANCE) {
return BACNET_STATUS_ERROR;
} }
len += decode_object_id(&apdu[len], &type, &wpdata->object_instance); apdu_len += len;
wpdata->object_type = type; if (wpdata) {
/* Tag 1: Property ID */ wpdata->object_type = type;
len += decode_tag_number_and_value( wpdata->object_instance = instance;
&apdu[len], &tag_number, &len_value_type);
if (tag_number != 1) {
return -1;
} }
len += decode_enumerated(&apdu[len], len_value_type, &property); } else {
wpdata->object_property = (BACNET_PROPERTY_ID)property; return BACNET_STATUS_ERROR;
/* Tag 2: Optional Array Index */ }
/* note: decode without incrementing len so we can check for opening tag /* property-identifier [1] BACnetPropertyIdentifier */
*/ len = bacnet_enumerated_context_decode(
tag_len = decode_tag_number_and_value( &apdu[apdu_len], apdu_size - apdu_len, 1, &property);
&apdu[len], &tag_number, &len_value_type); if (len > 0) {
if (tag_number == 2) { apdu_len += len;
len += tag_len; if (wpdata) {
len += decode_unsigned(&apdu[len], len_value_type, &unsigned_value); wpdata->object_property = (BACNET_PROPERTY_ID)property;
}
} else {
return BACNET_STATUS_ERROR;
}
/* property-array-index [2] Unsigned OPTIONAL */
len = bacnet_unsigned_context_decode(
&apdu[apdu_len], apdu_size - apdu_len, 2, &unsigned_value);
if (len > 0) {
apdu_len += len;
if (wpdata) {
wpdata->array_index = unsigned_value; wpdata->array_index = unsigned_value;
} else { }
} else {
/* wrong tag - skip apdu_len increment and go to next field */
if (wpdata) {
wpdata->array_index = BACNET_ARRAY_ALL; wpdata->array_index = BACNET_ARRAY_ALL;
} }
/* Tag 3: opening context tag */ }
if (!decode_is_opening_tag_number(&apdu[len], 3)) { /* property-value [3] ABSTRACT-SYNTAX.&Type */
return -1; if (!bacnet_is_opening_tag_number(
} &apdu[apdu_len], apdu_size - apdu_len, 3, &len)) {
/* determine the length of the data blob */ return BACNET_STATUS_ERROR;
imax = bacapp_data_len( }
&apdu[len], apdu_len - len, (BACNET_PROPERTY_ID)property); /* determine the length of the data blob */
if (imax == BACNET_STATUS_ERROR) { imax = bacapp_data_len(
return -2; &apdu[apdu_len], apdu_size - apdu_len, (BACNET_PROPERTY_ID)property);
} if (imax == BACNET_STATUS_ERROR) {
/* a tag number of 3 is not extended so only one octet */ return BACNET_STATUS_ERROR;
len++; }
/* copy the data from the APDU */ /* count the opening tag number length */
if (imax > (MAX_APDU - len - 1 /*closing*/)) { apdu_len += len;
imax = (MAX_APDU - len - 1); /* copy the data from the APDU */
} if (imax > MAX_APDU) {
/* not enough size in application_data to store the data chunk */
return BACNET_STATUS_ERROR;
} else if (wpdata) {
for (i = 0; i < imax; i++) { for (i = 0; i < imax; i++) {
wpdata->application_data[i] = apdu[len + i]; wpdata->application_data[i] = apdu[apdu_len + i];
} }
wpdata->application_data_len = imax; wpdata->application_data_len = imax;
/* add on the data length */ }
len += imax; /* add on the data length */
if (!decode_is_closing_tag_number(&apdu[len], 3)) { apdu_len += imax;
return -2; if (!bacnet_is_closing_tag_number(
} &apdu[apdu_len], apdu_size - apdu_len, 3, &len)) {
/* a tag number of 3 is not extended so only one octet */ return BACNET_STATUS_ERROR;
len++; }
/* Tag 4: optional Priority - assumed MAX if not explicitly set */ /* count the closing tag number length */
apdu_len += len;
/* priority [4] Unsigned (1..16) OPTIONAL */
/* assumed MAX priority if not explicitly set */
if (wpdata) {
wpdata->priority = BACNET_MAX_PRIORITY; wpdata->priority = BACNET_MAX_PRIORITY;
if ((unsigned)len < apdu_len) { }
tag_len = decode_tag_number_and_value( if ((unsigned)apdu_len < apdu_size) {
&apdu[len], &tag_number, &len_value_type); len = bacnet_unsigned_context_decode(
if (tag_number == 4) { &apdu[apdu_len], apdu_len - apdu_size, 4, &unsigned_value);
len += tag_len; if (len > 0) {
len = decode_unsigned( apdu_len += len;
&apdu[len], len_value_type, &unsigned_value); if ((unsigned_value >= BACNET_MIN_PRIORITY) &&
if ((unsigned_value >= BACNET_MIN_PRIORITY) && (unsigned_value <= BACNET_MAX_PRIORITY)) {
(unsigned_value <= BACNET_MAX_PRIORITY)) { if (wpdata) {
wpdata->priority = (uint8_t)unsigned_value; wpdata->priority = (uint8_t)unsigned_value;
} else {
return -5;
} }
} else {
return BACNET_STATUS_ERROR;
} }
} else {
return BACNET_STATUS_ERROR;
} }
} }
return len; return apdu_len;
} }
/** /**
test/CMakeLists.txt
+1
View File
@@ -52,6 +52,7 @@ list(APPEND testdirs
bacnet/bacapp bacnet/bacapp
bacnet/bacdcode bacnet/bacdcode
bacnet/bacdevobjpropref bacnet/bacdevobjpropref
bacnet/bacdest
bacnet/bacerror bacnet/bacerror
bacnet/bacint bacnet/bacint
bacnet/bacpropstates bacnet/bacpropstates
test/bacnet/bacdest/src/main.c
+1 -1
View File
@@ -11,7 +11,7 @@
#include <stdint.h> #include <stdint.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <ztest.h> #include <zephyr/ztest.h>
#include <bacnet/bacdest.h> #include <bacnet/bacdest.h>
/** /**
test/bacnet/wp/src/main.c
+71 -30
View File
@@ -17,33 +17,49 @@
*/ */
/** /**
* @brief Test encode/decode API for unsigned 16b integers * @brief Decode stub for WriteProperty service
* @return number of bytes decoded, or #BACNET_STATUS_ERROR
*/ */
static int wp_decode_apdu(uint8_t *apdu, static int wp_decode_apdu(uint8_t *apdu,
unsigned apdu_len, unsigned apdu_size,
uint8_t *invoke_id, uint8_t *invoke_id,
BACNET_WRITE_PROPERTY_DATA *wpdata) BACNET_WRITE_PROPERTY_DATA *wpdata)
{ {
int len = 0; int len = 0;
unsigned offset = 0; int apdu_len = 0;
if (!apdu) if (!apdu) {
return -1; return -1;
}
if (apdu_size < 4) {
return BACNET_STATUS_ERROR;
}
/* optional checking - most likely was already done prior to this call */ /* 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; return BACNET_STATUS_ERROR;
}
/* apdu[1] = encode_max_segs_max_apdu(0, MAX_APDU); */ /* 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_WRITE_PROPERTY) *invoke_id = apdu[2]; /* invoke id - filled in by net layer */
return -1; }
offset = 4; if (apdu[3] != SERVICE_CONFIRMED_WRITE_PROPERTY) {
return BACNET_STATUS_ERROR;
if (apdu_len > offset) { }
len = 4;
apdu_len += len;
if (apdu_len < apdu_size) {
len = len =
wp_decode_service_request(&apdu[offset], apdu_len - offset, wpdata); wp_decode_service_request(&apdu[apdu_len], apdu_size - apdu_len, wpdata);
if (len > 0) {
apdu_len += len;
} else {
apdu_len = len;
}
} else {
return BACNET_STATUS_ERROR;
} }
return len; return apdu_len;
} }
static void testWritePropertyTag(BACNET_APPLICATION_DATA_VALUE *value) static void testWritePropertyTag(BACNET_APPLICATION_DATA_VALUE *value)
@@ -53,6 +69,7 @@ static void testWritePropertyTag(BACNET_APPLICATION_DATA_VALUE *value)
BACNET_APPLICATION_DATA_VALUE test_value; BACNET_APPLICATION_DATA_VALUE test_value;
uint8_t apdu[480] = { 0 }; uint8_t apdu[480] = { 0 };
int len = 0; int len = 0;
int null_len = 0;
int apdu_len = 0; int apdu_len = 0;
uint8_t invoke_id = 128; uint8_t invoke_id = 128;
uint8_t test_invoke_id = 0; uint8_t test_invoke_id = 0;
@@ -60,17 +77,38 @@ static void testWritePropertyTag(BACNET_APPLICATION_DATA_VALUE *value)
wpdata.application_data_len = wpdata.application_data_len =
bacapp_encode_application_data(&wpdata.application_data[0], value); bacapp_encode_application_data(&wpdata.application_data[0], value);
null_len = wp_encode_apdu(NULL, invoke_id, &wpdata);
len = wp_encode_apdu(&apdu[0], invoke_id, &wpdata); len = wp_encode_apdu(&apdu[0], invoke_id, &wpdata);
zassert_equal(null_len, len, "null_len=%d len=%d", null_len, len);
zassert_not_equal(len, 0, "len=%d", len); zassert_not_equal(len, 0, "len=%d", len);
/* decode the data */ /* decode the data */
apdu_len = len; apdu_len = len;
null_len = wp_decode_apdu(&apdu[0], apdu_len, &test_invoke_id, NULL);
len = wp_decode_apdu(&apdu[0], apdu_len, &test_invoke_id, &test_data); len = wp_decode_apdu(&apdu[0], apdu_len, &test_invoke_id, &test_data);
zassert_equal(null_len, len, "null_len=%d len=%d", null_len, len);
zassert_true(len > 0, "len=%d", len); zassert_true(len > 0, "len=%d", len);
zassert_equal(test_data.object_type, wpdata.object_type, NULL); 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_instance, wpdata.object_instance, NULL);
zassert_equal(test_data.object_property, wpdata.object_property, NULL); zassert_equal(test_data.object_property, wpdata.object_property, NULL);
zassert_equal(test_data.array_index, wpdata.array_index, NULL); zassert_equal(test_data.array_index, wpdata.array_index, NULL);
/* test the OPTIONAL property-array-index */
wpdata.array_index = BACNET_ARRAY_ALL;
len = wp_encode_apdu(&apdu[0], invoke_id, &wpdata);
apdu_len = wp_decode_apdu(&apdu[0], len, &test_invoke_id, &test_data);
zassert_equal(apdu_len, len, "apdu_len=%d len=%d", apdu_len, len);
zassert_true(len > 0, "len=%d", len);
wpdata.array_index = 0;
/* test the OPTIONAL priority */
wpdata.priority = BACNET_MAX_PRIORITY;
len = wp_encode_apdu(&apdu[0], invoke_id, &wpdata);
apdu_len = wp_decode_apdu(&apdu[0], len, &test_invoke_id, &test_data);
zassert_equal(apdu_len, len, "apdu_len=%d len=%d", apdu_len, len);
zassert_true(len > 0, "len=%d", len);
wpdata.priority = 0;
/* decode the application value of the request */ /* decode the application value of the request */
len = wp_encode_apdu(&apdu[0], invoke_id, &wpdata);
apdu_len = wp_decode_apdu(&apdu[0], len, &test_invoke_id, &test_data);
apdu_len = len;
len = bacapp_decode_application_data(test_data.application_data, len = bacapp_decode_application_data(test_data.application_data,
test_data.application_data_len, &test_value); test_data.application_data_len, &test_value);
zassert_equal(test_value.tag, value->tag, NULL); zassert_equal(test_value.tag, value->tag, NULL);
@@ -98,29 +136,37 @@ static void testWritePropertyTag(BACNET_APPLICATION_DATA_VALUE *value)
test_value.type.Enumerated, value->type.Enumerated, NULL); test_value.type.Enumerated, value->type.Enumerated, NULL);
break; break;
case BACNET_APPLICATION_TAG_DATE: case BACNET_APPLICATION_TAG_DATE:
zassert_equal(test_value.type.Date.year, value->type.Date.year, NULL); zassert_equal(
test_value.type.Date.year, value->type.Date.year, NULL);
zassert_equal( zassert_equal(
test_value.type.Date.month, value->type.Date.month, NULL); test_value.type.Date.month, value->type.Date.month, NULL);
zassert_equal(test_value.type.Date.day, value->type.Date.day, NULL); zassert_equal(test_value.type.Date.day, value->type.Date.day, NULL);
zassert_equal(test_value.type.Date.wday, value->type.Date.wday, NULL); zassert_equal(
test_value.type.Date.wday, value->type.Date.wday, NULL);
break; break;
case BACNET_APPLICATION_TAG_TIME: case BACNET_APPLICATION_TAG_TIME:
zassert_equal(test_value.type.Time.hour, value->type.Time.hour, NULL); zassert_equal(
test_value.type.Time.hour, value->type.Time.hour, NULL);
zassert_equal(test_value.type.Time.min, value->type.Time.min, NULL); zassert_equal(test_value.type.Time.min, value->type.Time.min, NULL);
zassert_equal(test_value.type.Time.sec, value->type.Time.sec, NULL); zassert_equal(test_value.type.Time.sec, value->type.Time.sec, NULL);
zassert_equal( zassert_equal(test_value.type.Time.hundredths,
test_value.type.Time.hundredths, value->type.Time.hundredths, NULL); value->type.Time.hundredths, NULL);
break; break;
case BACNET_APPLICATION_TAG_OBJECT_ID: case BACNET_APPLICATION_TAG_OBJECT_ID:
zassert_equal( zassert_equal(test_value.type.Object_Id.type,
test_value.type.Object_Id.type, value->type.Object_Id.type, NULL); value->type.Object_Id.type, NULL);
zassert_equal( zassert_equal(test_value.type.Object_Id.instance,
test_value.type.Object_Id.instance, value->type.Object_Id.instance, NULL);
value->type.Object_Id.instance, NULL);
break; break;
default: default:
break; break;
} }
/* test packets that are too short */
while (apdu_len) {
apdu_len--;
len = wp_decode_apdu(&apdu[0], apdu_len, &test_invoke_id, &test_data);
zassert_true(len <= 0, "len=%d tag=%d", len, value->tag);
}
} }
#if defined(CONFIG_ZTEST_NEW_API) #if defined(CONFIG_ZTEST_NEW_API)
@@ -199,22 +245,17 @@ static void testWriteProperty(void)
value.type.Object_Id.type = OBJECT_LIFE_SAFETY_ZONE; value.type.Object_Id.type = OBJECT_LIFE_SAFETY_ZONE;
value.type.Object_Id.instance = BACNET_MAX_INSTANCE; value.type.Object_Id.instance = BACNET_MAX_INSTANCE;
testWritePropertyTag(&value); testWritePropertyTag(&value);
return;
} }
/** /**
* @} * @}
*/ */
#if defined(CONFIG_ZTEST_NEW_API) #if defined(CONFIG_ZTEST_NEW_API)
ZTEST_SUITE(wp_tests, NULL, NULL, NULL, NULL, NULL); ZTEST_SUITE(wp_tests, NULL, NULL, NULL, NULL, NULL);
#else #else
void test_main(void) void test_main(void)
{ {
ztest_test_suite(wp_tests, ztest_test_suite(wp_tests, ztest_unit_test(testWriteProperty));
ztest_unit_test(testWriteProperty)
);
ztest_run_test_suite(wp_tests); ztest_run_test_suite(wp_tests);
} }