diff options
author | Peter Marko <peter.marko@siemens.com> | 2025-03-06 19:35:51 +0100 |
---|---|---|
committer | Armin Kuster <akuster808@gmail.com> | 2025-03-07 19:40:47 -0500 |
commit | 73e6789fdfdd31a8b512aedb59dfe9cfd5b40342 (patch) | |
tree | f0088c38b72ce61f34e0a5251635900d287f0c65 | |
parent | 454cc113175e42d803fe09d5a6c495c369b5a68a (diff) | |
download | meta-openembedded-73e6789fdfdd31a8b512aedb59dfe9cfd5b40342.tar.gz |
libmodbus: patch CVE-2024-10918
Pick commit mentioning the bug and two follow-up commits mentioning the
first commit as well as commit to adapt tests for these.
Tested by running the test-suite.
Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Armin Kuster <akuster808@gmail.com>
5 files changed, 628 insertions, 1 deletions
diff --git a/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-01.patch b/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-01.patch new file mode 100644 index 0000000000..f50bee68e8 --- /dev/null +++ b/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-01.patch | |||
@@ -0,0 +1,177 @@ | |||
1 | From df79a02feb253c0a9a009bcdbb21e47581315111 Mon Sep 17 00:00:00 2001 | ||
2 | From: =?UTF-8?q?St=C3=A9phane=20Raimbault?= <stephane.raimbault@gmail.com> | ||
3 | Date: Fri, 18 Oct 2024 10:47:14 +0200 | ||
4 | Subject: [PATCH] Check length passed to modbus_reply (write_bit) | ||
5 | |||
6 | The modbus_reply function is designed to receive arguments | ||
7 | from modbus_receive. This patch avoid a wrong use of memcpy if | ||
8 | the user chooses to inject a bad length argument. | ||
9 | |||
10 | Thank you Nozomi Networks Labs Advisory for the report. | ||
11 | |||
12 | CVE: CVE-2024-10918 | ||
13 | Upstream-Status: Backport [https://github.com/stephane/libmodbus/commit/df79a02feb253c0a9a009bcdbb21e47581315111] | ||
14 | Signed-off-by: Peter Marko <peter.marko@siemens.com> | ||
15 | --- | ||
16 | src/modbus.c | 54 +++++++++++++++++++++++++--------------- | ||
17 | tests/unit-test-client.c | 9 +++++-- | ||
18 | tests/unit-test-server.c | 16 +++++++++--- | ||
19 | tests/unit-test.h.in | 1 + | ||
20 | 4 files changed, 55 insertions(+), 25 deletions(-) | ||
21 | |||
22 | diff --git a/src/modbus.c b/src/modbus.c | ||
23 | index 5a9b867..33086ec 100644 | ||
24 | --- a/src/modbus.c | ||
25 | +++ b/src/modbus.c | ||
26 | @@ -897,24 +897,37 @@ int modbus_reply(modbus_t *ctx, | ||
27 | FALSE, | ||
28 | "Illegal data address 0x%0X in write_bit\n", | ||
29 | address); | ||
30 | + break; | ||
31 | + } | ||
32 | + | ||
33 | + /* This check is only done here to ensure using memcpy is safe. */ | ||
34 | + rsp_length = compute_response_length_from_request(ctx, (uint8_t *) req); | ||
35 | + if (rsp_length != req_length) { | ||
36 | + /* Bad use of modbus_reply */ | ||
37 | + rsp_length = response_exception(ctx, | ||
38 | + &sft, | ||
39 | + MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
40 | + rsp, | ||
41 | + FALSE, | ||
42 | + "Invalid request length (%d)\n", | ||
43 | + req_length); | ||
44 | + break; | ||
45 | + } | ||
46 | + | ||
47 | + int data = (req[offset + 3] << 8) + req[offset + 4]; | ||
48 | + if (data == 0xFF00 || data == 0x0) { | ||
49 | + mb_mapping->tab_bits[mapping_address] = data ? ON : OFF; | ||
50 | + memcpy(rsp, req, rsp_length); | ||
51 | } else { | ||
52 | - int data = (req[offset + 3] << 8) + req[offset + 4]; | ||
53 | - | ||
54 | - if (data == 0xFF00 || data == 0x0) { | ||
55 | - mb_mapping->tab_bits[mapping_address] = data ? ON : OFF; | ||
56 | - memcpy(rsp, req, req_length); | ||
57 | - rsp_length = req_length; | ||
58 | - } else { | ||
59 | - rsp_length = response_exception( | ||
60 | - ctx, | ||
61 | - &sft, | ||
62 | - MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
63 | - rsp, | ||
64 | - FALSE, | ||
65 | - "Illegal data value 0x%0X in write_bit request at address %0X\n", | ||
66 | - data, | ||
67 | - address); | ||
68 | - } | ||
69 | + rsp_length = response_exception( | ||
70 | + ctx, | ||
71 | + &sft, | ||
72 | + MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
73 | + rsp, | ||
74 | + FALSE, | ||
75 | + "Illegal data value 0x%0X in write_bit request at address %0X\n", | ||
76 | + data, | ||
77 | + address); | ||
78 | } | ||
79 | } break; | ||
80 | case MODBUS_FC_WRITE_SINGLE_REGISTER: { | ||
81 | @@ -1127,8 +1140,8 @@ int modbus_reply(modbus_t *ctx, | ||
82 | break; | ||
83 | } | ||
84 | |||
85 | - /* Suppress any responses in RTU when the request was a broadcast, excepted when quirk | ||
86 | - * is enabled. */ | ||
87 | + /* Suppress any responses in RTU when the request was a broadcast, excepted when | ||
88 | + * quirk is enabled. */ | ||
89 | if (ctx->backend->backend_type == _MODBUS_BACKEND_TYPE_RTU && | ||
90 | slave == MODBUS_BROADCAST_ADDRESS && | ||
91 | !(ctx->quirks & MODBUS_QUIRK_REPLY_TO_BROADCAST)) { | ||
92 | @@ -1821,7 +1834,8 @@ int modbus_set_byte_timeout(modbus_t *ctx, uint32_t to_sec, uint32_t to_usec) | ||
93 | return 0; | ||
94 | } | ||
95 | |||
96 | -/* Get the timeout interval used by the server to wait for an indication from a client */ | ||
97 | +/* Get the timeout interval used by the server to wait for an indication from a client | ||
98 | + */ | ||
99 | int modbus_get_indication_timeout(modbus_t *ctx, uint32_t *to_sec, uint32_t *to_usec) | ||
100 | { | ||
101 | if (ctx == NULL) { | ||
102 | diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c | ||
103 | index a441766..50859f8 100644 | ||
104 | --- a/tests/unit-test-client.c | ||
105 | +++ b/tests/unit-test-client.c | ||
106 | @@ -400,11 +400,11 @@ int main(int argc, char *argv[]) | ||
107 | ASSERT_TRUE(rc == -1 && errno == EMBXILADD, ""); | ||
108 | |||
109 | rc = modbus_write_bits(ctx, 0, 1, tab_rp_bits); | ||
110 | - printf("* modbus_write_coils (0): "); | ||
111 | + printf("* modbus_write_bits (0): "); | ||
112 | ASSERT_TRUE(rc == -1 && errno == EMBXILADD, ""); | ||
113 | |||
114 | rc = modbus_write_bits(ctx, UT_BITS_ADDRESS + UT_BITS_NB, UT_BITS_NB, tab_rp_bits); | ||
115 | - printf("* modbus_write_coils (max): "); | ||
116 | + printf("* modbus_write_bits (max): "); | ||
117 | ASSERT_TRUE(rc == -1 && errno == EMBXILADD, ""); | ||
118 | |||
119 | rc = modbus_write_register(ctx, 0, tab_rp_registers[0]); | ||
120 | @@ -500,6 +500,11 @@ int main(int argc, char *argv[]) | ||
121 | rc = modbus_set_slave(ctx, old_slave); | ||
122 | ASSERT_TRUE(rc == 0, "Uanble to restore slave value") | ||
123 | |||
124 | + /** BAD USE OF REPLY FUNCTION **/ | ||
125 | + rc = modbus_write_bit(ctx, UT_BITS_ADDRESS_INVALID_REQUEST_LENGTH, ON); | ||
126 | + printf("* modbus_write_bit (triggers invalid reply): "); | ||
127 | + ASSERT_TRUE(rc == -1 && errno == EMBXILVAL, ""); | ||
128 | + | ||
129 | /** SLAVE REPLY **/ | ||
130 | |||
131 | printf("\nTEST SLAVE REPLY:\n"); | ||
132 | diff --git a/tests/unit-test-server.c b/tests/unit-test-server.c | ||
133 | index 561d64d..8e28124 100644 | ||
134 | --- a/tests/unit-test-server.c | ||
135 | +++ b/tests/unit-test-server.c | ||
136 | @@ -150,9 +150,8 @@ int main(int argc, char *argv[]) | ||
137 | break; | ||
138 | } | ||
139 | |||
140 | - /* Special server behavior to test client */ | ||
141 | - if (query[header_length] == 0x03) { | ||
142 | - /* Read holding registers */ | ||
143 | + /** Special server behavior to test client **/ | ||
144 | + if (query[header_length] == MODBUS_FC_READ_HOLDING_REGISTERS) { | ||
145 | |||
146 | if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 3) == | ||
147 | UT_REGISTERS_NB_SPECIAL) { | ||
148 | @@ -204,6 +203,17 @@ int main(int argc, char *argv[]) | ||
149 | } | ||
150 | continue; | ||
151 | } | ||
152 | + | ||
153 | + } else if (query[header_length] == MODBUS_FC_WRITE_SINGLE_COIL) { | ||
154 | + if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 1) == | ||
155 | + UT_BITS_ADDRESS_INVALID_REQUEST_LENGTH) { | ||
156 | + // The valid length is lengths of header + checkum + FC + address + value | ||
157 | + // (max 12) | ||
158 | + rc = 34; | ||
159 | + printf("Special modbus_write_bit detected. Inject a wrong rc value (%d) " | ||
160 | + "in modbus_reply\n", | ||
161 | + rc); | ||
162 | + } | ||
163 | } | ||
164 | |||
165 | rc = modbus_reply(ctx, query, rc, mb_mapping); | ||
166 | diff --git a/tests/unit-test.h.in b/tests/unit-test.h.in | ||
167 | index 5e379bb..21d09e3 100644 | ||
168 | --- a/tests/unit-test.h.in | ||
169 | +++ b/tests/unit-test.h.in | ||
170 | @@ -30,6 +30,7 @@ | ||
171 | const uint16_t UT_BITS_ADDRESS = 0x130; | ||
172 | const uint16_t UT_BITS_NB = 0x25; | ||
173 | const uint8_t UT_BITS_TAB[] = { 0xCD, 0x6B, 0xB2, 0x0E, 0x1B }; | ||
174 | +const uint16_t UT_BITS_ADDRESS_INVALID_REQUEST_LENGTH = UT_BITS_ADDRESS + 2; | ||
175 | |||
176 | const uint16_t UT_INPUT_BITS_ADDRESS = 0x1C4; | ||
177 | const uint16_t UT_INPUT_BITS_NB = 0x16; | ||
diff --git a/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-02.patch b/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-02.patch new file mode 100644 index 0000000000..16b9ba72c5 --- /dev/null +++ b/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-02.patch | |||
@@ -0,0 +1,121 @@ | |||
1 | From d8a971e04d52be16bf405b51d934a30b8aa3f2c3 Mon Sep 17 00:00:00 2001 | ||
2 | From: =?UTF-8?q?St=C3=A9phane=20Raimbault?= <stephane.raimbault@gmail.com> | ||
3 | Date: Sun, 20 Oct 2024 18:02:22 +0200 | ||
4 | Subject: [PATCH] Check length passed to modbus_reply (write_register) | ||
5 | |||
6 | Related to df79a02feb253c. | ||
7 | |||
8 | CVE: CVE-2024-10918 | ||
9 | Upstream-Status: Backport [https://github.com/stephane/libmodbus/commit/d8a971e04d52be16bf405b51d934a30b8aa3f2c3] | ||
10 | Signed-off-by: Peter Marko <peter.marko@siemens.com> | ||
11 | --- | ||
12 | src/modbus.c | 38 ++++++++++++++++++++++++++------------ | ||
13 | tests/unit-test-client.c | 6 +++++- | ||
14 | tests/unit-test-server.c | 13 +++++++++++-- | ||
15 | 3 files changed, 42 insertions(+), 15 deletions(-) | ||
16 | |||
17 | diff --git a/src/modbus.c b/src/modbus.c | ||
18 | index 33086ec..fe192a7 100644 | ||
19 | --- a/src/modbus.c | ||
20 | +++ b/src/modbus.c | ||
21 | @@ -904,13 +904,14 @@ int modbus_reply(modbus_t *ctx, | ||
22 | rsp_length = compute_response_length_from_request(ctx, (uint8_t *) req); | ||
23 | if (rsp_length != req_length) { | ||
24 | /* Bad use of modbus_reply */ | ||
25 | - rsp_length = response_exception(ctx, | ||
26 | - &sft, | ||
27 | - MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
28 | - rsp, | ||
29 | - FALSE, | ||
30 | - "Invalid request length (%d)\n", | ||
31 | - req_length); | ||
32 | + rsp_length = | ||
33 | + response_exception(ctx, | ||
34 | + &sft, | ||
35 | + MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
36 | + rsp, | ||
37 | + FALSE, | ||
38 | + "Invalid request length used in modbus_reply (%d)\n", | ||
39 | + req_length); | ||
40 | break; | ||
41 | } | ||
42 | |||
43 | @@ -942,13 +943,26 @@ int modbus_reply(modbus_t *ctx, | ||
44 | FALSE, | ||
45 | "Illegal data address 0x%0X in write_register\n", | ||
46 | address); | ||
47 | - } else { | ||
48 | - int data = (req[offset + 3] << 8) + req[offset + 4]; | ||
49 | + break; | ||
50 | + } | ||
51 | |||
52 | - mb_mapping->tab_registers[mapping_address] = data; | ||
53 | - memcpy(rsp, req, req_length); | ||
54 | - rsp_length = req_length; | ||
55 | + rsp_length = compute_response_length_from_request(ctx, (uint8_t *) req); | ||
56 | + if (rsp_length != req_length) { | ||
57 | + /* Bad use of modbus_reply */ | ||
58 | + rsp_length = | ||
59 | + response_exception(ctx, | ||
60 | + &sft, | ||
61 | + MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
62 | + rsp, | ||
63 | + FALSE, | ||
64 | + "Invalid request length used in modbus_reply (%d)\n", | ||
65 | + req_length); | ||
66 | + break; | ||
67 | } | ||
68 | + int data = (req[offset + 3] << 8) + req[offset + 4]; | ||
69 | + | ||
70 | + mb_mapping->tab_registers[mapping_address] = data; | ||
71 | + memcpy(rsp, req, rsp_length); | ||
72 | } break; | ||
73 | case MODBUS_FC_WRITE_MULTIPLE_COILS: { | ||
74 | int nb = (req[offset + 3] << 8) + req[offset + 4]; | ||
75 | diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c | ||
76 | index 50859f8..84d5840 100644 | ||
77 | --- a/tests/unit-test-client.c | ||
78 | +++ b/tests/unit-test-client.c | ||
79 | @@ -498,13 +498,17 @@ int main(int argc, char *argv[]) | ||
80 | |||
81 | modbus_disable_quirks(ctx, MODBUS_QUIRK_MAX_SLAVE); | ||
82 | rc = modbus_set_slave(ctx, old_slave); | ||
83 | - ASSERT_TRUE(rc == 0, "Uanble to restore slave value") | ||
84 | + ASSERT_TRUE(rc == 0, "Unable to restore slave value") | ||
85 | |||
86 | /** BAD USE OF REPLY FUNCTION **/ | ||
87 | rc = modbus_write_bit(ctx, UT_BITS_ADDRESS_INVALID_REQUEST_LENGTH, ON); | ||
88 | printf("* modbus_write_bit (triggers invalid reply): "); | ||
89 | ASSERT_TRUE(rc == -1 && errno == EMBXILVAL, ""); | ||
90 | |||
91 | + rc = modbus_write_register(ctx, UT_REGISTERS_ADDRESS_SPECIAL, 0x42); | ||
92 | + printf("* modbus_write_register (triggers invalid reply): "); | ||
93 | + ASSERT_TRUE(rc == -1 && errno == EMBXILVAL, ""); | ||
94 | + | ||
95 | /** SLAVE REPLY **/ | ||
96 | |||
97 | printf("\nTEST SLAVE REPLY:\n"); | ||
98 | diff --git a/tests/unit-test-server.c b/tests/unit-test-server.c | ||
99 | index 8e28124..fc7ceb3 100644 | ||
100 | --- a/tests/unit-test-server.c | ||
101 | +++ b/tests/unit-test-server.c | ||
102 | @@ -210,8 +210,17 @@ int main(int argc, char *argv[]) | ||
103 | // The valid length is lengths of header + checkum + FC + address + value | ||
104 | // (max 12) | ||
105 | rc = 34; | ||
106 | - printf("Special modbus_write_bit detected. Inject a wrong rc value (%d) " | ||
107 | - "in modbus_reply\n", | ||
108 | + printf( | ||
109 | + "Special modbus_write_bit detected. Inject a wrong length value (%d) " | ||
110 | + "in modbus_reply\n", | ||
111 | + rc); | ||
112 | + } | ||
113 | + } else if (query[header_length] == MODBUS_FC_WRITE_SINGLE_REGISTER) { | ||
114 | + if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 1) == | ||
115 | + UT_REGISTERS_ADDRESS_SPECIAL) { | ||
116 | + rc = 45; | ||
117 | + printf("Special modbus_write_register detected. Inject a wrong length " | ||
118 | + "value (%d) in modbus_reply\n", | ||
119 | rc); | ||
120 | } | ||
121 | } | ||
diff --git a/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-03.patch b/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-03.patch new file mode 100644 index 0000000000..6ae9305b33 --- /dev/null +++ b/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-03.patch | |||
@@ -0,0 +1,84 @@ | |||
1 | From 7ea85f1b41f7066849f4bde7c0a3549b1e087bbf Mon Sep 17 00:00:00 2001 | ||
2 | From: =?UTF-8?q?St=C3=A9phane=20Raimbault?= <stephane.raimbault@gmail.com> | ||
3 | Date: Sun, 20 Oct 2024 18:23:26 +0200 | ||
4 | Subject: [PATCH] Small cleanups of unit test server | ||
5 | |||
6 | CVE: CVE-2024-10918 | ||
7 | Upstream-Status: Backport [https://github.com/stephane/libmodbus/commit/7ea85f1b41f7066849f4bde7c0a3549b1e087bbf] | ||
8 | Signed-off-by: Peter Marko <peter.marko@siemens.com> | ||
9 | --- | ||
10 | tests/unit-test-server.c | 29 ++++++++++++----------------- | ||
11 | 1 file changed, 12 insertions(+), 17 deletions(-) | ||
12 | |||
13 | diff --git a/tests/unit-test-server.c b/tests/unit-test-server.c | ||
14 | index fc7ceb3..bb25ba4 100644 | ||
15 | --- a/tests/unit-test-server.c | ||
16 | +++ b/tests/unit-test-server.c | ||
17 | @@ -150,21 +150,21 @@ int main(int argc, char *argv[]) | ||
18 | break; | ||
19 | } | ||
20 | |||
21 | + uint8_t function = query[header_length]; | ||
22 | + uint16_t address = MODBUS_GET_INT16_FROM_INT8(query, header_length + 1); | ||
23 | + | ||
24 | /** Special server behavior to test client **/ | ||
25 | - if (query[header_length] == MODBUS_FC_READ_HOLDING_REGISTERS) { | ||
26 | - | ||
27 | + if (function == MODBUS_FC_READ_HOLDING_REGISTERS) { | ||
28 | if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 3) == | ||
29 | UT_REGISTERS_NB_SPECIAL) { | ||
30 | printf("Set an incorrect number of values\n"); | ||
31 | MODBUS_SET_INT16_TO_INT8( | ||
32 | query, header_length + 3, UT_REGISTERS_NB_SPECIAL - 1); | ||
33 | - } else if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 1) == | ||
34 | - UT_REGISTERS_ADDRESS_SPECIAL) { | ||
35 | + } else if (address == UT_REGISTERS_ADDRESS_SPECIAL) { | ||
36 | printf("Reply to this special register address by an exception\n"); | ||
37 | modbus_reply_exception(ctx, query, MODBUS_EXCEPTION_SLAVE_OR_SERVER_BUSY); | ||
38 | continue; | ||
39 | - } else if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 1) == | ||
40 | - UT_REGISTERS_ADDRESS_INVALID_TID_OR_SLAVE) { | ||
41 | + } else if (address == UT_REGISTERS_ADDRESS_INVALID_TID_OR_SLAVE) { | ||
42 | const int RAW_REQ_LENGTH = 5; | ||
43 | uint8_t raw_req[] = {(use_backend == RTU) ? INVALID_SERVER_ID : 0xFF, | ||
44 | 0x03, | ||
45 | @@ -175,12 +175,10 @@ int main(int argc, char *argv[]) | ||
46 | printf("Reply with an invalid TID or slave\n"); | ||
47 | modbus_send_raw_request(ctx, raw_req, RAW_REQ_LENGTH * sizeof(uint8_t)); | ||
48 | continue; | ||
49 | - } else if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 1) == | ||
50 | - UT_REGISTERS_ADDRESS_SLEEP_500_MS) { | ||
51 | + } else if (address == UT_REGISTERS_ADDRESS_SLEEP_500_MS) { | ||
52 | printf("Sleep 0.5 s before replying\n"); | ||
53 | usleep(500000); | ||
54 | - } else if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 1) == | ||
55 | - UT_REGISTERS_ADDRESS_BYTE_SLEEP_5_MS) { | ||
56 | + } else if (address == UT_REGISTERS_ADDRESS_BYTE_SLEEP_5_MS) { | ||
57 | /* Test low level only available in TCP mode */ | ||
58 | /* Catch the reply and send reply byte a byte */ | ||
59 | uint8_t req[] = "\x00\x1C\x00\x00\x00\x05\xFF\x03\x02\x00\x00"; | ||
60 | @@ -203,10 +201,8 @@ int main(int argc, char *argv[]) | ||
61 | } | ||
62 | continue; | ||
63 | } | ||
64 | - | ||
65 | - } else if (query[header_length] == MODBUS_FC_WRITE_SINGLE_COIL) { | ||
66 | - if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 1) == | ||
67 | - UT_BITS_ADDRESS_INVALID_REQUEST_LENGTH) { | ||
68 | + } else if (function == MODBUS_FC_WRITE_SINGLE_COIL) { | ||
69 | + if (address == UT_BITS_ADDRESS_INVALID_REQUEST_LENGTH) { | ||
70 | // The valid length is lengths of header + checkum + FC + address + value | ||
71 | // (max 12) | ||
72 | rc = 34; | ||
73 | @@ -215,9 +211,8 @@ int main(int argc, char *argv[]) | ||
74 | "in modbus_reply\n", | ||
75 | rc); | ||
76 | } | ||
77 | - } else if (query[header_length] == MODBUS_FC_WRITE_SINGLE_REGISTER) { | ||
78 | - if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 1) == | ||
79 | - UT_REGISTERS_ADDRESS_SPECIAL) { | ||
80 | + } else if (function == MODBUS_FC_WRITE_SINGLE_REGISTER) { | ||
81 | + if (address == UT_REGISTERS_ADDRESS_SPECIAL) { | ||
82 | rc = 45; | ||
83 | printf("Special modbus_write_register detected. Inject a wrong length " | ||
84 | "value (%d) in modbus_reply\n", | ||
diff --git a/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-04.patch b/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-04.patch new file mode 100644 index 0000000000..4538054193 --- /dev/null +++ b/meta-oe/recipes-extended/libmodbus/libmodbus/CVE-2024-10918-04.patch | |||
@@ -0,0 +1,239 @@ | |||
1 | From 81bf713cf029bfa5b5da87b945c1e8817b4398f9 Mon Sep 17 00:00:00 2001 | ||
2 | From: =?UTF-8?q?St=C3=A9phane=20Raimbault?= <stephane.raimbault@gmail.com> | ||
3 | Date: Mon, 21 Oct 2024 17:32:39 +0200 | ||
4 | Subject: [PATCH] Fix request length check in modbus_reply in RTU | ||
5 | |||
6 | - rename internal *_prepare_response_tid to *_get_response_tid | ||
7 | - change signature, don't need req length anymore | ||
8 | - remove misleading modification of req_length | ||
9 | - check of req length before use in memcpy for mask write register | ||
10 | |||
11 | Related to df79a02feb253c0a9a009bcdbb21e47581315111 | ||
12 | |||
13 | CVE: CVE-2024-10918 | ||
14 | Upstream-Status: Backport [https://github.com/stephane/libmodbus/commit/81bf713cf029bfa5b5da87b945c1e8817b4398f9] | ||
15 | Signed-off-by: Peter Marko <peter.marko@siemens.com> | ||
16 | --- | ||
17 | src/modbus-private.h | 2 +- | ||
18 | src/modbus-rtu.c | 5 ++-- | ||
19 | src/modbus-tcp.c | 6 ++-- | ||
20 | src/modbus.c | 71 ++++++++++++++++++++++++++++---------------- | ||
21 | 4 files changed, 52 insertions(+), 32 deletions(-) | ||
22 | |||
23 | diff --git a/src/modbus-private.h b/src/modbus-private.h | ||
24 | index 6cd3424..ea83187 100644 | ||
25 | --- a/src/modbus-private.h | ||
26 | +++ b/src/modbus-private.h | ||
27 | @@ -75,7 +75,7 @@ typedef struct _modbus_backend { | ||
28 | int (*build_request_basis)( | ||
29 | modbus_t *ctx, int function, int addr, int nb, uint8_t *req); | ||
30 | int (*build_response_basis)(sft_t *sft, uint8_t *rsp); | ||
31 | - int (*prepare_response_tid)(const uint8_t *req, int *req_length); | ||
32 | + int (*get_response_tid)(const uint8_t *req); | ||
33 | int (*send_msg_pre)(uint8_t *req, int req_length); | ||
34 | ssize_t (*send)(modbus_t *ctx, const uint8_t *req, int req_length); | ||
35 | int (*receive)(modbus_t *ctx, uint8_t *req); | ||
36 | diff --git a/src/modbus-rtu.c b/src/modbus-rtu.c | ||
37 | index b774923..8b5ee08 100644 | ||
38 | --- a/src/modbus-rtu.c | ||
39 | +++ b/src/modbus-rtu.c | ||
40 | @@ -129,9 +129,8 @@ static uint16_t crc16(uint8_t *buffer, uint16_t buffer_length) | ||
41 | return (crc_hi << 8 | crc_lo); | ||
42 | } | ||
43 | |||
44 | -static int _modbus_rtu_prepare_response_tid(const uint8_t *req, int *req_length) | ||
45 | +static int _modbus_rtu_get_response_tid(const uint8_t *req) | ||
46 | { | ||
47 | - (*req_length) -= _MODBUS_RTU_CHECKSUM_LENGTH; | ||
48 | /* No TID */ | ||
49 | return 0; | ||
50 | } | ||
51 | @@ -1187,7 +1186,7 @@ const modbus_backend_t _modbus_rtu_backend = { | ||
52 | _modbus_set_slave, | ||
53 | _modbus_rtu_build_request_basis, | ||
54 | _modbus_rtu_build_response_basis, | ||
55 | - _modbus_rtu_prepare_response_tid, | ||
56 | + _modbus_rtu_get_response_tid, | ||
57 | _modbus_rtu_send_msg_pre, | ||
58 | _modbus_rtu_send, | ||
59 | _modbus_rtu_receive, | ||
60 | diff --git a/src/modbus-tcp.c b/src/modbus-tcp.c | ||
61 | index 733a4a9..60ac6b4 100644 | ||
62 | --- a/src/modbus-tcp.c | ||
63 | +++ b/src/modbus-tcp.c | ||
64 | @@ -151,7 +151,7 @@ static int _modbus_tcp_build_response_basis(sft_t *sft, uint8_t *rsp) | ||
65 | return _MODBUS_TCP_PRESET_RSP_LENGTH; | ||
66 | } | ||
67 | |||
68 | -static int _modbus_tcp_prepare_response_tid(const uint8_t *req, int *req_length) | ||
69 | +static int _modbus_tcp_get_response_tid(const uint8_t *req) | ||
70 | { | ||
71 | return (req[0] << 8) + req[1]; | ||
72 | } | ||
73 | @@ -812,7 +812,7 @@ const modbus_backend_t _modbus_tcp_backend = { | ||
74 | _modbus_set_slave, | ||
75 | _modbus_tcp_build_request_basis, | ||
76 | _modbus_tcp_build_response_basis, | ||
77 | - _modbus_tcp_prepare_response_tid, | ||
78 | + _modbus_tcp_get_response_tid, | ||
79 | _modbus_tcp_send_msg_pre, | ||
80 | _modbus_tcp_send, | ||
81 | _modbus_tcp_receive, | ||
82 | @@ -835,7 +835,7 @@ const modbus_backend_t _modbus_tcp_pi_backend = { | ||
83 | _modbus_set_slave, | ||
84 | _modbus_tcp_build_request_basis, | ||
85 | _modbus_tcp_build_response_basis, | ||
86 | - _modbus_tcp_prepare_response_tid, | ||
87 | + _modbus_tcp_get_response_tid, | ||
88 | _modbus_tcp_send_msg_pre, | ||
89 | _modbus_tcp_send, | ||
90 | _modbus_tcp_receive, | ||
91 | diff --git a/src/modbus.c b/src/modbus.c | ||
92 | index fe192a7..e3737bb 100644 | ||
93 | --- a/src/modbus.c | ||
94 | +++ b/src/modbus.c | ||
95 | @@ -125,7 +125,7 @@ int modbus_flush(modbus_t *ctx) | ||
96 | return rc; | ||
97 | } | ||
98 | |||
99 | -/* Computes the length of the expected response */ | ||
100 | +/* Computes the length of the expected response including checksum */ | ||
101 | static unsigned int compute_response_length_from_request(modbus_t *ctx, uint8_t *req) | ||
102 | { | ||
103 | int length; | ||
104 | @@ -386,8 +386,7 @@ int _modbus_receive_msg(modbus_t *ctx, uint8_t *msg, msg_type_t msg_type) | ||
105 | length_to_read = ctx->backend->header_length + 1; | ||
106 | |||
107 | if (msg_type == MSG_INDICATION) { | ||
108 | - /* Wait for a message, we don't know when the message will be | ||
109 | - * received */ | ||
110 | + /* Wait for a message, we don't know when the message will be received */ | ||
111 | if (ctx->indication_timeout.tv_sec == 0 && ctx->indication_timeout.tv_usec == 0) { | ||
112 | /* By default, the indication timeout isn't set */ | ||
113 | p_tv = NULL; | ||
114 | @@ -799,7 +798,7 @@ int modbus_reply(modbus_t *ctx, | ||
115 | |||
116 | sft.slave = slave; | ||
117 | sft.function = function; | ||
118 | - sft.t_id = ctx->backend->prepare_response_tid(req, &req_length); | ||
119 | + sft.t_id = ctx->backend->get_response_tid(req); | ||
120 | |||
121 | /* Data are flushed on illegal number of values errors. */ | ||
122 | switch (function) { | ||
123 | @@ -895,7 +894,7 @@ int modbus_reply(modbus_t *ctx, | ||
124 | MODBUS_EXCEPTION_ILLEGAL_DATA_ADDRESS, | ||
125 | rsp, | ||
126 | FALSE, | ||
127 | - "Illegal data address 0x%0X in write_bit\n", | ||
128 | + "Illegal data address 0x%0X in write bit\n", | ||
129 | address); | ||
130 | break; | ||
131 | } | ||
132 | @@ -904,20 +903,26 @@ int modbus_reply(modbus_t *ctx, | ||
133 | rsp_length = compute_response_length_from_request(ctx, (uint8_t *) req); | ||
134 | if (rsp_length != req_length) { | ||
135 | /* Bad use of modbus_reply */ | ||
136 | - rsp_length = | ||
137 | - response_exception(ctx, | ||
138 | - &sft, | ||
139 | - MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
140 | - rsp, | ||
141 | - FALSE, | ||
142 | - "Invalid request length used in modbus_reply (%d)\n", | ||
143 | - req_length); | ||
144 | + rsp_length = response_exception( | ||
145 | + ctx, | ||
146 | + &sft, | ||
147 | + MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
148 | + rsp, | ||
149 | + FALSE, | ||
150 | + "Invalid request length in modbus_reply to write bit (%d)\n", | ||
151 | + req_length); | ||
152 | break; | ||
153 | } | ||
154 | |||
155 | + /* Don't copy the CRC, if any, it will be computed later (even if identical to the | ||
156 | + * request) */ | ||
157 | + rsp_length -= ctx->backend->checksum_length; | ||
158 | + | ||
159 | int data = (req[offset + 3] << 8) + req[offset + 4]; | ||
160 | if (data == 0xFF00 || data == 0x0) { | ||
161 | + /* Apply the change to mapping */ | ||
162 | mb_mapping->tab_bits[mapping_address] = data ? ON : OFF; | ||
163 | + /* Prepare response */ | ||
164 | memcpy(rsp, req, rsp_length); | ||
165 | } else { | ||
166 | rsp_length = response_exception( | ||
167 | @@ -949,19 +954,21 @@ int modbus_reply(modbus_t *ctx, | ||
168 | rsp_length = compute_response_length_from_request(ctx, (uint8_t *) req); | ||
169 | if (rsp_length != req_length) { | ||
170 | /* Bad use of modbus_reply */ | ||
171 | - rsp_length = | ||
172 | - response_exception(ctx, | ||
173 | - &sft, | ||
174 | - MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
175 | - rsp, | ||
176 | - FALSE, | ||
177 | - "Invalid request length used in modbus_reply (%d)\n", | ||
178 | - req_length); | ||
179 | + rsp_length = response_exception( | ||
180 | + ctx, | ||
181 | + &sft, | ||
182 | + MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
183 | + rsp, | ||
184 | + FALSE, | ||
185 | + "Invalid request length in modbus_reply to write register (%d)\n", | ||
186 | + req_length); | ||
187 | break; | ||
188 | } | ||
189 | int data = (req[offset + 3] << 8) + req[offset + 4]; | ||
190 | |||
191 | mb_mapping->tab_registers[mapping_address] = data; | ||
192 | + | ||
193 | + rsp_length -= ctx->backend->checksum_length; | ||
194 | memcpy(rsp, req, rsp_length); | ||
195 | } break; | ||
196 | case MODBUS_FC_WRITE_MULTIPLE_COILS: { | ||
197 | @@ -1082,8 +1089,23 @@ int modbus_reply(modbus_t *ctx, | ||
198 | |||
199 | data = (data & and) | (or &(~and)); | ||
200 | mb_mapping->tab_registers[mapping_address] = data; | ||
201 | - memcpy(rsp, req, req_length); | ||
202 | - rsp_length = req_length; | ||
203 | + | ||
204 | + rsp_length = compute_response_length_from_request(ctx, (uint8_t *) req); | ||
205 | + if (rsp_length != req_length) { | ||
206 | + /* Bad use of modbus_reply */ | ||
207 | + rsp_length = response_exception(ctx, | ||
208 | + &sft, | ||
209 | + MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, | ||
210 | + rsp, | ||
211 | + FALSE, | ||
212 | + "Invalid request length in modbus_reply " | ||
213 | + "to mask write register (%d)\n", | ||
214 | + req_length); | ||
215 | + break; | ||
216 | + } | ||
217 | + | ||
218 | + rsp_length -= ctx->backend->checksum_length; | ||
219 | + memcpy(rsp, req, rsp_length); | ||
220 | } | ||
221 | } break; | ||
222 | case MODBUS_FC_WRITE_AND_READ_REGISTERS: { | ||
223 | @@ -1171,7 +1193,6 @@ int modbus_reply_exception(modbus_t *ctx, const uint8_t *req, unsigned int excep | ||
224 | int function; | ||
225 | uint8_t rsp[MAX_MESSAGE_LENGTH]; | ||
226 | int rsp_length; | ||
227 | - int dummy_length = 99; | ||
228 | sft_t sft; | ||
229 | |||
230 | if (ctx == NULL) { | ||
231 | @@ -1185,7 +1206,7 @@ int modbus_reply_exception(modbus_t *ctx, const uint8_t *req, unsigned int excep | ||
232 | |||
233 | sft.slave = slave; | ||
234 | sft.function = function + 0x80; | ||
235 | - sft.t_id = ctx->backend->prepare_response_tid(req, &dummy_length); | ||
236 | + sft.t_id = ctx->backend->get_response_tid(req); | ||
237 | rsp_length = ctx->backend->build_response_basis(&sft, rsp); | ||
238 | |||
239 | /* Positive exception code */ | ||
diff --git a/meta-oe/recipes-extended/libmodbus/libmodbus_3.1.10.bb b/meta-oe/recipes-extended/libmodbus/libmodbus_3.1.10.bb index 9e17f91669..c8e1c3a3e2 100644 --- a/meta-oe/recipes-extended/libmodbus/libmodbus_3.1.10.bb +++ b/meta-oe/recipes-extended/libmodbus/libmodbus_3.1.10.bb | |||
@@ -6,7 +6,13 @@ SECTION = "libs" | |||
6 | LICENSE = "LGPL-2.1-or-later" | 6 | LICENSE = "LGPL-2.1-or-later" |
7 | LIC_FILES_CHKSUM = "file://COPYING.LESSER;md5=4fbd65380cdd255951079008b364516c" | 7 | LIC_FILES_CHKSUM = "file://COPYING.LESSER;md5=4fbd65380cdd255951079008b364516c" |
8 | 8 | ||
9 | SRC_URI = "git://github.com/stephane/libmodbus;branch=master;protocol=https" | 9 | SRC_URI = " \ |
10 | git://github.com/stephane/libmodbus;branch=master;protocol=https \ | ||
11 | file://CVE-2024-10918-01.patch \ | ||
12 | file://CVE-2024-10918-02.patch \ | ||
13 | file://CVE-2024-10918-03.patch \ | ||
14 | file://CVE-2024-10918-04.patch \ | ||
15 | " | ||
10 | SRCREV = "2cbafa3113e276c3697d297f68e88d112b53174d" | 16 | SRCREV = "2cbafa3113e276c3697d297f68e88d112b53174d" |
11 | 17 | ||
12 | S = "${WORKDIR}/git" | 18 | S = "${WORKDIR}/git" |