From 80c51a06e39c36448f2573bb27474a77d5971a5f Mon Sep 17 00:00:00 2001 From: Steve Karg Date: Tue, 26 Mar 2024 12:29:15 -0500 Subject: [PATCH] Secured the WPM and RPM service encoders (#604) * Secured the WPM and RPM client service encoders. * Fixed RPM and WPM apps when fail to encode request. * Fixed WPM app number of arguments checking. --- apps/readpropm/main.c | 4 + apps/writepropm/main.c | 6 +- src/bacnet/basic/service/s_wpm.c | 4 +- src/bacnet/rpm.c | 158 +++++++++-------- src/bacnet/rpm.h | 10 ++ src/bacnet/wpm.c | 279 +++++++++++++++++++------------ src/bacnet/wpm.h | 10 ++ 7 files changed, 291 insertions(+), 180 deletions(-) diff --git a/apps/readpropm/main.c b/apps/readpropm/main.c index c08c080d..a57b14c3 100644 --- a/apps/readpropm/main.c +++ b/apps/readpropm/main.c @@ -549,6 +549,10 @@ int main(int argc, char *argv[]) Request_Invoke_ID = Send_Read_Property_Multiple_Request( &buffer[0], sizeof(buffer), Target_Device_Object_Instance, Read_Access_Data); + if (Request_Invoke_ID == 0) { + fprintf(stderr, "\rError: failed to send request!\n"); + break; + } } else if (tsm_invoke_id_free(Request_Invoke_ID)) { break; } else if (tsm_invoke_id_failed(Request_Invoke_ID)) { diff --git a/apps/writepropm/main.c b/apps/writepropm/main.c index 7b2cf977..be2d8fbf 100644 --- a/apps/writepropm/main.c +++ b/apps/writepropm/main.c @@ -307,7 +307,7 @@ int main(int argc, char *argv[]) return 0; } } - if (argc < 9) { + if (argc < 8) { print_usage(filename); return 0; } @@ -523,6 +523,10 @@ int main(int argc, char *argv[]) Request_Invoke_ID = Send_Write_Property_Multiple_Request( &buffer[0], sizeof(buffer), Target_Device_Object_Instance, Write_Access_Data); + if (Request_Invoke_ID == 0) { + fprintf(stderr, "\rError: failed to send request!\n"); + break; + } } else if (tsm_invoke_id_free(Request_Invoke_ID)) { break; } else if (tsm_invoke_id_failed(Request_Invoke_ID)) { diff --git a/src/bacnet/basic/service/s_wpm.c b/src/bacnet/basic/service/s_wpm.c index e784c264..64bfc57d 100644 --- a/src/bacnet/basic/service/s_wpm.c +++ b/src/bacnet/basic/service/s_wpm.c @@ -92,8 +92,10 @@ uint8_t Send_Write_Property_Multiple_Request(uint8_t *pdu, /* encode the APDU portion of the packet */ len = wpm_encode_apdu( &pdu[pdu_len], max_pdu - pdu_len, invoke_id, write_access_data); + if (len <= 0) { + return 0; + } pdu_len += len; - /* will it fit in the sender? note: if there is a bottleneck router in between us and the destination, we won't know unless diff --git a/src/bacnet/rpm.c b/src/bacnet/rpm.c index 30976454..a2678acd 100644 --- a/src/bacnet/rpm.c +++ b/src/bacnet/rpm.c @@ -45,26 +45,22 @@ #if BACNET_SVC_RPM_A -/** Encode the initial portion of the service - * - * @param apdu Pointer to the buffer for encoding. +/** + * @brief Encode the initial portion of the service + * @param apdu Pointer to the buffer for encoding, or NULL for length * @param invoke_id Invoke ID - * - * @return Bytes encoded (usually 4) or zero on error. + * @return number of bytes encoded */ int rpm_encode_apdu_init(uint8_t *apdu, uint8_t invoke_id) { - int apdu_len = 0; /* total length of the apdu, return value */ - if (apdu) { apdu[0] = PDU_TYPE_CONFIRMED_SERVICE_REQUEST; apdu[1] = encode_max_segs_max_apdu(0, MAX_APDU); apdu[2] = invoke_id; apdu[3] = SERVICE_CONFIRMED_READ_PROP_MULTIPLE; /* service choice */ - apdu_len = 4; } - return apdu_len; + return 4; } /** Encode the beginning, including @@ -134,95 +130,123 @@ int rpm_encode_apdu_object_end(uint8_t *apdu) return apdu_len; } -/** Encode an RPM request, to be sent. - * - * @param apdu [in,out] Buffer to hold encoded bytes. - * @param max_apdu [in] Length of apdu buffer. - * @param invoke_id [in] The Invoke ID to use for this message. +/** + * @brief Encode the ReadPropertyMultiple-Request + * @param apdu Buffer to hold encoded bytes, or NULL for length * @param read_access_data [in] The RPM data to be requested. * @return Length of encoded bytes, or 0 on failure. */ -int rpm_encode_apdu(uint8_t *apdu, - size_t max_apdu, - uint8_t invoke_id, - BACNET_READ_ACCESS_DATA *read_access_data) +int read_property_multiple_request_encode( + uint8_t *apdu, + BACNET_READ_ACCESS_DATA *data) { + int len = 0; /* length of each encoding */ int apdu_len = 0; /* total length of the apdu, return value */ - int len = 0; /* length of the data */ BACNET_READ_ACCESS_DATA *rpm_object; /* current object */ - uint8_t apdu_temp[16]; /* temp for data before copy */ BACNET_PROPERTY_REFERENCE *rpm_property; /* current property */ - len = rpm_encode_apdu_init(&apdu_temp[0], invoke_id); - len = (int)memcopy(&apdu[0], &apdu_temp[0], (size_t)apdu_len, (size_t)len, - (size_t)max_apdu); - if (len == 0) { - return 0; - } - apdu_len += len; - rpm_object = read_access_data; + rpm_object = data; while (rpm_object) { - /* The encode function will return a length not more than 12. So the - * temp buffer being 16 bytes is fine enough. */ - len = encode_context_object_id(&apdu_temp[0], 0, + len = encode_context_object_id(apdu, 0, rpm_object->object_type, rpm_object->object_instance); - len = (int)memcopy(&apdu[0], &apdu_temp[0], (size_t)apdu_len, - (size_t)len, (size_t)max_apdu); - if (len == 0) { - return 0; - } apdu_len += len; + if (apdu) { + apdu += len; + } /* Tag 1: sequence of ReadAccessSpecification */ - len = encode_opening_tag(&apdu_temp[0], 1); - len = (int)memcopy(&apdu[0], &apdu_temp[0], (size_t)apdu_len, - (size_t)len, (size_t)max_apdu); - if (len == 0) { - return 0; - } + len = encode_opening_tag(apdu, 1); apdu_len += len; + if (apdu) { + apdu += len; + } rpm_property = rpm_object->listOfProperties; while (rpm_property) { - /* The encode function will return a length not more than 12. So the - * temp buffer being 16 bytes is fine enough. Stuff as many - * properties into it as APDU length will allow. */ len = encode_context_enumerated( - &apdu_temp[0], 0, rpm_property->propertyIdentifier); - len = (int)memcopy(&apdu[0], &apdu_temp[0], (size_t)apdu_len, - (size_t)len, (size_t)max_apdu); - if (len == 0) { - return 0; - } + apdu, 0, rpm_property->propertyIdentifier); apdu_len += len; + if (apdu) { + apdu += len; + } /* optional array index */ if (rpm_property->propertyArrayIndex != BACNET_ARRAY_ALL) { len = encode_context_unsigned( - &apdu_temp[0], 1, rpm_property->propertyArrayIndex); - len = (int)memcopy(&apdu[0], &apdu_temp[0], (size_t)apdu_len, - (size_t)len, (size_t)max_apdu); - if (len == 0) { - return 0; - } + apdu, 1, rpm_property->propertyArrayIndex); apdu_len += len; + if (apdu) { + apdu += len; + } } rpm_property = rpm_property->next; - /* Full? */ - if ((unsigned)apdu_len >= max_apdu) { - return 0; - } - } - len = encode_closing_tag(&apdu_temp[0], 1); - len = (int)memcopy(&apdu[0], &apdu_temp[0], (size_t)apdu_len, - (size_t)len, (size_t)max_apdu); - if (len == 0) { - return 0; } + len = encode_closing_tag(apdu, 1); apdu_len += len; + if (apdu) { + apdu += len; + } rpm_object = rpm_object->next; } return apdu_len; } +/** + * @brief Encode the ReadPropertyMultiple-Request service + * @param apdu Pointer to the buffer for encoding into + * @param apdu_size number of bytes available in the buffer + * @param data Pointer to the service data used for encoding values + * @return number of bytes encoded, or zero if unable to encode or too large + */ +size_t read_property_multiple_request_service_encode( + uint8_t *apdu, size_t apdu_size, BACNET_READ_ACCESS_DATA *data) +{ + size_t apdu_len = 0; /* total length of the apdu, return value */ + + apdu_len = read_property_multiple_request_encode(NULL, data); + if (apdu_len > apdu_size) { + apdu_len = 0; + } else { + apdu_len = read_property_multiple_request_encode(apdu, data); + } + + return apdu_len; +} + +/** Encode an RPM request, to be sent. + * + * @param apdu [in,out] Buffer to hold encoded bytes. + * @param apdu_size [in] Length of apdu buffer. + * @param invoke_id [in] The Invoke ID to use for this message. + * @param data [in] The RPM data to be requested. + * @return Length of encoded bytes, or 0 on failure. + */ +int rpm_encode_apdu(uint8_t *apdu, + size_t apdu_size, + uint8_t invoke_id, + BACNET_READ_ACCESS_DATA *data) +{ + int apdu_len = 0; /* total length of the apdu, return value */ + int len = 0; /* length of the data */ + + len = rpm_encode_apdu_init(NULL, invoke_id); + if (len > apdu_size) { + return 0; + } else { + len = rpm_encode_apdu_init(apdu, invoke_id); + apdu_len += len; + if (apdu) { + apdu += len; + } + len = read_property_multiple_request_service_encode( + apdu, apdu_size-apdu_len, data); + if (len > 0) { + apdu_len += len; + } else { + apdu_len = 0; + } + } + + return apdu_len; +} #endif /** Decode the object portion of the service request only. Bails out if diff --git a/src/bacnet/rpm.h b/src/bacnet/rpm.h index ff55a1a5..769fd232 100644 --- a/src/bacnet/rpm.h +++ b/src/bacnet/rpm.h @@ -108,6 +108,16 @@ extern "C" { int rpm_encode_apdu_object_end( uint8_t * apdu); + BACNET_STACK_EXPORT + int read_property_multiple_request_encode( + uint8_t *apdu, + BACNET_READ_ACCESS_DATA *data); + BACNET_STACK_EXPORT + size_t read_property_multiple_request_service_encode( + uint8_t *apdu, + size_t apdu_size, + BACNET_READ_ACCESS_DATA *data); + BACNET_STACK_EXPORT int rpm_encode_apdu( uint8_t * apdu, diff --git a/src/bacnet/wpm.c b/src/bacnet/wpm.c index fa07d8da..08e3314d 100644 --- a/src/bacnet/wpm.c +++ b/src/bacnet/wpm.c @@ -206,176 +206,233 @@ int wpm_decode_object_property( return len; } -/** Init the APDU for encoding. - * - * @param apdu [in] The APDU buffer. +/** + * @brief Init the APDU for encoding. + * @param apdu [in] The APDU buffer, or NULL for length * @param invoke_id [in] The ID of the saervice invoked. - * - * @return Bytes encoded, usually 4 on success. + * @return number of bytes encoded */ int wpm_encode_apdu_init(uint8_t *apdu, uint8_t invoke_id) { - int apdu_len = 0; /* total length of the apdu, return value */ - if (apdu) { apdu[0] = PDU_TYPE_CONFIRMED_SERVICE_REQUEST; apdu[1] = encode_max_segs_max_apdu(0, MAX_APDU); apdu[2] = invoke_id; apdu[3] = SERVICE_CONFIRMED_WRITE_PROP_MULTIPLE; /* service choice */ - apdu_len = 4; } - return apdu_len; + return 4; } -/** Decode the very begin of an object in the APDU. - * - * @param apdu [in] The APDU buffer. +/** + * @brief Decode the very begin of an object in the APDU. + * @param apdu [in] The APDU buffer, or NULL for length * @param object_type [in] The object type to decode. * @param object_instance [in] The object instance. - * - * @return Bytes encoded. + * @return number of bytes encoded */ int wpm_encode_apdu_object_begin( uint8_t *apdu, BACNET_OBJECT_TYPE object_type, uint32_t object_instance) { int apdu_len = 0; /* total length of the apdu, return value */ + int len = 0; + len = encode_context_object_id(apdu, 0, object_type, object_instance); + apdu_len += len; if (apdu) { - apdu_len = - encode_context_object_id(&apdu[0], 0, object_type, object_instance); - /* Tag 1: sequence of WriteAccessSpecification */ - apdu_len += encode_opening_tag(&apdu[apdu_len], 1); + apdu += len; } + /* Tag 1: sequence of WriteAccessSpecification */ + len = encode_opening_tag(apdu, 1); + apdu_len += len; return apdu_len; } -/** Decode the very end of an object in the APDU. - * - * @param apdu [in] The APDU buffer. - * - * @return Bytes encoded. +/** + * @brief Encode the very end of an object in the APDU. + * @param apdu [in] The APDU buffer, or NULL for length + * @return number of bytes encoded */ int wpm_encode_apdu_object_end(uint8_t *apdu) { - int apdu_len = 0; /* total length of the apdu, return value */ - - if (apdu) { - apdu_len = encode_closing_tag(&apdu[0], 1); - } - - return apdu_len; + return encode_closing_tag(apdu, 1); } -/** Encode the object property into the APDU. - * - * @param apdu [in] The APDU buffer. +/** + * @brief Encode the object property into the APDU. + * @param apdu [in] The APDU buffer, or NULL for length * @param wpdata [in] Pointer to the property data. - * - * @return Bytes encoded. + * @return number of bytes encoded */ int wpm_encode_apdu_object_property( uint8_t *apdu, BACNET_WRITE_PROPERTY_DATA *wpdata) { int apdu_len = 0; /* total length of the apdu, return value */ int len = 0; - int imax; + if (!wpdata) { + return 0; + } + len = encode_context_enumerated(apdu, 0, wpdata->object_property); + apdu_len += len; if (apdu) { - apdu_len = - encode_context_enumerated(&apdu[0], 0, wpdata->object_property); - /* optional array index */ - if (wpdata->array_index != BACNET_ARRAY_ALL) { - apdu_len += encode_context_unsigned( - &apdu[apdu_len], 1, wpdata->array_index); + apdu += len; + } + /* optional array index */ + if (wpdata->array_index != BACNET_ARRAY_ALL) { + len = encode_context_unsigned(apdu, 1, wpdata->array_index); + apdu_len += len; + if (apdu) { + apdu += len; } - apdu_len += encode_opening_tag(&apdu[apdu_len], 2); - imax = wpdata->application_data_len; - if (imax > (MAX_APDU - apdu_len - 2)) { - imax = (MAX_APDU - apdu_len - 2); - } - for (len = 0; len < imax; len++) { - apdu[apdu_len] = wpdata->application_data[len]; - apdu_len++; - } - apdu_len += encode_closing_tag(&apdu[apdu_len], 2); - if (wpdata->priority != BACNET_NO_PRIORITY) { - if (apdu_len < MAX_APDU) { - apdu_len += encode_context_unsigned( - &apdu[apdu_len], 3, wpdata->priority); - } + } + len = encode_opening_tag(apdu, 2); + apdu_len += len; + if (apdu) { + apdu += len; + } + /* copy the property value */ + apdu_len += wpdata->application_data_len; + if (apdu) { + for (len = 0; len < wpdata->application_data_len; len++) { + *apdu = wpdata->application_data[len]; + apdu++; } } + len = encode_closing_tag(apdu, 2); + apdu_len += len; + if (apdu) { + apdu += len; + } + /* optional priority */ + if (wpdata->priority != BACNET_NO_PRIORITY) { + len = encode_context_unsigned(apdu, 3, wpdata->priority); + apdu_len += len; + } return apdu_len; } -/** Encode the request into the APDU. +/** + * @brief Encode APDU for WritePropertyMultiple-Request * - * @param apdu [in] The APDU buffer. - * @param max_apdu [in] Maximum space in the buffer. + * WritePropertyMultiple-Request ::= SEQUENCE { + * + * } + * + * @param apdu Pointer to the buffer, or NULL for length + * @param data Pointer to the data to encode. + * @return number of bytes encoded, or zero on error. + */ +int write_property_multiple_request_encode(uint8_t *apdu, + BACNET_WRITE_ACCESS_DATA *data) +{ + int len = 0; /* length of each encoding */ + int apdu_len = 0; /* total length of the apdu, return value */ + BACNET_WRITE_ACCESS_DATA *wpm_object; /* current object */ + BACNET_PROPERTY_VALUE *wpm_property; /* current property */ + BACNET_WRITE_PROPERTY_DATA wpdata; + + wpm_object = data; + while (wpm_object) { + len = wpm_encode_apdu_object_begin(apdu, + wpm_object->object_type, wpm_object->object_instance); + apdu_len += len; + if (apdu) { + apdu += len; + } + wpm_property = wpm_object->listOfProperties; + while (wpm_property) { + wpdata.object_property = wpm_property->propertyIdentifier; + wpdata.array_index = wpm_property->propertyArrayIndex; + wpdata.priority = wpm_property->priority; + /* check length for fitting */ + wpdata.application_data_len = bacapp_encode_data( + NULL, &wpm_property->value); + if (wpdata.application_data_len > + sizeof(wpdata.application_data)) { + /* too big for buffer */ + return 0; + } + wpdata.application_data_len = bacapp_encode_data( + wpdata.application_data, &wpm_property->value); + len = wpm_encode_apdu_object_property(apdu, &wpdata); + apdu_len += len; + if (apdu) { + apdu += len; + } + wpm_property = wpm_property->next; + } + len = wpm_encode_apdu_object_end(apdu); + apdu_len += len; + if (apdu) { + apdu += len; + } + wpm_object = wpm_object->next; + } + + return apdu_len; +} + +/** + * @brief Encode the WritePropertyMultiple-Request service + * @param apdu Pointer to the buffer for encoding into + * @param apdu_size number of bytes available in the buffer + * @param data Pointer to the service data used for encoding values + * @return number of bytes encoded, or zero if unable to encode or + * too big for buffer + */ +size_t write_property_multiple_request_service_encode( + uint8_t *apdu, size_t apdu_size, BACNET_WRITE_ACCESS_DATA *data) +{ + size_t apdu_len = 0; /* total length of the apdu, return value */ + + apdu_len = write_property_multiple_request_encode(NULL, data); + if (apdu_len > apdu_size) { + /* too big for buffer */ + apdu_len = 0; + } else { + apdu_len = write_property_multiple_request_encode(apdu, data); + } + + return apdu_len; +} + +/** + * @brief Encode the WritePropertyMultiple-Request into the APDU. + * @param apdu [in] The APDU buffer + * @param apdu_size [in] Maximum space in the buffer. * @param invoke_id [in] Invoked service ID. - * @param write_access_data [in] Access data. - * - * @return Bytes encoded. + * @param data [in] BACnetWriteAccessData + * @return number of bytes encoded, or zero if unable to encode or + * too big for buffer */ int wpm_encode_apdu(uint8_t *apdu, - size_t max_apdu, + size_t apdu_size, uint8_t invoke_id, - BACNET_WRITE_ACCESS_DATA *write_access_data) + BACNET_WRITE_ACCESS_DATA *data) { int apdu_len = 0; int len = 0; - size_t usize; - BACNET_WRITE_ACCESS_DATA *wpm_object; /* current object */ - uint8_t apdu_temp[MAX_APDU]; /* temp for data before copy */ - BACNET_PROPERTY_VALUE *wpm_property; /* current property */ - BACNET_WRITE_PROPERTY_DATA - wpdata; /* for compatibility with wpm_encode_apdu_object_property - function */ + len = wpm_encode_apdu_init(NULL, invoke_id); + if (len > apdu_size) { + /* too big for buffer */ + return 0; + } + len = wpm_encode_apdu_init(apdu, invoke_id); + apdu_len += len; if (apdu) { - len = wpm_encode_apdu_init(&apdu[0], invoke_id); + apdu += len; + } + len = write_property_multiple_request_service_encode( + apdu, apdu_size-apdu_len, data); + if (len > 0) { + /* too big for buffer */ apdu_len += len; - - wpm_object = write_access_data; - - while (wpm_object) { - len = wpm_encode_apdu_object_begin(&apdu[apdu_len], - wpm_object->object_type, wpm_object->object_instance); - apdu_len += len; - if (apdu_len >= (int)max_apdu) { - break; - } - - wpm_property = wpm_object->listOfProperties; - - while (wpm_property) { - wpdata.object_property = wpm_property->propertyIdentifier; - wpdata.array_index = wpm_property->propertyArrayIndex; - wpdata.priority = wpm_property->priority; - usize = (size_t)bacapp_encode_data( - &apdu_temp[0], &wpm_property->value); - if (usize > sizeof(wpdata.application_data)) { - usize = sizeof(wpdata.application_data); - } - wpdata.application_data_len = (int)usize; - memcpy(&wpdata.application_data[0], &apdu_temp[0], usize); - len = wpm_encode_apdu_object_property(&apdu[apdu_len], &wpdata); - apdu_len += len; - if (apdu_len >= (int)max_apdu) { - break; - } - - wpm_property = wpm_property->next; - } - - len = wpm_encode_apdu_object_end(&apdu[apdu_len]); - apdu_len += len; - - wpm_object = wpm_object->next; - } + } else { + return 0; } return apdu_len; diff --git a/src/bacnet/wpm.h b/src/bacnet/wpm.h index 3d6ff07a..b8feea7e 100644 --- a/src/bacnet/wpm.h +++ b/src/bacnet/wpm.h @@ -77,6 +77,16 @@ extern "C" { BACNET_STACK_EXPORT int wpm_encode_apdu_object_end( uint8_t * apdu); + + BACNET_STACK_EXPORT + int write_property_multiple_request_encode(uint8_t *apdu, + BACNET_WRITE_ACCESS_DATA *data); + BACNET_STACK_EXPORT + size_t write_property_multiple_request_service_encode( + uint8_t *apdu, + size_t apdu_size, + BACNET_WRITE_ACCESS_DATA *data); + BACNET_STACK_EXPORT int wpm_encode_apdu( uint8_t * apdu,