From df79a02feb253c0a9a009bcdbb21e47581315111 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Raimbault?= Date: Fri, 18 Oct 2024 10:47:14 +0200 Subject: [PATCH] Check length passed to modbus_reply (write_bit) The modbus_reply function is designed to receive arguments from modbus_receive. This patch avoid a wrong use of memcpy if the user chooses to inject a bad length argument. Thank you Nozomi Networks Labs Advisory for the report. CVE: CVE-2024-10918 Upstream-Status: Backport [https://github.com/stephane/libmodbus/commit/df79a02feb253c0a9a009bcdbb21e47581315111] Signed-off-by: Peter Marko --- src/modbus.c | 54 +++++++++++++++++++++++++--------------- tests/unit-test-client.c | 9 +++++-- tests/unit-test-server.c | 16 +++++++++--- tests/unit-test.h.in | 1 + 4 files changed, 55 insertions(+), 25 deletions(-) diff --git a/src/modbus.c b/src/modbus.c index 5a9b867..33086ec 100644 --- a/src/modbus.c +++ b/src/modbus.c @@ -897,24 +897,37 @@ int modbus_reply(modbus_t *ctx, FALSE, "Illegal data address 0x%0X in write_bit\n", address); + break; + } + + /* This check is only done here to ensure using memcpy is safe. */ + rsp_length = compute_response_length_from_request(ctx, (uint8_t *) req); + if (rsp_length != req_length) { + /* Bad use of modbus_reply */ + rsp_length = response_exception(ctx, + &sft, + MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, + rsp, + FALSE, + "Invalid request length (%d)\n", + req_length); + break; + } + + int data = (req[offset + 3] << 8) + req[offset + 4]; + if (data == 0xFF00 || data == 0x0) { + mb_mapping->tab_bits[mapping_address] = data ? ON : OFF; + memcpy(rsp, req, rsp_length); } else { - int data = (req[offset + 3] << 8) + req[offset + 4]; - - if (data == 0xFF00 || data == 0x0) { - mb_mapping->tab_bits[mapping_address] = data ? ON : OFF; - memcpy(rsp, req, req_length); - rsp_length = req_length; - } else { - rsp_length = response_exception( - ctx, - &sft, - MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, - rsp, - FALSE, - "Illegal data value 0x%0X in write_bit request at address %0X\n", - data, - address); - } + rsp_length = response_exception( + ctx, + &sft, + MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, + rsp, + FALSE, + "Illegal data value 0x%0X in write_bit request at address %0X\n", + data, + address); } } break; case MODBUS_FC_WRITE_SINGLE_REGISTER: { @@ -1127,8 +1140,8 @@ int modbus_reply(modbus_t *ctx, break; } - /* Suppress any responses in RTU when the request was a broadcast, excepted when quirk - * is enabled. */ + /* Suppress any responses in RTU when the request was a broadcast, excepted when + * quirk is enabled. */ if (ctx->backend->backend_type == _MODBUS_BACKEND_TYPE_RTU && slave == MODBUS_BROADCAST_ADDRESS && !(ctx->quirks & MODBUS_QUIRK_REPLY_TO_BROADCAST)) { @@ -1821,7 +1834,8 @@ int modbus_set_byte_timeout(modbus_t *ctx, uint32_t to_sec, uint32_t to_usec) return 0; } -/* Get the timeout interval used by the server to wait for an indication from a client */ +/* Get the timeout interval used by the server to wait for an indication from a client + */ int modbus_get_indication_timeout(modbus_t *ctx, uint32_t *to_sec, uint32_t *to_usec) { if (ctx == NULL) { diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index a441766..50859f8 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -400,11 +400,11 @@ int main(int argc, char *argv[]) ASSERT_TRUE(rc == -1 && errno == EMBXILADD, ""); rc = modbus_write_bits(ctx, 0, 1, tab_rp_bits); - printf("* modbus_write_coils (0): "); + printf("* modbus_write_bits (0): "); ASSERT_TRUE(rc == -1 && errno == EMBXILADD, ""); rc = modbus_write_bits(ctx, UT_BITS_ADDRESS + UT_BITS_NB, UT_BITS_NB, tab_rp_bits); - printf("* modbus_write_coils (max): "); + printf("* modbus_write_bits (max): "); ASSERT_TRUE(rc == -1 && errno == EMBXILADD, ""); rc = modbus_write_register(ctx, 0, tab_rp_registers[0]); @@ -500,6 +500,11 @@ int main(int argc, char *argv[]) rc = modbus_set_slave(ctx, old_slave); ASSERT_TRUE(rc == 0, "Uanble to restore slave value") + /** BAD USE OF REPLY FUNCTION **/ + rc = modbus_write_bit(ctx, UT_BITS_ADDRESS_INVALID_REQUEST_LENGTH, ON); + printf("* modbus_write_bit (triggers invalid reply): "); + ASSERT_TRUE(rc == -1 && errno == EMBXILVAL, ""); + /** SLAVE REPLY **/ printf("\nTEST SLAVE REPLY:\n"); diff --git a/tests/unit-test-server.c b/tests/unit-test-server.c index 561d64d..8e28124 100644 --- a/tests/unit-test-server.c +++ b/tests/unit-test-server.c @@ -150,9 +150,8 @@ int main(int argc, char *argv[]) break; } - /* Special server behavior to test client */ - if (query[header_length] == 0x03) { - /* Read holding registers */ + /** Special server behavior to test client **/ + if (query[header_length] == MODBUS_FC_READ_HOLDING_REGISTERS) { if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 3) == UT_REGISTERS_NB_SPECIAL) { @@ -204,6 +203,17 @@ int main(int argc, char *argv[]) } continue; } + + } else if (query[header_length] == MODBUS_FC_WRITE_SINGLE_COIL) { + if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 1) == + UT_BITS_ADDRESS_INVALID_REQUEST_LENGTH) { + // The valid length is lengths of header + checkum + FC + address + value + // (max 12) + rc = 34; + printf("Special modbus_write_bit detected. Inject a wrong rc value (%d) " + "in modbus_reply\n", + rc); + } } rc = modbus_reply(ctx, query, rc, mb_mapping); diff --git a/tests/unit-test.h.in b/tests/unit-test.h.in index 5e379bb..21d09e3 100644 --- a/tests/unit-test.h.in +++ b/tests/unit-test.h.in @@ -30,6 +30,7 @@ const uint16_t UT_BITS_ADDRESS = 0x130; const uint16_t UT_BITS_NB = 0x25; const uint8_t UT_BITS_TAB[] = { 0xCD, 0x6B, 0xB2, 0x0E, 0x1B }; +const uint16_t UT_BITS_ADDRESS_INVALID_REQUEST_LENGTH = UT_BITS_ADDRESS + 2; const uint16_t UT_INPUT_BITS_ADDRESS = 0x1C4; const uint16_t UT_INPUT_BITS_NB = 0x16;