Feature/safe decode npdu (#84)

* Added safe version to decode npdu

* Calling safe decoder function.

* Added comments and apdu_len checks.

* Fixed signed/unsigned warnings.

src/bacnet/bacapp.c

@@ -54,6 +54,14 @@
 #define snprintf _snprintf
 #endif
 
+/** @brief Encode application data given by a pointer into the APDU.
+ *  Return the number encoded bytes.
+ *
+ * @param apdu  Pointer to the buffer to encode to.
+ * @param value  Pointer to the application data.
+ *
+ * @return Bytes encoded.
+ */
 int bacapp_encode_application_data(
     uint8_t *apdu, BACNET_APPLICATION_DATA_VALUE *value)
 {
@@ -395,8 +403,15 @@ bool bacapp_decode_application_data_safe(uint8_t *new_apdu,
     return ret;
 }
 
-/* Decode the data and
+/** @brief Decode the data and
-   return the number of octets consumed. */
+ *  return the number of octets consumed.
+ *
+ *  @param apdu  Pointer to the received data.
+ *  @param tag_data_type  Data type to be decoded.
+ *  @param len_value_type  Length of the data in bytes.
+ *
+ *  @return Count of bytes decoded.
+ */
 int bacapp_decode_data_len(
     uint8_t *apdu, uint8_t tag_data_type, uint32_t len_value_type)
 {

src/bacnet/basic/npdu/h_npdu.c

@@ -76,7 +76,7 @@ void npdu_handler(BACNET_ADDRESS *src, /* source address */
 
     /* only handle the version that we know how to handle */
     if (pdu[0] == BACNET_PROTOCOL_VERSION) {
-        apdu_offset = npdu_decode(&pdu[0], &dest, src, &npdu_data);
+        apdu_offset = bacnet_npdu_decode(&pdu[0], pdu_len, &dest, src, &npdu_data);
         if (npdu_data.network_layer_message) {
             /*FIXME: network layer message received!  Handle it! */
 #if PRINT_ENABLED

src/bacnet/npdu.c

@@ -286,6 +286,7 @@ void npdu_encode_npdu_data(BACNET_NPDU_DATA *npdu_data,
     }
 }
 
+
 /** Decode the NPDU portion of a received message, particularly the NCPI byte.
  *  The Network Layer Protocol Control Information byte is described
  *  in section 6.2.2 of the BACnet standard.
@@ -302,6 +303,38 @@ void npdu_encode_npdu_data(BACNET_NPDU_DATA *npdu_data,
  *                   This src describes the original source of the message when
  *                   it had to be routed to reach this BACnet Device.
  * @param npdu_data [out] Returns a filled-out structure with information
+ *                   decoded from the NCPI and other NPDU
+ * bytes.
+ * @return On success, returns the number of bytes which were decoded from the
+ *         NPDU section; if this is a  network layer message, there may
+ * be more bytes left in the NPDU; if not a network msg, the APDU follows. If 0
+ * or negative, there were problems with the data or arguments.
+ */
+int npdu_decode(uint8_t *npdu,
+    BACNET_ADDRESS *dest,
+    BACNET_ADDRESS *src,
+    BACNET_NPDU_DATA *npdu_data)
+{
+    return bacnet_npdu_decode(npdu, MAX_NPDU, dest, src, npdu_data);
+}
+
+/** Decode the NPDU portion of a received message, particularly the NCPI byte.
+ *  The Network Layer Protocol Control Information byte is described
+ *  in section 6.2.2 of the BACnet standard.
+ * @param npdu [in] Buffer holding the received NPDU header bytes (must be at
+ * least 2)
+ * @param pdu_len [in] Length of the received data to prevent overruns.
+ * @param dest [out] Returned with routing destination information if the NPDU
+ *                   has any and if this points to non-null storage for it.
+ *                   If dest->net and dest->len are 0 on return, there is no
+ *                   routing destination information.
+ * @param src  [out] Returned with routing source information if the NPDU
+ *                   has any and if this points to non-null storage for it.
+ *                   If src->net and src->len are 0 on return, there is no
+ *                   routing source information.
+ *                   This src describes the original source of the message when
+ *                   it had to be routed to reach this BACnet Device.
+ * @param npdu_data [out] Returns a filled-out structure with information
  * 					 decoded from the NCPI and other NPDU
  * bytes.
  * @return On success, returns the number of bytes which were decoded from the
@@ -309,7 +342,8 @@ void npdu_encode_npdu_data(BACNET_NPDU_DATA *npdu_data,
  * be more bytes left in the NPDU; if not a network msg, the APDU follows. If 0
  * or negative, there were problems with the data or arguments.
  */
-int npdu_decode(uint8_t *npdu,
+int bacnet_npdu_decode(uint8_t *npdu,
+    uint16_t pdu_len,
     BACNET_ADDRESS *dest,
     BACNET_ADDRESS *src,
     BACNET_NPDU_DATA *npdu_data)
@@ -322,7 +356,7 @@ int npdu_decode(uint8_t *npdu,
     uint8_t dlen = 0;
     uint8_t mac_octet = 0;
 
-    if (npdu && npdu_data) {
+    if (npdu && npdu_data && (pdu_len >= 2)) {
         /* Protocol Version */
         npdu_data->protocol_version = npdu[0];
         /* control octet */
@@ -356,24 +390,26 @@ int npdu_decode(uint8_t *npdu,
         /* DLEN = 0 denotes broadcast MAC DADR and DADR field is absent */
         /* DLEN > 0 specifies length of DADR field */
         if (npdu[1] & BIT(5)) {
-            len += decode_unsigned16(&npdu[len], &dest_net);
+            if (pdu_len >= (len + 3)) {
-            /* DLEN = 0 denotes broadcast MAC DADR and DADR field is absent */
+                len += decode_unsigned16(&npdu[len], &dest_net);
-            /* DLEN > 0 specifies length of DADR field */
+                /* DLEN = 0 denotes broadcast MAC DADR and DADR field is absent */
-            dlen = npdu[len++];
+                /* DLEN > 0 specifies length of DADR field */
-            if (dest) {
+                dlen = npdu[len++];
-                dest->net = dest_net;
+                if (dest) {
-                dest->len = dlen;
+                    dest->net = dest_net;
-            }
+                    dest->len = dlen;
-            if (dlen) {
-                if (dlen > MAX_MAC_LEN) {
-                    /* address is too large could be a malformed message */
-                    return -1;
                 }
+                if (dlen) {
+                    if ((dlen > MAX_MAC_LEN) || (pdu_len < (len + dlen))) {
+                        /* address is too large could be a malformed message */
+                        return -1;
+                    }
 
-                for (i = 0; i < dlen; i++) {
+                    for (i = 0; i < dlen; i++) {
-                    mac_octet = npdu[len++];
+                        mac_octet = npdu[len++];
-                    if (dest) {
+                        if (dest) {
-                        dest->adr[i] = mac_octet;
+                            dest->adr[i] = mac_octet;
+                        }
                     }
                 }
             }
@@ -390,24 +426,26 @@ int npdu_decode(uint8_t *npdu,
         /* 0 =  SNET, SLEN, and SADR absent */
         /* 1 =  SNET, SLEN, and SADR present */
         if (npdu[1] & BIT(3)) {
-            len += decode_unsigned16(&npdu[len], &src_net);
+            if (pdu_len >= (len + 3)) {
-            /* SLEN = 0 denotes broadcast MAC SADR and SADR field is absent */
+                len += decode_unsigned16(&npdu[len], &src_net);
-            /* SLEN > 0 specifies length of SADR field */
+                /* SLEN = 0 denotes broadcast MAC SADR and SADR field is absent */
-            slen = npdu[len++];
+                /* SLEN > 0 specifies length of SADR field */
-            if (src) {
+                slen = npdu[len++];
-                src->net = src_net;
+                if (src) {
-                src->len = slen;
+                    src->net = src_net;
-            }
+                    src->len = slen;
-            if (slen) {
-                if (slen > MAX_MAC_LEN) {
-                    /* address is too large could be a malformed message */
-                    return -1;
                 }
+                if (slen) {
+                    if ((slen > MAX_MAC_LEN) || (pdu_len < (len + slen))) {
+                        /* address is too large could be a malformed message */
+                        return -1;
+                    }
 
-                for (i = 0; i < slen; i++) {
+                    for (i = 0; i < slen; i++) {
-                    mac_octet = npdu[len++];
+                        mac_octet = npdu[len++];
-                    if (src) {
+                        if (src) {
-                        src->adr[i] = mac_octet;
+                            src->adr[i] = mac_octet;
+                        }
                     }
                 }
             }
@@ -428,19 +466,27 @@ int npdu_decode(uint8_t *npdu,
         /* destined for a remote network, i.e., if DNET is present. */
         /* This is a one-octet field that is initialized to a value of 0xff. */
         if (dest_net) {
-            npdu_data->hop_count = npdu[len++];
+            if (pdu_len > len) {
+                npdu_data->hop_count = npdu[len++];
+            } else {
+                npdu_data->hop_count = 0;
+            }
         } else {
             npdu_data->hop_count = 0;
         }
         /* Indicates that the NSDU conveys a network layer message. */
         /* Message Type field is present. */
         if (npdu_data->network_layer_message) {
-            npdu_data->network_message_type =
+            if (pdu_len > len) {
-                (BACNET_NETWORK_MESSAGE_TYPE)npdu[len++];
+                npdu_data->network_message_type =
-            /* Message Type field contains a value in the range 0x80 - 0xFF, */
+                    (BACNET_NETWORK_MESSAGE_TYPE)npdu[len++];
-            /* then a Vendor ID field shall be present */
+                /* Message Type field contains a value in the range 0x80 - 0xFF, */
-            if (npdu_data->network_message_type >= 0x80) {
+                /* then a Vendor ID field shall be present */
-                len += decode_unsigned16(&npdu[len], &npdu_data->vendor_id);
+                if (npdu_data->network_message_type >= 0x80) {
+                    if (pdu_len >= (len + 2)) {
+                        len += decode_unsigned16(&npdu[len], &npdu_data->vendor_id);
+                    }
+                }
             }
         } else {
             /* Since npdu_data->network_layer_message is false,

src/bacnet/npdu.h

@@ -95,6 +95,14 @@ extern "C" {
         BACNET_ADDRESS * src,
         BACNET_NPDU_DATA * npdu_data);
 
+    BACNET_STACK_EXPORT
+    int bacnet_npdu_decode(
+        uint8_t * npdu,
+        uint16_t pdu_len,
+        BACNET_ADDRESS * dest,
+        BACNET_ADDRESS * src,
+        BACNET_NPDU_DATA * npdu_data);
+
 #ifdef __cplusplus
 }
 #endif /* __cplusplus */

src/bacnet/whohas.c

@@ -39,8 +39,12 @@
 
 /** @file whohas.c  Encode/Decode Who-Has requests */
 
-/* encode service  - use -1 for limit for unlimited */
+/** Encode Who-Hasservice  - use -1 for limit for unlimited
-
+ *
+ * @param apdu  Pointer to the APDU.
+ * @param data  Pointer to the Who-Has application data.
+ *
+ * @return Bytes encoded. */
 int whohas_encode_apdu(uint8_t *apdu, BACNET_WHO_HAS_DATA *data)
 {
     int len = 0; /* length of each encoding */
@@ -75,7 +79,13 @@ int whohas_encode_apdu(uint8_t *apdu, BACNET_WHO_HAS_DATA *data)
     return apdu_len;
 }
 
-/* decode the service request only */
+/** Decode the Who-Has service request only.
+ *
+ * @param apdu  Pointer to the received data.
+ * @param apdu_len  Bytes valid in the receive buffer.
+ * @param data  Pointer to the application dta to be filled in.
+ *
+ * @return Bytes decoded. */
 int whohas_decode_service_request(
     uint8_t *apdu, unsigned apdu_len, BACNET_WHO_HAS_DATA *data)
 {
@@ -90,42 +100,56 @@ int whohas_decode_service_request(
         if (decode_is_context_tag(&apdu[len], 0)) {
             len += decode_tag_number_and_value(
                 &apdu[len], &tag_number, &len_value);
-            len += decode_unsigned(&apdu[len], len_value, &unsigned_value);
+            if ((unsigned)len < apdu_len) {
-            if (unsigned_value <= BACNET_MAX_INSTANCE) {
+                len += decode_unsigned(&apdu[len], len_value, &unsigned_value);
-                data->low_limit = unsigned_value;
+                if ((unsigned)len < apdu_len) {
-            }
+                    if (unsigned_value <= BACNET_MAX_INSTANCE) {
-            if (!decode_is_context_tag(&apdu[len], 1)) {
+                        data->low_limit = unsigned_value;
-                return -1;
+                    }
-            }
+                    if (!decode_is_context_tag(&apdu[len], 1)) {
-            len += decode_tag_number_and_value(
+                        return -1;
-                &apdu[len], &tag_number, &len_value);
+                    }
-            len += decode_unsigned(&apdu[len], len_value, &unsigned_value);
+                    len += decode_tag_number_and_value(
-            if (unsigned_value <= BACNET_MAX_INSTANCE) {
+                        &apdu[len], &tag_number, &len_value);
-                data->high_limit = unsigned_value;
+                    if ((unsigned)len < apdu_len) {
+                        len += decode_unsigned(&apdu[len], len_value, &unsigned_value);
+                        if (unsigned_value <= BACNET_MAX_INSTANCE) {
+                            data->high_limit = unsigned_value;
+                        }
+                    }
+                }
             }
         } else {
             data->low_limit = -1;
             data->high_limit = -1;
         }
         /* object id */
-        if (decode_is_context_tag(&apdu[len], 2)) {
+        if ((unsigned)len < apdu_len) {
-            data->is_object_name = false;
+            if (decode_is_context_tag(&apdu[len], 2)) {
-            len += decode_tag_number_and_value(
+                data->is_object_name = false;
-                &apdu[len], &tag_number, &len_value);
+                len += decode_tag_number_and_value(
-            len += decode_object_id(
+                    &apdu[len], &tag_number, &len_value);
-                &apdu[len], &decoded_type, &data->object.identifier.instance);
+                if ((unsigned)len < apdu_len) {
-            data->object.identifier.type = decoded_type;
+                    len += decode_object_id(
-        }
+                        &apdu[len], &decoded_type, &data->object.identifier.instance);
-        /* object name */
+                    data->object.identifier.type = decoded_type;
-        else if (decode_is_context_tag(&apdu[len], 3)) {
+                }
-            data->is_object_name = true;
+            }
-            len += decode_tag_number_and_value(
+            /* object name */
-                &apdu[len], &tag_number, &len_value);
+            else if (decode_is_context_tag(&apdu[len], 3)) {
-            len += decode_character_string(
+                data->is_object_name = true;
-                &apdu[len], len_value, &data->object.name);
+                len += decode_tag_number_and_value(
-        }
+                    &apdu[len], &tag_number, &len_value);
-        /* missing required parameters */
+                if ((unsigned)len < apdu_len) {
-        else {
+                    len += decode_character_string(
+                        &apdu[len], len_value, &data->object.name);
+                }
+            } else {
+                /* missing required parameters */
+                return -1;
+            }
+        } else {
+            /* message too short */
             return -1;
         }
     }