From 05aaee7f6b2d429198285dd3b9c1b7022e62b96f Mon Sep 17 00:00:00 2001 From: Roy Schneider Date: Sat, 2 May 2020 03:29:21 +0200 Subject: [PATCH] Comments and length checks. (#81) * Comments and length checks. * Added max_apdu checking. --- src/bacnet/basic/npdu/h_npdu.c | 2 +- src/bacnet/basic/service/h_apdu.c | 12 ++-- src/bacnet/basic/tsm/tsm.c | 108 +++++++++++++++++++++++------- src/bacnet/timesync.c | 66 ++++++++++++++++-- 4 files changed, 154 insertions(+), 34 deletions(-) diff --git a/src/bacnet/basic/npdu/h_npdu.c b/src/bacnet/basic/npdu/h_npdu.c index fd0ded7f..a5eece46 100644 --- a/src/bacnet/basic/npdu/h_npdu.c +++ b/src/bacnet/basic/npdu/h_npdu.c @@ -82,7 +82,7 @@ void npdu_handler(BACNET_ADDRESS *src, /* source address */ #if PRINT_ENABLED fprintf(stderr, "NPDU: Network Layer Message discarded!\n"); #endif - } else if ((apdu_offset > 0) && (apdu_offset <= pdu_len)) { + } else if ((apdu_offset > 0) && (apdu_offset < pdu_len)) { if ((dest.net == 0) || (dest.net == BACNET_BROADCAST_NETWORK)) { /* only handle the version that we know how to handle */ /* and we are not a router, so ignore messages with diff --git a/src/bacnet/basic/service/h_apdu.c b/src/bacnet/basic/service/h_apdu.c index f186d22b..6092e602 100644 --- a/src/bacnet/basic/service/h_apdu.c +++ b/src/bacnet/basic/service/h_apdu.c @@ -674,11 +674,13 @@ void apdu_handler(BACNET_ADDRESS *src, len += decode_enumerated(&apdu[len], len_value, &error_code); if (service_choice == - SERVICE_CONFIRMED_PRIVATE_TRANSFER) { /* skip over - closing tag 0 */ - if (decode_is_closing_tag_number(&apdu[len], 0)) { - len++; /* a tag number of 0 is not extended so only one - octet */ + SERVICE_CONFIRMED_PRIVATE_TRANSFER) { + if (len < apdu_len) { + /* skip over closing tag 0 */ + if (decode_is_closing_tag_number(&apdu[len], 0)) { + len++; /* a tag number of 0 is not extended so + only one octet */ + } } } } diff --git a/src/bacnet/basic/tsm/tsm.c b/src/bacnet/basic/tsm/tsm.c index 227692c6..d8197752 100644 --- a/src/bacnet/basic/tsm/tsm.c +++ b/src/bacnet/basic/tsm/tsm.c @@ -73,14 +73,23 @@ void tsm_set_timeout_handler(tsm_timeout_function pFunction) Timeout_Function = pFunction; } -/* returns MAX_TSM_TRANSACTIONS if not found */ +/** Find the given Invoke-Id in the list and + * return the index. + * + * @param invokeID Invoke Id + * + * @return Index of the id or MAX_TSM_TRANSACTIONS + * if not found + */ static uint8_t tsm_find_invokeID_index(uint8_t invokeID) { unsigned i = 0; /* counter */ uint8_t index = MAX_TSM_TRANSACTIONS; /* return value */ - for (i = 0; i < MAX_TSM_TRANSACTIONS; i++) { - if (TSM_List[i].InvokeID == invokeID) { + const BACNET_TSM_DATA *plist = TSM_List; + + for (i = 0; i < MAX_TSM_TRANSACTIONS; i++, plist++) { + if (plist->InvokeID == invokeID) { index = (uint8_t)i; break; } @@ -89,13 +98,20 @@ static uint8_t tsm_find_invokeID_index(uint8_t invokeID) return index; } +/** Find the first free index in the TSM table. + * + * @return Index of the id or MAX_TSM_TRANSACTIONS + * if no entry is free. + */ static uint8_t tsm_find_first_free_index(void) { unsigned i = 0; /* counter */ uint8_t index = MAX_TSM_TRANSACTIONS; /* return value */ - for (i = 0; i < MAX_TSM_TRANSACTIONS; i++) { - if (TSM_List[i].InvokeID == 0) { + const BACNET_TSM_DATA *plist = TSM_List; + + for (i = 0; i < MAX_TSM_TRANSACTIONS; i++, plist++) { + if (plist->InvokeID == 0) { index = (uint8_t)i; break; } @@ -104,13 +120,19 @@ static uint8_t tsm_find_first_free_index(void) return index; } +/** Check if space for transactions is available. + * + * @return true/false + */ bool tsm_transaction_available(void) { bool status = false; /* return value */ unsigned i = 0; /* counter */ - for (i = 0; i < MAX_TSM_TRANSACTIONS; i++) { - if (TSM_List[i].InvokeID == 0) { + const BACNET_TSM_DATA *plist = TSM_List; + + for (i = 0; i < MAX_TSM_TRANSACTIONS; i++, plist++) { + if (plist->InvokeID == 0) { /* one is available! */ status = true; break; @@ -120,14 +142,20 @@ bool tsm_transaction_available(void) return status; } +/** Return the count of idle transaction. + * + * @return Count of idle transaction. + */ uint8_t tsm_transaction_idle_count(void) { uint8_t count = 0; /* return value */ unsigned i = 0; /* counter */ - for (i = 0; i < MAX_TSM_TRANSACTIONS; i++) { - if ((TSM_List[i].InvokeID == 0) && - (TSM_List[i].state == TSM_STATE_IDLE)) { + const BACNET_TSM_DATA *plist = TSM_List; + + for (i = 0; i < MAX_TSM_TRANSACTIONS; i++, plist++) { + if ((plist->InvokeID == 0) && + (plist->state == TSM_STATE_IDLE)) { /* one is available! */ count++; } @@ -136,8 +164,11 @@ uint8_t tsm_transaction_idle_count(void) return count; } -/* sets the invokeID */ - +/** + * Sets the current invokeID. + * + * @param invokeID Invoke ID + */ void tsm_invokeID_set(uint8_t invokeID) { if (invokeID == 0) { @@ -146,16 +177,20 @@ void tsm_invokeID_set(uint8_t invokeID) Current_Invoke_ID = invokeID; } -/* gets the next free invokeID, - and reserves a spot in the table - returns 0 if none are available */ +/** Gets the next free invokeID, + * and reserves a spot in the table + * returns 0 if none are available. + * + * @return free invoke ID + */ uint8_t tsm_next_free_invokeID(void) { uint8_t index = 0; uint8_t invokeID = 0; bool found = false; + BACNET_TSM_DATA *plist = NULL; - /* is there even space available? */ + /* Is there even space available? */ if (tsm_transaction_available()) { while (!found) { index = tsm_find_invokeID_index(Current_Invoke_ID); @@ -165,9 +200,10 @@ uint8_t tsm_next_free_invokeID(void) /* set this id into the table */ index = tsm_find_first_free_index(); if (index != MAX_TSM_TRANSACTIONS) { - TSM_List[index].InvokeID = invokeID = Current_Invoke_ID; - TSM_List[index].state = TSM_STATE_IDLE; - TSM_List[index].RequestTimer = apdu_timeout(); + plist = &TSM_List[index]; + plist->InvokeID = invokeID = Current_Invoke_ID; + plist->state = TSM_STATE_IDLE; + plist->RequestTimer = apdu_timeout(); /* update for the next call or check */ Current_Invoke_ID++; /* skip zero - we treat that internally as invalid or no @@ -191,6 +227,15 @@ uint8_t tsm_next_free_invokeID(void) return invokeID; } +/** Set for an unsegmented transaction + * the state to await confirmation. + * + * @param invokeID Invoke-ID + * @param dest Pointer to the BACnet destination address. + * @param ndpu_data Pointer to the NPDU structure. + * @param apdu Pointer to the received message. + * @param apdu_len Bytes valid in the received message. + */ void tsm_set_confirmed_unsegmented_transaction(uint8_t invokeID, BACNET_ADDRESS *dest, BACNET_NPDU_DATA *ndpu_data, @@ -201,7 +246,7 @@ void tsm_set_confirmed_unsegmented_transaction(uint8_t invokeID, uint8_t index; BACNET_TSM_DATA *plist; - if (invokeID) { + if (invokeID && ndpu_data && apdu && (apdu_len > 0)) { index = tsm_find_invokeID_index(invokeID); if (index < MAX_TSM_TRANSACTIONS) { plist = &TSM_List[index]; @@ -223,8 +268,18 @@ void tsm_set_confirmed_unsegmented_transaction(uint8_t invokeID, return; } -/* used to retrieve the transaction payload */ -/* if we wanted to find out what we sent (i.e. when we get an ack) */ +/** Used to retrieve the transaction payload. Used + * if we wanted to find out what we sent (i.e. when + * we get an ack). + * + * @param invokeID Invoke-ID + * @param dest Pointer to the BACnet destination address. + * @param ndpu_data Pointer to the NPDU structure. + * @param apdu Pointer to the received message. + * @param apdu_len Pointer to a variable, that takes + * the count of bytes valid in the + * received message. + */ bool tsm_get_transaction_pdu(uint8_t invokeID, BACNET_ADDRESS *dest, BACNET_NPDU_DATA *ndpu_data, @@ -236,7 +291,7 @@ bool tsm_get_transaction_pdu(uint8_t invokeID, bool found = false; BACNET_TSM_DATA *plist; - if (invokeID) { + if (invokeID && apdu && ndpu_data && apdu_len) { index = tsm_find_invokeID_index(invokeID); /* how much checking is needed? state? dest match? just invokeID? */ if (index < MAX_TSM_TRANSACTIONS) { @@ -261,6 +316,8 @@ bool tsm_get_transaction_pdu(uint8_t invokeID, } /** Called once a millisecond or slower. + * This function calls the handler for a + * timeout 'Timeout_Function', if neccessary. * * @param milliseconds - Count of milliseconds passed, since the last call. */ @@ -300,7 +357,10 @@ void tsm_timer_milliseconds(uint16_t milliseconds) } } -/* frees the invokeID and sets its state to IDLE */ +/** Frees the invokeID and sets its state to IDLE + * + * @param invokeID Invoke-ID + */ void tsm_free_invoke_id(uint8_t invokeID) { uint8_t index; diff --git a/src/bacnet/timesync.c b/src/bacnet/timesync.c index 97045f83..0be92460 100644 --- a/src/bacnet/timesync.c +++ b/src/bacnet/timesync.c @@ -40,7 +40,16 @@ /** @file timesync.c Encode/Decode TimeSync APDUs */ #if BACNET_SVC_TS_A -/* encode service */ +/** Encode the time synchronisation service. + * + * @param apdu [in] Buffer in which the APDU contents are written + * @param service [in] Time service that shall be encoded, like + * SERVICE_UNCONFIRMED_UTC_TIME_SYNCHRONIZATION. + * @param my_date [in] Pointer to the date structure used to encode. + * @param my_time [in] Pointer to the time structure used to encode. + * + * @return Count of encoded bytes. + */ int timesync_encode_apdu_service(uint8_t *apdu, BACNET_UNCONFIRMED_SERVICE service, BACNET_DATE *my_date, @@ -62,6 +71,14 @@ int timesync_encode_apdu_service(uint8_t *apdu, return apdu_len; } +/** Encode the service 'UTC_TIME_SYNCHRONIZATION'. + * + * @param apdu [in] Buffer in which the APDU contents are written + * @param my_date [in] Pointer to the date structure used to encode. + * @param my_time [in] Pointer to the time structure used to encode. + * + * @return Count of encoded bytes. + */ int timesync_utc_encode_apdu( uint8_t *apdu, BACNET_DATE *my_date, BACNET_TIME *my_time) { @@ -69,6 +86,14 @@ int timesync_utc_encode_apdu( apdu, SERVICE_UNCONFIRMED_UTC_TIME_SYNCHRONIZATION, my_date, my_time); } +/** Encode the service 'TIME_SYNCHRONIZATION'. + * + * @param apdu [in] Buffer in which the APDU contents are written + * @param my_date [in] Pointer to the date structure used to encode. + * @param my_time [in] Pointer to the time structure used to encode. + * + * @return Count of encoded bytes. + */ int timesync_encode_apdu( uint8_t *apdu, BACNET_DATE *my_date, BACNET_TIME *my_time) { @@ -77,7 +102,15 @@ int timesync_encode_apdu( } #endif -/* decode the service request only */ +/** Decode the service request only. + * + * @param apdu [in] Buffer in which the APDU contents are read + * @param apdu_len [in] length of the APDU buffer. + * @param my_date [in] Pointer to the date structure filled in. + * @param my_time [in] Pointer to the time structure filled in. + * + * @return Count of decoded bytes. + */ int timesync_decode_service_request(uint8_t *apdu, unsigned apdu_len, BACNET_DATE *my_date, @@ -91,17 +124,25 @@ int timesync_decode_service_request(uint8_t *apdu, /* date */ len += decode_tag_number_and_value(&apdu[len], &tag_number, &len_value); if (tag_number == BACNET_APPLICATION_TAG_DATE) { - len += decode_date(&apdu[len], my_date); + if ((unsigned)(len + 4) <= apdu_len) { + len += decode_date(&apdu[len], my_date); + } else { + return -1; + } } else { return -1; } /* time */ len += decode_tag_number_and_value(&apdu[len], &tag_number, &len_value); if (tag_number == BACNET_APPLICATION_TAG_TIME) { + if ((unsigned)(len + 4) <= apdu_len) { len += decode_bacnet_time(&apdu[len], my_time); } else { return -1; } + } else { + return -1; + } } return len; @@ -139,6 +180,10 @@ int timesync_encode_timesync_recipients( BACNET_OCTET_STRING octet_string; BACNET_RECIPIENT_LIST *pRecipient; + if ((!apdu) || (max_apdu < 1) || (!recipient)) { + return(0); + } + pRecipient = recipient; while (pRecipient != NULL) { if (pRecipient->tag == 0) { @@ -231,11 +276,18 @@ int timesync_decode_timesync_recipients( BACNET_OCTET_STRING octet_string; BACNET_RECIPIENT_LIST *pRecipient; + if ((!apdu) || (max_apdu < 1) || (!recipient)) { + return BACNET_STATUS_ABORT; + } + pRecipient = recipient; while (pRecipient != NULL) { /* device [0] BACnetObjectIdentifier */ if (decode_is_context_tag(&apdu[apdu_len], 0)) { pRecipient->tag = 0; + if ((unsigned)(apdu_len + 4) > max_apdu) { + return BACNET_STATUS_ABORT; + } len = decode_context_object_id(&apdu[apdu_len], 0, &pRecipient->type.device.type, &pRecipient->type.device.instance); @@ -250,6 +302,9 @@ int timesync_decode_timesync_recipients( tag_len = decode_tag_number_and_value( &apdu[apdu_len], &tag_number, &len_value_type); apdu_len += tag_len; + if ((unsigned)apdu_len > max_apdu) { + return BACNET_STATUS_ABORT; + } if (tag_number != BACNET_APPLICATION_TAG_UNSIGNED_INT) { return BACNET_STATUS_ABORT; } @@ -257,6 +312,9 @@ int timesync_decode_timesync_recipients( &apdu[apdu_len], len_value_type, &unsigned_value); pRecipient->type.address.net = (uint16_t)unsigned_value; apdu_len += len; + if ((unsigned)apdu_len > max_apdu) { + return BACNET_STATUS_ABORT; + } /* mac-address OCTET STRING */ tag_len = decode_tag_number_and_value( &apdu[apdu_len], &tag_number, &len_value_type); @@ -265,7 +323,7 @@ int timesync_decode_timesync_recipients( return BACNET_STATUS_ABORT; } len = bacnet_octet_string_decode(&apdu[apdu_len], - max_apdu, len_value_type, &octet_string); + max_apdu - apdu_len, len_value_type, &octet_string); if (len < 0) { return BACNET_STATUS_ERROR; }