Fix network control handler to ensure proper offset calculation and prevent buffer overrun (bacnet-stack#1387)

skarg · web-flow · commit 4ed32fcd06e0 · 2026-06-16T10:33:35.000-05:00
* Fix out-of-bounds read in network control handler by validating NPDU length and adjusting offsets

* Refactor network control handler to use dedicated processing functions for I-Am-Router and Init Routing Table messages, improving code clarity and maintainability.
diff --git a/apps/router-ipv6/main.c b/apps/router-ipv6/main.c
@@ -638,9 +638,7 @@ static void network_control_handler(
     uint8_t *npdu,
     uint16_t npdu_len)
 {
-    uint16_t npdu_offset = 0;
     uint16_t dnet = 0;
-    uint16_t len = 0;
     const char *msg_name = NULL;
 
     msg_name = bactext_network_layer_msg_name(npdu_data->network_message_type);
@@ -652,19 +650,8 @@ static void network_control_handler(
             break;
         case NETWORK_MESSAGE_I_AM_ROUTER_TO_NETWORK:
             /* add its DNETs to our routing table */
-            fprintf(stderr, "for Networks: ");
-            len = 2;
-            while (npdu_len >= len) {
-                len = decode_unsigned16(&npdu[npdu_offset], &dnet);
-                fprintf(stderr, "%hu", dnet);
-                dnet_add(snet, dnet, src);
-                npdu_len -= len;
-                npdu_offset += len;
-                if (npdu_len) {
-                    fprintf(stderr, ", ");
-                }
-            }
-            fprintf(stderr, ".\n");
+            npdu_i_am_router_to_network_process(
+                snet, src, npdu, npdu_len, dnet_add);
             break;
         case NETWORK_MESSAGE_I_COULD_BE_ROUTER_TO_NETWORK:
             /* Do nothing, same as previous case. */
@@ -709,32 +696,10 @@ static void network_control_handler(
         case NETWORK_MESSAGE_INIT_RT_TABLE:
             /* If sent with Number of Ports == 0, we respond with
              * NETWORK_MESSAGE_INIT_RT_TABLE_ACK and a list of all our
-             * reachable networks.
-             */
-            if (npdu_len > 0) {
-                /* If Number of Ports is 0, broadcast our "full" table */
-                if (npdu[0] == 0) {
-                    send_initialize_routing_table_ack(snet, NULL);
-                } else {
-                    /* they sent us a list */
-                    int net_count = npdu[0];
-                    while (net_count--) {
-                        int i = 1;
-                        /* DNET */
-                        decode_unsigned16(&npdu[i], &dnet);
-                        /* update routing table */
-                        dnet_add(snet, dnet, src);
-                        if (npdu[i + 3] > 0) {
-                            /* find next NET value */
-                            i = npdu[i + 3] + 4;
-                        } else {
-                            i += 4;
-                        }
-                    }
-                    send_initialize_routing_table_ack(snet, NULL);
-                }
-                break;
-            }
+             * reachable networks. */
+            npdu_init_routing_table_process(
+                snet, src, npdu, npdu_len, dnet_add);
+            send_initialize_routing_table_ack(snet, NULL);
             break;
         case NETWORK_MESSAGE_INIT_RT_TABLE_ACK:
             /* Do nothing with the routing table info, since don't support
diff --git a/apps/router-mstp/main.c b/apps/router-mstp/main.c
@@ -659,9 +659,7 @@ static void network_control_handler(
     uint8_t *npdu,
     uint16_t npdu_len)
 {
-    uint16_t npdu_offset = 0;
     uint16_t dnet = 0;
-    uint16_t len = 0;
     const char *msg_name = NULL;
 
     (void)src;
@@ -675,19 +673,8 @@ static void network_control_handler(
             break;
         case NETWORK_MESSAGE_I_AM_ROUTER_TO_NETWORK:
             /* add its DNETs to our routing table */
-            fprintf(stderr, "for Networks: ");
-            len = 2;
-            while (npdu_len >= len) {
-                len = decode_unsigned16(&npdu[npdu_offset], &dnet);
-                fprintf(stderr, "%hu", dnet);
-                dnet_add(snet, dnet, src);
-                npdu_len -= len;
-                npdu_offset += len;
-                if (npdu_len) {
-                    fprintf(stderr, ", ");
-                }
-            }
-            fprintf(stderr, ".\n");
+            npdu_i_am_router_to_network_process(
+                snet, src, npdu, npdu_len, dnet_add);
             break;
         case NETWORK_MESSAGE_I_COULD_BE_ROUTER_TO_NETWORK:
             /* Do nothing, same as previous case. */
@@ -734,30 +721,9 @@ static void network_control_handler(
              * NETWORK_MESSAGE_INIT_RT_TABLE_ACK and a list of all our
              * reachable networks.
              */
-            if (npdu_len > 0) {
-                /* If Number of Ports is 0, broadcast our "full" table */
-                if (npdu[0] == 0) {
-                    send_initialize_routing_table_ack(snet, NULL);
-                } else {
-                    /* they sent us a list */
-                    int net_count = npdu[0];
-                    while (net_count--) {
-                        int i = 1;
-                        /* DNET */
-                        decode_unsigned16(&npdu[i], &dnet);
-                        /* update routing table */
-                        dnet_add(snet, dnet, src);
-                        if (npdu[i + 3] > 0) {
-                            /* find next NET value */
-                            i = npdu[i + 3] + 4;
-                        } else {
-                            i += 4;
-                        }
-                    }
-                    send_initialize_routing_table_ack(snet, NULL);
-                }
-                break;
-            }
+            npdu_init_routing_table_process(
+                snet, src, npdu, npdu_len, dnet_add);
+            send_initialize_routing_table_ack(snet, NULL);
             break;
         case NETWORK_MESSAGE_INIT_RT_TABLE_ACK:
             /* Do nothing with the routing table info, since don't support
diff --git a/ports/linux/dlmstp.c b/ports/linux/dlmstp.c
@@ -925,7 +925,7 @@ bool dlmstp_init(const char *ifname)
         RS485_Set_Interface(ifname);
         debug_fprintf(stderr, "MS/TP Interface: %s\n", ifname);
     } else {
-        ifname = (char *)RS485_Interface();
+        ifname = RS485_Interface();
     }
     pthread_condattr_init(&attr);
     if ((rv = pthread_condattr_setclock(&attr, CLOCK_MONOTONIC)) != 0) {
diff --git a/src/bacnet/npdu.c b/src/bacnet/npdu.c
@@ -890,3 +890,108 @@ bool npdu_is_data_expecting_reply(
         request_pdu, request_pdu_len, &request_address, reply_pdu,
         reply_pdu_len, &reply_address);
 }
+
+/**
+ * @brief Process the NPDU portion of an I-Am-Router-To-Network message, which
+ * contains a list of BACnet network numbers that the router is connected to.
+ * @param snet [in] The source network number of the I-Am-Router-To
+ * Network message, which is the network number of the router sending the
+ * message.
+ * @param src [in] The source address of the I-Am-Router-To-Network message,
+ * which is the address of the router sending the message.
+ * @param npdu [in] The buffer containing the NPDU portion of the
+ * I-Am-Router-To-Network message, which contains the list of BACnet network
+ * numbers that the router is connected to.
+ * @param npdu_size [in] The size of the npdu buffer in bytes.
+ * @param dnet_add [in] A callback function that will be called for each BACnet
+ * network number (DNET)
+ */
+void npdu_i_am_router_to_network_process(
+    uint16_t snet,
+    const BACNET_ADDRESS *src,
+    const uint8_t *npdu,
+    uint16_t npdu_size,
+    npdu_dnet_add_callback_t dnet_add)
+{
+    int len = 2;
+    uint16_t dnet = 0;
+    uint16_t npdu_offset = 0;
+    uint16_t npdu_len = npdu_size;
+
+    while (npdu_len >= len) {
+        len = decode_unsigned16(&npdu[npdu_offset], &dnet);
+        if (dnet_add) {
+            dnet_add(snet, dnet, src);
+        }
+        npdu_len -= len;
+        npdu_offset += len;
+    }
+}
+
+/**
+ * @brief Process the NPDU portion of an Initialize-Routing-Table message, which
+ * contains a list of BACnet network numbers (DNETs) and per-port information.
+ * @param snet [in] The source network number of the Initialize-Routing-Table
+ * message, which is the network number of the router sending the message.
+ * @param src [in] The source address of the I-Have-Router-To-Network message,
+ * which is the address of the router sending the message.
+ * @param npdu [in] The buffer containing the NPDU portion of the
+ * I-Have-Router-To-Network message, which contains the list of BACnet network
+ * numbers that the router is connected to, along with port information.
+ * @param npdu_size [in] The size of the npdu buffer in bytes.
+ * @param dnet_add [in] Optional callback invoked for each decoded DNET value.
+ * The callback receives the source network, decoded DNET, and source address.
+ */
+void npdu_init_routing_table_process(
+    uint16_t snet,
+    const BACNET_ADDRESS *src,
+    const uint8_t *npdu,
+    uint16_t npdu_size,
+    npdu_dnet_add_callback_t dnet_add)
+{
+    int len = 2;
+    uint16_t dnet = 0;
+    uint16_t npdu_offset = 0;
+    uint8_t port_id = 0;
+    uint8_t port_info_len = 0;
+    uint8_t net_count;
+    uint16_t npdu_len = npdu_size;
+
+    if (npdu_len <= 1) {
+        /* malformed message */
+        return;
+    }
+    net_count = npdu[npdu_offset];
+    npdu_offset += 1;
+    npdu_len -= 1;
+    if (net_count == 0) {
+        /* no networks, nothing to do */
+        return;
+    }
+    /* DNET(2) + PortID(1) + PortInfoLen(1) = 4 bytes */
+    while ((npdu_len >= 4) && (net_count--)) {
+        /* DNET */
+        len = decode_unsigned16(&npdu[npdu_offset], &dnet);
+        npdu_offset += len;
+        npdu_len -= len;
+        /* update routing table */
+        if (dnet_add) {
+            dnet_add(snet, dnet, src);
+        }
+        /* skip port_id & port_info */
+        port_id = npdu[npdu_offset];
+        npdu_offset += 1;
+        npdu_len -= 1;
+        port_info_len = npdu[npdu_offset];
+        npdu_offset += 1;
+        npdu_len -= 1;
+        if (npdu_len >= port_info_len) {
+            npdu_offset += port_info_len;
+            npdu_len -= port_info_len;
+        } else {
+            /* malformed message */
+            break;
+        }
+        (void)port_id;
+    }
+}
diff --git a/src/bacnet/npdu.h b/src/bacnet/npdu.h
@@ -54,6 +54,9 @@ typedef struct router_port_t {
     struct router_port_t *next; /**< Point to next in linked list */
 } BACNET_ROUTER_PORT;
 
+typedef void (*npdu_dnet_add_callback_t)(
+    uint16_t snet, uint16_t net, const BACNET_ADDRESS *addr);
+
 #define NETWORK_NUMBER_LEARNED 0
 #define NETWORK_NUMBER_CONFIGURED 1
 
@@ -123,13 +126,28 @@ bool npdu_is_expected_reply(
     const uint8_t *reply_pdu,
     uint16_t reply_pdu_len,
     BACNET_ADDRESS *reply_address);
+BACNET_STACK_EXPORT
 bool npdu_is_data_expecting_reply(
     const uint8_t *request_pdu,
     uint16_t request_pdu_len,
     uint8_t request_mac,
     const uint8_t *reply_pdu,
     uint16_t reply_pdu_len,
     uint8_t reply_mac);
+BACNET_STACK_EXPORT
+void npdu_i_am_router_to_network_process(
+    uint16_t snet,
+    const BACNET_ADDRESS *src,
+    const uint8_t *npdu,
+    uint16_t npdu_size,
+    npdu_dnet_add_callback_t dnet_add);
+BACNET_STACK_EXPORT
+void npdu_init_routing_table_process(
+    uint16_t snet,
+    const BACNET_ADDRESS *src,
+    const uint8_t *npdu,
+    uint16_t npdu_size,
+    npdu_dnet_add_callback_t dnet_add);
 
 #ifdef __cplusplus
 }
diff --git a/test/bacnet/npdu/src/main.c b/test/bacnet/npdu/src/main.c

Original file line number	Diff line number	Diff line change
`@@ -925,7 +925,7 @@ bool dlmstp_init(const char *ifname)`
`925`	`925`	`RS485_Set_Interface(ifname);`
`926`	`926`	`debug_fprintf(stderr, "MS/TP Interface: %s\n", ifname);`
`927`	`927`	`} else {`
`928`		`- ifname = (char *)RS485_Interface();`
	`928`	`+ ifname = RS485_Interface();`
`929`	`929`	`}`
`930`	`930`	`pthread_condattr_init(&attr);`
`931`	`931`	`if ((rv = pthread_condattr_setclock(&attr, CLOCK_MONOTONIC)) != 0) {`