Bugfix/bacnet array encoding overflows (#414)

* Add common BACnetARRAY encode function to fix Device object list buffer overflow. Refactor device, analog-output, access-door and binary-output objects to use common BACnetARRAY encoder. * Fix non-POSIX builds (win32). * Cleanup some ports/stm32 build warnings --------- Co-authored-by: Steve Karg <skarg@users.sourceforge.net>
2023-04-13 20:43:54 -05:00
parent 064c6f7f1c
commit e517df0d47
23 changed files with 872 additions and 834 deletions
@@ -158,13 +158,14 @@ CFLAGS += -ffunction-sections -fdata-sections -fno-strict-aliasing
 CFLAGS += -fno-builtin
 # place uninitialized global variables in the data section of the object file.
 CFLAGS += -fno-common
+WARNING_ALL := -Wall
 # enable all relevant warnings that find bugs
-WARNING_ALL := -Wall -Wextra -Wall -Wfloat-equal -Wconversion -Wparentheses
-WARNING_ALL += -pedantic -Wunused-parameter -Wunused-variable -Wreturn-type
-WARNING_ALL += -Wunused-function -Wreturn-type -Wunused-value
-WARNING_ALL += -Wswitch-default -Wuninitialized -Winit-self
-WARNING_ALL += -Wno-sign-conversion -Wno-conversion -Wno-sign-compare
-WARNING_ALL += -Wno-long-long
+#WARNING_ALL += -pedantic -Wextra -Wfloat-equal -Wconversion -Wparentheses
+#WARNING_ALL += -Wunused-parameter -Wunused-variable -Wreturn-type
+#WARNING_ALL += -Wunused-function -Wreturn-type -Wunused-value
+#WARNING_ALL += -Wswitch-default -Wuninitialized -Winit-self
+#WARNING_ALL += -Wno-sign-conversion -Wno-conversion -Wno-sign-compare
+#WARNING_ALL += -Wno-long-long
 #WARNING_ALL += -Wredundant-decls
 #WARNING_ALL += -Werror
 CFLAGS += $(WARNING_ALL)
@@ -174,7 +175,8 @@ CFLAGS += -Wno-missing-braces
 CFLAGS += -Wno-missing-prototypes
 # don't warn about array subscript being char
 CFLAGS += -Wno-char-subscripts
-
+# FIXME later
+CFLAGS += -Wno-unused-parameter
 #  -Wa,<options> Pass comma-separated <options> on to the assembler
 AFLAGS = -Wa,-ahls,-mapcs-32,-adhlns=$(<:.s=.lst)

@@ -59,12 +59,11 @@ static struct my_object_functions {
    read_property_function Object_Read_Property;
    write_property_function Object_Write_Property;
    rpm_property_lists_function Object_RPM_List;
-} Object_Table[] = {
-    { OBJECT_DEVICE, NULL, /* don't init - recursive! */
-        Device_Count, Device_Index_To_Instance,
-        Device_Valid_Object_Instance_Number,
-        Device_Object_Name, Device_Read_Property_Local,
-        Device_Write_Property_Local, Device_Property_Lists },
+} Object_Table[] = { { OBJECT_DEVICE, NULL, /* don't init - recursive! */
+                         Device_Count, Device_Index_To_Instance,
+                         Device_Valid_Object_Instance_Number,
+                         Device_Object_Name, Device_Read_Property_Local,
+                         Device_Write_Property_Local, Device_Property_Lists },
    { OBJECT_BINARY_OUTPUT, Binary_Output_Init, Binary_Output_Count,
        Binary_Output_Index_To_Instance, Binary_Output_Valid_Instance,
        Binary_Output_Object_Name, Binary_Output_Read_Property,
@@ -75,7 +74,8 @@ static struct my_object_functions {
        Network_Port_Object_Name, Network_Port_Read_Property,
        Network_Port_Write_Property, Network_Port_Property_Lists },
 #endif
-    { MAX_BACNET_OBJECT_TYPE, NULL, NULL, NULL, NULL, NULL, NULL, NULL } };
+    { MAX_BACNET_OBJECT_TYPE, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
+        NULL } };

 /* note: you really only need to define variables for
   properties that are writable or that may change.
@@ -185,13 +185,9 @@ static int Read_Property_Common(
 #if (BACNET_PROTOCOL_REVISION >= 14)
        case PROP_PROPERTY_LIST:
            Device_Objects_Property_List(
-                rpdata->object_type,
-                rpdata->object_instance,
-                &property_list);
-            apdu_len = property_list_encode(
-                rpdata,
-                property_list.Required.pList,
-                property_list.Optional.pList,
+                rpdata->object_type, rpdata->object_instance, &property_list);
+            apdu_len = property_list_encode(rpdata,
+                property_list.Required.pList, property_list.Optional.pList,
                property_list.Proprietary.pList);
            break;
 #endif
@@ -346,8 +342,7 @@ bool Device_Set_Object_Name(BACNET_CHARACTER_STRING *object_name)
    return status;
 }

-bool Device_Reinitialize(
-    BACNET_REINITIALIZE_DEVICE_DATA * rd_data)
+bool Device_Reinitialize(BACNET_REINITIALIZE_DEVICE_DATA *rd_data)
 {
    bool status = false;

@@ -453,6 +448,7 @@ int Device_Set_System_Status(BACNET_DEVICE_STATUS status, bool local)
    /*return value - 0 = ok, -1 = bad value, -2 = not allowed */
    int result = -1;

+    (void)local;
    if (status < MAX_DEVICE_STATUS) {
        System_Status = status;
        result = 0;
@@ -532,6 +528,39 @@ bool Device_Object_List_Identifier(
    return status;
 }

+/**
+ * @brief Encode a BACnetARRAY property element
+ * @param object_instance [in] BACnet network port object instance number
+ * @param array_index [in] array index requested:
+ *    0 to N for individual array members
+ * @param apdu [out] Buffer in which the APDU contents are built, or NULL to
+ * return the length of buffer if it had been built
+ * @return The length of the apdu encoded or
+ *   BACNET_STATUS_ERROR for ERROR_CODE_INVALID_ARRAY_INDEX
+ */
+int Device_Object_List_Element_Encode(
+    uint32_t object_instance, BACNET_ARRAY_INDEX array_index, uint8_t *apdu)
+{
+    int apdu_len = BACNET_STATUS_ERROR;
+    BACNET_OBJECT_TYPE object_type;
+    uint32_t instance;
+    bool found;
+
+    if (object_instance == Device_Object_Instance_Number()) {
+        /* single element is zero based, add 1 for BACnetARRAY which is one
+         * based */
+        array_index++;
+        found =
+            Device_Object_List_Identifier(array_index, &object_type, &instance);
+        if (found) {
+            apdu_len =
+                encode_application_object_id(apdu, object_type, instance);
+        }
+    }
+
+    return apdu_len;
+}
+
 bool Device_Valid_Object_Name(BACNET_CHARACTER_STRING *object_name1,
    BACNET_OBJECT_TYPE *object_type,
    uint32_t *object_instance)
@@ -600,14 +629,12 @@ bool Device_Object_Name_Copy(BACNET_OBJECT_TYPE object_type,
 int Device_Read_Property_Local(BACNET_READ_PROPERTY_DATA *rpdata)
 {
    int apdu_len = 0; /* return value */
-    int len = 0; /* apdu len intermediate value */
    BACNET_BIT_STRING bit_string;
    BACNET_CHARACTER_STRING char_string;
    uint32_t i = 0;
-    BACNET_OBJECT_TYPE object_type = OBJECT_NONE;
-    uint32_t instance = 0;
    uint32_t count = 0;
    uint8_t *apdu = NULL;
+    int apdu_max = 0;
    struct my_object_functions *pObject = NULL;

    if ((rpdata->application_data == NULL) ||
@@ -615,6 +642,7 @@ int Device_Read_Property_Local(BACNET_READ_PROPERTY_DATA *rpdata)
        return 0;
    }
    apdu = rpdata->application_data;
+    apdu_max = rpdata->application_data_len;
    switch ((int)rpdata->object_property) {
        case PROP_DESCRIPTION:
            characterstring_init_ansi(&char_string, "BACnet Development Kit");
@@ -692,47 +720,15 @@ int Device_Read_Property_Local(BACNET_READ_PROPERTY_DATA *rpdata)
            break;
        case PROP_OBJECT_LIST:
            count = Device_Object_List_Count();
-            /* Array element zero is the number of objects in the list */
-            if (rpdata->array_index == 0)
-                apdu_len = encode_application_unsigned(&apdu[0], count);
-            /* if no index was specified, then try to encode the entire list */
-            /* into one packet.  Note that more than likely you will have */
-            /* to return an error if the number of encoded objects exceeds */
-            /* your maximum APDU size. */
-            else if (rpdata->array_index == BACNET_ARRAY_ALL) {
-                for (i = 1; i <= count; i++) {
-                    if (Device_Object_List_Identifier(
-                            i, &object_type, &instance)) {
-                        len = encode_application_object_id(
-                            &apdu[apdu_len], object_type, instance);
-                        apdu_len += len;
-                        /* assume next one is the same size as this one */
-                        /* can we all fit into the APDU? */
-                        if ((apdu_len + len) >= MAX_APDU) {
-                            /* Abort response */
-                            rpdata->error_code =
-                                ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED;
-                            apdu_len = BACNET_STATUS_ABORT;
-                            break;
-                        }
-                    } else {
-                        /* error: internal error? */
-                        rpdata->error_class = ERROR_CLASS_SERVICES;
-                        rpdata->error_code = ERROR_CODE_OTHER;
-                        apdu_len = BACNET_STATUS_ERROR;
-                        break;
-                    }
-                }
-            } else {
-                if (Device_Object_List_Identifier(
-                        rpdata->array_index, &object_type, &instance))
-                    apdu_len = encode_application_object_id(
-                        &apdu[0], object_type, instance);
-                else {
-                    rpdata->error_class = ERROR_CLASS_PROPERTY;
-                    rpdata->error_code = ERROR_CODE_INVALID_ARRAY_INDEX;
-                    apdu_len = BACNET_STATUS_ERROR;
-                }
+            apdu_len = bacnet_array_encode(rpdata->object_instance,
+                rpdata->array_index, Device_Object_List_Element_Encode, count,
+                apdu, apdu_max);
+            if (apdu_len == BACNET_STATUS_ABORT) {
+                rpdata->error_code =
+                    ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED;
+            } else if (apdu_len == BACNET_STATUS_ERROR) {
+                rpdata->error_class = ERROR_CLASS_PROPERTY;
+                rpdata->error_code = ERROR_CODE_INVALID_ARRAY_INDEX;
            }
            break;
        case PROP_MAX_APDU_LENGTH_ACCEPTED:
@@ -254,54 +254,96 @@ static bool dlmstp_compare_data_expecting_reply(uint8_t *request_pdu,
    };
    struct DER_compare_t request;
    struct DER_compare_t reply;
+    bool request_segmented = false;
+    bool reply_segmented = false;

    /* decode the request data */
    request.address.mac[0] = src_address;
    request.address.mac_len = 1;
-    offset = npdu_decode(
-        &request_pdu[0], NULL, &request.address, &request.npdu_data);
+    offset = bacnet_npdu_decode(request_pdu, request_pdu_len, NULL,
+        &request.address, &request.npdu_data);
    if (request.npdu_data.network_layer_message) {
        return false;
    }
+    if (offset >= request_pdu_len) {
+        return false;
+    }
    request.pdu_type = request_pdu[offset] & 0xF0;
    if (request.pdu_type != PDU_TYPE_CONFIRMED_SERVICE_REQUEST) {
        return false;
    }
+    if (request_pdu[offset] & BIT(3)) {
+        request_segmented = true;
+    }
+    if ((offset + 2) >= request_pdu_len) {
+        return false;
+    }
    request.invoke_id = request_pdu[offset + 2];
    /* segmented message? */
-    if (request_pdu[offset] & BIT(3))
+    if (request_segmented) {
+        if ((offset + 5) >= request_pdu_len) {
+            return false;
+        }
        request.service_choice = request_pdu[offset + 5];
-    else
+    } else {
+        if ((offset + 3) >= request_pdu_len) {
+            return false;
+        }
        request.service_choice = request_pdu[offset + 3];
+    }
    /* decode the reply data */
    reply.address.mac[0] = dest_address;
    reply.address.mac_len = 1;
-    offset = npdu_decode(&reply_pdu[0], &reply.address, NULL, &reply.npdu_data);
+    offset = bacnet_npdu_decode(
+        reply_pdu, reply_pdu_len, &reply.address, NULL, &reply.npdu_data);
    if (reply.npdu_data.network_layer_message) {
        return false;
    }
+    if (offset >= request_pdu_len) {
+        return false;
+    }
+    reply.pdu_type = reply_pdu[offset] & 0xF0;
+    if (reply_pdu[offset] & BIT(3)) {
+        reply_segmented = true;
+    }
    /* reply could be a lot of things:
       confirmed, simple ack, abort, reject, error */
-    reply.pdu_type = reply_pdu[offset] & 0xF0;
    switch (reply.pdu_type) {
        case PDU_TYPE_SIMPLE_ACK:
+            if ((offset + 2) >= request_pdu_len) {
+                return false;
+            }
            reply.invoke_id = reply_pdu[offset + 1];
            reply.service_choice = reply_pdu[offset + 2];
            break;
        case PDU_TYPE_COMPLEX_ACK:
-            reply.invoke_id = reply_pdu[offset + 1];
            /* segmented message? */
-            if (reply_pdu[offset] & BIT(3))
+            if (reply_segmented) {
+                if ((offset + 4) >= request_pdu_len) {
+                    return false;
+                }
+                reply.invoke_id = reply_pdu[offset + 1];
                reply.service_choice = reply_pdu[offset + 4];
-            else
+            } else {
+                if ((offset + 2) >= request_pdu_len) {
+                    return false;
+                }
+                reply.invoke_id = reply_pdu[offset + 1];
                reply.service_choice = reply_pdu[offset + 2];
+            }
            break;
        case PDU_TYPE_ERROR:
+            if ((offset + 2) >= request_pdu_len) {
+                return false;
+            }
            reply.invoke_id = reply_pdu[offset + 1];
            reply.service_choice = reply_pdu[offset + 2];
            break;
        case PDU_TYPE_REJECT:
        case PDU_TYPE_ABORT:
+            if ((offset + 1) >= request_pdu_len) {
+                return false;
+            }
            reply.invoke_id = reply_pdu[offset + 1];
            break;
        default:
@@ -47,7 +47,7 @@
 /* MS/TP specific */
 #include "bacnet/datalink/dlmstp.h"
 #include "rs485.h"
-//#include "nvdata.h"
+// #include "nvdata.h"
 /* me */
 #include "bacnet/basic/object/netport.h"

@@ -370,6 +370,8 @@ bool Network_Port_Link_Speed_Set(uint32_t object_instance, float value)
            Object_List[0].Changes_Pending = true;
            status = true;
            break;
+        default:
+            break;
    }

    return status;