From 094ac1db00b36b020dbaeb5d5e2a5fd3c1e709ea Mon Sep 17 00:00:00 2001 From: Roy Schneider Date: Mon, 11 May 2020 15:30:18 +0200 Subject: [PATCH] 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 | 19 ++++- src/bacnet/basic/npdu/h_npdu.c | 2 +- src/bacnet/npdu.c | 128 ++++++++++++++++++++++----------- src/bacnet/npdu.h | 8 +++ src/bacnet/whohas.c | 90 ++++++++++++++--------- 5 files changed, 170 insertions(+), 77 deletions(-) diff --git a/src/bacnet/bacapp.c b/src/bacnet/bacapp.c index 0ccb21d3..7347114d 100644 --- a/src/bacnet/bacapp.c +++ b/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 - return the number of octets consumed. */ +/** @brief Decode the data and + * 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) { diff --git a/src/bacnet/basic/npdu/h_npdu.c b/src/bacnet/basic/npdu/h_npdu.c index a5eece46..446507bf 100644 --- a/src/bacnet/basic/npdu/h_npdu.c +++ b/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 diff --git a/src/bacnet/npdu.c b/src/bacnet/npdu.c index 848c279b..6ebfb013 100644 --- a/src/bacnet/npdu.c +++ b/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); - /* DLEN = 0 denotes broadcast MAC DADR and DADR field is absent */ - /* DLEN > 0 specifies length of DADR field */ - dlen = npdu[len++]; - if (dest) { - 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 (pdu_len >= (len + 3)) { + len += decode_unsigned16(&npdu[len], &dest_net); + /* DLEN = 0 denotes broadcast MAC DADR and DADR field is absent */ + /* DLEN > 0 specifies length of DADR field */ + dlen = npdu[len++]; + if (dest) { + dest->net = dest_net; + dest->len = dlen; } + 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++) { - mac_octet = npdu[len++]; - if (dest) { - dest->adr[i] = mac_octet; + for (i = 0; i < dlen; i++) { + mac_octet = npdu[len++]; + if (dest) { + 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); - /* SLEN = 0 denotes broadcast MAC SADR and SADR field is absent */ - /* SLEN > 0 specifies length of SADR field */ - slen = npdu[len++]; - if (src) { - 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 (pdu_len >= (len + 3)) { + len += decode_unsigned16(&npdu[len], &src_net); + /* SLEN = 0 denotes broadcast MAC SADR and SADR field is absent */ + /* SLEN > 0 specifies length of SADR field */ + slen = npdu[len++]; + if (src) { + src->net = src_net; + src->len = slen; } + 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++) { - mac_octet = npdu[len++]; - if (src) { - src->adr[i] = mac_octet; + for (i = 0; i < slen; i++) { + mac_octet = npdu[len++]; + if (src) { + 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 = - (BACNET_NETWORK_MESSAGE_TYPE)npdu[len++]; - /* Message Type field contains a value in the range 0x80 - 0xFF, */ - /* then a Vendor ID field shall be present */ - if (npdu_data->network_message_type >= 0x80) { - len += decode_unsigned16(&npdu[len], &npdu_data->vendor_id); + if (pdu_len > len) { + npdu_data->network_message_type = + (BACNET_NETWORK_MESSAGE_TYPE)npdu[len++]; + /* Message Type field contains a value in the range 0x80 - 0xFF, */ + /* then a Vendor ID field shall be present */ + 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, diff --git a/src/bacnet/npdu.h b/src/bacnet/npdu.h index 54894f55..010802c2 100644 --- a/src/bacnet/npdu.h +++ b/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 */ diff --git a/src/bacnet/whohas.c b/src/bacnet/whohas.c index 9824f649..242c2cde 100644 --- a/src/bacnet/whohas.c +++ b/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_value <= BACNET_MAX_INSTANCE) { - data->low_limit = unsigned_value; - } - if (!decode_is_context_tag(&apdu[len], 1)) { - return -1; - } - len += decode_tag_number_and_value( - &apdu[len], &tag_number, &len_value); - len += decode_unsigned(&apdu[len], len_value, &unsigned_value); - if (unsigned_value <= BACNET_MAX_INSTANCE) { - data->high_limit = unsigned_value; + if ((unsigned)len < apdu_len) { + len += decode_unsigned(&apdu[len], len_value, &unsigned_value); + if ((unsigned)len < apdu_len) { + if (unsigned_value <= BACNET_MAX_INSTANCE) { + data->low_limit = unsigned_value; + } + if (!decode_is_context_tag(&apdu[len], 1)) { + return -1; + } + len += decode_tag_number_and_value( + &apdu[len], &tag_number, &len_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)) { - data->is_object_name = false; - len += decode_tag_number_and_value( - &apdu[len], &tag_number, &len_value); - len += decode_object_id( - &apdu[len], &decoded_type, &data->object.identifier.instance); - data->object.identifier.type = decoded_type; - } - /* object name */ - else if (decode_is_context_tag(&apdu[len], 3)) { - data->is_object_name = true; - len += decode_tag_number_and_value( - &apdu[len], &tag_number, &len_value); - len += decode_character_string( - &apdu[len], len_value, &data->object.name); - } - /* missing required parameters */ - else { + if ((unsigned)len < apdu_len) { + if (decode_is_context_tag(&apdu[len], 2)) { + data->is_object_name = false; + len += decode_tag_number_and_value( + &apdu[len], &tag_number, &len_value); + if ((unsigned)len < apdu_len) { + len += decode_object_id( + &apdu[len], &decoded_type, &data->object.identifier.instance); + data->object.identifier.type = decoded_type; + } + } + /* object name */ + else if (decode_is_context_tag(&apdu[len], 3)) { + data->is_object_name = true; + len += decode_tag_number_and_value( + &apdu[len], &tag_number, &len_value); + if ((unsigned)len < apdu_len) { + len += decode_character_string( + &apdu[len], len_value, &data->object.name); + } + } else { + /* missing required parameters */ + return -1; + } + } else { + /* message too short */ return -1; } }