diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ce5bcf2..3cd6c7b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,13 @@ The git repositories are hosted at the following sites: * https://bacnet.sourceforge.net/ * https://github.com/bacnet-stack/bacnet-stack/ -## [Unreleased] - 2025-11-15 +## [Unreleased] - 2025-12-04 ### Security + +* Secured npdu_is_expected_reply() function where the MS/TP reply matcher + could have an out-of-bounds read. (#1178) + ### Added * Added Send_x_Address() API to ReadPropertyMultiple, WritePropertyMultiple, diff --git a/src/bacnet/npdu.c b/src/bacnet/npdu.c index 7c6eb263..6bfb0845 100644 --- a/src/bacnet/npdu.c +++ b/src/bacnet/npdu.c @@ -699,15 +699,28 @@ bool npdu_is_expected_reply( if (request.npdu_data.network_layer_message) { return false; } + if (request_pdu_len <= offset) { + return false; + } + /* confirmed service request? */ request.pdu_type = request_pdu[offset] & 0xF0; if (request.pdu_type != PDU_TYPE_CONFIRMED_SERVICE_REQUEST) { return false; } + if (request_pdu_len <= (offset + 2)) { + return false; + } request.invoke_id = request_pdu[offset + 2]; /* segmented request? */ if (request_pdu[offset] & BIT(3)) { + if (request_pdu_len <= (offset + 5)) { + return false; + } request.service_choice = request_pdu[offset + 5]; } else { + if (request_pdu_len <= (offset + 3)) { + return false; + } request.service_choice = request_pdu[offset + 3]; } if ((reply_pdu_len > 0) && (reply_pdu[0] != BACNET_PROTOCOL_VERSION)) { @@ -720,33 +733,52 @@ bool npdu_is_expected_reply( if (offset <= 0) { return false; } + /* reply is not a network layer message */ if (reply.npdu_data.network_layer_message) { return false; } /* reply could be a lot of things: - confirmed, simple ack, abort, reject, error */ + confirmed, simple ack, abort, reject, error */ + if (reply_pdu_len <= offset) { + return false; + } reply.pdu_type = reply_pdu[offset] & 0xF0; switch (reply.pdu_type) { case PDU_TYPE_SIMPLE_ACK: + if (reply_pdu_len <= (offset + 2)) { + return false; + } reply.invoke_id = reply_pdu[offset + 1]; reply.service_choice = reply_pdu[offset + 2]; break; case PDU_TYPE_COMPLEX_ACK: + if (reply_pdu_len <= (offset + 2)) { + return false; + } reply.invoke_id = reply_pdu[offset + 1]; /* segmented message? */ if (reply_pdu[offset] & BIT(3)) { + if (reply_pdu_len <= (offset + 4)) { + return false; + } reply.service_choice = reply_pdu[offset + 4]; } else { reply.service_choice = reply_pdu[offset + 2]; } break; case PDU_TYPE_ERROR: + if (reply_pdu_len <= (offset + 2)) { + 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: case PDU_TYPE_SEGMENT_ACK: + if (reply_pdu_len <= (offset + 1)) { + return false; + } reply.invoke_id = reply_pdu[offset + 1]; break; default: @@ -784,8 +816,9 @@ bool npdu_is_expected_reply( * @param reply_pdu_len - number of bytes of PDU data * @param reply_mac - MS/TP MAC address of the sender of the reply * @return true if the reply PDU is the data expected by the request PDU - * @note This function is used by the DLMSTP datalink layer to match confirmed - * service requests with their replies in the ANSWER_DATA_REQUEST state. + * @note This function is used by the DLMSTP datalink layer to match + * confirmed service requests with their replies in the ANSWER_DATA_REQUEST + * state. */ bool npdu_is_data_expecting_reply( const uint8_t *request_pdu, diff --git a/test/bacnet/npdu/src/main.c b/test/bacnet/npdu/src/main.c index a6aa3fcd..43cc4a57 100644 --- a/test/bacnet/npdu/src/main.c +++ b/test/bacnet/npdu/src/main.c @@ -343,6 +343,40 @@ static void test_NPDU_Segmented_Complex_Ack_Reply(void) zassert_true(status, NULL); } +static void test_npdu_is_expected_reply_too_short( + const uint8_t *request_pdu, + uint16_t request_pdu_len, + BACNET_ADDRESS *request_address, + uint16_t request_minimum_len, + const uint8_t *reply_pdu, + uint16_t reply_pdu_len, + BACNET_ADDRESS *reply_address, + uint16_t reply_minimum_len) +{ + int test_len; + bool status; + + /* shrink the buffers to test for buffer out-of-bounds read */ + /* smallest valid request */ + test_len = request_minimum_len; + while (test_len) { + test_len--; + status = npdu_is_expected_reply( + request_pdu, test_len, request_address, reply_pdu, reply_pdu_len, + reply_address); + zassert_false(status, "test_len=%d\n", test_len); + } + /* smallest valid reply */ + test_len = reply_minimum_len; + while (test_len) { + test_len--; + status = npdu_is_expected_reply( + request_pdu, request_pdu_len, request_address, reply_pdu, test_len, + reply_address); + zassert_false(status, "test_len=%d\n", test_len); + } +} + #if defined(CONFIG_ZTEST_NEW_API) ZTEST(npdu_tests, test_NPDU_Data_Expecting_Reply) #else @@ -355,7 +389,8 @@ static void test_NPDU_Data_Expecting_Reply(void) uint8_t apdu[MAX_APDU] = { 0 }; uint8_t request_pdu[MAX_NPDU + MAX_APDU] = { 0 }; uint8_t reply_pdu[MAX_NPDU + MAX_APDU] = { 0 }; - int request_pdu_len = 0, reply_pdu_len = 0, npdu_len = 0, apdu_len = 0; + int request_pdu_len = 0, reply_pdu_len = 0, npdu_len = 0, apdu_len = 0, + request_npdu_len = 0; uint8_t invoke_id = 1; bool status; @@ -376,6 +411,7 @@ static void test_NPDU_Data_Expecting_Reply(void) apdu_len = rp_encode_apdu(&request_pdu[npdu_len], invoke_id, &rpdata); zassert_true(apdu_len > 0, NULL); request_pdu_len = npdu_len + apdu_len; + request_npdu_len = npdu_len; /* reply */ npdu_encode_npdu_data(&npdu_data, false, MESSAGE_PRIORITY_NORMAL); npdu_len = npdu_encode_pdu( @@ -395,6 +431,9 @@ static void test_NPDU_Data_Expecting_Reply(void) request_pdu, request_pdu_len, &test_address, reply_pdu, reply_pdu_len, &test_address); zassert_true(status, NULL); + test_npdu_is_expected_reply_too_short( + request_pdu, request_pdu_len, &test_address, request_npdu_len + 4, + reply_pdu, reply_pdu_len, &test_address, npdu_len + 3); /* using the MAC version of the function */ status = npdu_is_data_expecting_reply( request_pdu, request_pdu_len, test_address.mac[0], reply_pdu, @@ -458,6 +497,9 @@ static void test_NPDU_Data_Expecting_Reply(void) request_pdu, request_pdu_len, &test_address, reply_pdu, reply_pdu_len, &test_address); zassert_true(status, NULL); + test_npdu_is_expected_reply_too_short( + request_pdu, request_pdu_len, &test_address, request_npdu_len + 4, + reply_pdu, reply_pdu_len, &test_address, npdu_len + 3); /* reply with REJECT PDU */ apdu_len = reject_encode_apdu( &reply_pdu[npdu_len], invoke_id, REJECT_REASON_OTHER); @@ -467,6 +509,9 @@ static void test_NPDU_Data_Expecting_Reply(void) request_pdu, request_pdu_len, &test_address, reply_pdu, reply_pdu_len, &test_address); zassert_true(status, NULL); + test_npdu_is_expected_reply_too_short( + request_pdu, request_pdu_len, &test_address, request_npdu_len + 4, + reply_pdu, reply_pdu_len, &test_address, npdu_len + 2); /* reply with ABORT PDU */ apdu_len = abort_encode_apdu( &reply_pdu[npdu_len], invoke_id, ABORT_REASON_OTHER, true); @@ -476,6 +521,9 @@ static void test_NPDU_Data_Expecting_Reply(void) request_pdu, request_pdu_len, &test_address, reply_pdu, reply_pdu_len, &test_address); zassert_true(status, NULL); + test_npdu_is_expected_reply_too_short( + request_pdu, request_pdu_len, &test_address, request_npdu_len + 4, + reply_pdu, reply_pdu_len, &test_address, npdu_len + 2); /* reply with simple ack - note this is totally fake! */ apdu_len = encode_simple_ack( &reply_pdu[npdu_len], invoke_id, SERVICE_CONFIRMED_READ_PROPERTY); @@ -484,6 +532,10 @@ static void test_NPDU_Data_Expecting_Reply(void) status = npdu_is_expected_reply( request_pdu, request_pdu_len, &test_address, reply_pdu, reply_pdu_len, &test_address); + zassert_true(status, NULL); + test_npdu_is_expected_reply_too_short( + request_pdu, request_pdu_len, &test_address, request_npdu_len + 4, + reply_pdu, reply_pdu_len, &test_address, npdu_len + 3); } #if defined(CONFIG_ZTEST_NEW_API)