From d8a971e04d52be16bf405b51d934a30b8aa3f2c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Raimbault?= Date: Sun, 20 Oct 2024 18:02:22 +0200 Subject: [PATCH] Check length passed to modbus_reply (write_register) Related to df79a02feb253c. CVE: CVE-2024-10918 Upstream-Status: Backport [https://github.com/stephane/libmodbus/commit/d8a971e04d52be16bf405b51d934a30b8aa3f2c3] Signed-off-by: Peter Marko --- src/modbus.c | 38 ++++++++++++++++++++++++++------------ tests/unit-test-client.c | 6 +++++- tests/unit-test-server.c | 13 +++++++++++-- 3 files changed, 42 insertions(+), 15 deletions(-) diff --git a/src/modbus.c b/src/modbus.c index 33086ec..fe192a7 100644 --- a/src/modbus.c +++ b/src/modbus.c @@ -904,13 +904,14 @@ int modbus_reply(modbus_t *ctx, 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); + rsp_length = + response_exception(ctx, + &sft, + MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, + rsp, + FALSE, + "Invalid request length used in modbus_reply (%d)\n", + req_length); break; } @@ -942,13 +943,26 @@ int modbus_reply(modbus_t *ctx, FALSE, "Illegal data address 0x%0X in write_register\n", address); - } else { - int data = (req[offset + 3] << 8) + req[offset + 4]; + break; + } - mb_mapping->tab_registers[mapping_address] = data; - memcpy(rsp, req, req_length); - rsp_length = req_length; + 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 used in modbus_reply (%d)\n", + req_length); + break; } + int data = (req[offset + 3] << 8) + req[offset + 4]; + + mb_mapping->tab_registers[mapping_address] = data; + memcpy(rsp, req, rsp_length); } break; case MODBUS_FC_WRITE_MULTIPLE_COILS: { int nb = (req[offset + 3] << 8) + req[offset + 4]; diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index 50859f8..84d5840 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -498,13 +498,17 @@ int main(int argc, char *argv[]) modbus_disable_quirks(ctx, MODBUS_QUIRK_MAX_SLAVE); rc = modbus_set_slave(ctx, old_slave); - ASSERT_TRUE(rc == 0, "Uanble to restore slave value") + ASSERT_TRUE(rc == 0, "Unable 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, ""); + rc = modbus_write_register(ctx, UT_REGISTERS_ADDRESS_SPECIAL, 0x42); + printf("* modbus_write_register (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 8e28124..fc7ceb3 100644 --- a/tests/unit-test-server.c +++ b/tests/unit-test-server.c @@ -210,8 +210,17 @@ int main(int argc, char *argv[]) // 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", + printf( + "Special modbus_write_bit detected. Inject a wrong length value (%d) " + "in modbus_reply\n", + rc); + } + } else if (query[header_length] == MODBUS_FC_WRITE_SINGLE_REGISTER) { + if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 1) == + UT_REGISTERS_ADDRESS_SPECIAL) { + rc = 45; + printf("Special modbus_write_register detected. Inject a wrong length " + "value (%d) in modbus_reply\n", rc); } }