Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fix remote buffer overflow vulnerability on write requests

Protects against crafted write requests with a large quantity but a small
byte count. If address + quantity was in the mapping space of the server
and quantity greater than the response size, it was possible to crash
the server.

The sleep/flush sequence improves the handling of following requests.
  • Loading branch information...
commit 83c3410267d1383b36e0928d994fefdd634f1a1d 1 parent 29851c2
@stephane authored
Showing with 162 additions and 77 deletions.
  1. +44 −3 src/modbus.c
  2. +113 −72 tests/unit-test-client.c
  3. +5 −2 tests/unit-test-server.c
View
47 src/modbus.c
@@ -718,6 +718,8 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
"Illegal nb of values %d in read_bits (max %d)\n",
nb, MODBUS_MAX_READ_BITS);
}
+ _sleep_response_timeout(ctx);
+ modbus_flush(ctx);
rsp_length = response_exception(
ctx, &sft,
MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp);
@@ -749,6 +751,8 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
"Illegal nb of values %d in read_input_bits (max %d)\n",
nb, MODBUS_MAX_READ_BITS);
}
+ _sleep_response_timeout(ctx);
+ modbus_flush(ctx);
rsp_length = response_exception(
ctx, &sft,
MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp);
@@ -778,6 +782,8 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
"Illegal nb of values %d in read_holding_registers (max %d)\n",
nb, MODBUS_MAX_READ_REGISTERS);
}
+ _sleep_response_timeout(ctx);
+ modbus_flush(ctx);
rsp_length = response_exception(
ctx, &sft,
MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp);
@@ -812,6 +818,8 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
"Illegal number of values %d in read_input_registers (max %d)\n",
nb, MODBUS_MAX_READ_REGISTERS);
}
+ _sleep_response_timeout(ctx);
+ modbus_flush(ctx);
rsp_length = response_exception(
ctx, &sft,
MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp);
@@ -842,6 +850,8 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
"Illegal data address %0X in write_bit\n",
address);
}
+ _sleep_response_timeout(ctx);
+ modbus_flush(ctx);
rsp_length = response_exception(
ctx, &sft,
MODBUS_EXCEPTION_ILLEGAL_DATA_ADDRESS, rsp);
@@ -870,6 +880,8 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
fprintf(stderr, "Illegal data address %0X in write_register\n",
address);
}
+ _sleep_response_timeout(ctx);
+ modbus_flush(ctx);
rsp_length = response_exception(
ctx, &sft,
MODBUS_EXCEPTION_ILLEGAL_DATA_ADDRESS, rsp);
@@ -884,7 +896,21 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
case MODBUS_FC_WRITE_MULTIPLE_COILS: {
int nb = (req[offset + 3] << 8) + req[offset + 4];
- if ((address + nb) > mb_mapping->nb_bits) {
+ if (nb < 1 || MODBUS_MAX_WRITE_BITS < nb) {
+ if (ctx->debug) {
+ fprintf(stderr,
+ "Illegal number of values %d in write_bits (max %d)\n",
+ nb, MODBUS_MAX_WRITE_BITS);
+ }
+ /* May be the indication has been truncated on reading because of
+ * invalid address (eg. nb is 0 but the request contains values to
+ * write) so it's necessary to flush. */
+ _sleep_response_timeout(ctx);
+ modbus_flush(ctx);
+ rsp_length = response_exception(
+ ctx, &sft,
+ MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp);
+ } else if ((address + nb) > mb_mapping->nb_bits) {
if (ctx->debug) {
fprintf(stderr, "Illegal data address %0X in write_bits\n",
address + nb);
@@ -905,8 +931,21 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
break;
case MODBUS_FC_WRITE_MULTIPLE_REGISTERS: {
int nb = (req[offset + 3] << 8) + req[offset + 4];
-
- if ((address + nb) > mb_mapping->nb_registers) {
+ if (nb < 1 || MODBUS_MAX_WRITE_REGISTERS < nb) {
+ if (ctx->debug) {
+ fprintf(stderr,
+ "Illegal number of values %d in write_registers (max %d)\n",
+ nb, MODBUS_MAX_WRITE_REGISTERS);
+ }
+ /* May be the indication has been truncated on reading because of
+ * invalid address (eg. nb is 0 but the request contains values to
+ * write) so it's necessary to flush. */
+ _sleep_response_timeout(ctx);
+ modbus_flush(ctx);
+ rsp_length = response_exception(
+ ctx, &sft,
+ MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp);
+ } else if ((address + nb) > mb_mapping->nb_registers) {
if (ctx->debug) {
fprintf(stderr, "Illegal data address %0X in write_registers\n",
address + nb);
@@ -988,6 +1027,8 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
nb_write, nb,
MODBUS_MAX_WR_WRITE_REGISTERS, MODBUS_MAX_WR_READ_REGISTERS);
}
+ _sleep_response_timeout(ctx);
+ modbus_flush(ctx);
rsp_length = response_exception(
ctx, &sft,
MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp);
View
185 tests/unit-test-client.c
@@ -30,7 +30,11 @@ enum {
RTU
};
-int test_raw_request(modbus_t *, int);
+int test_server(modbus_t *ctx, int use_backend);
+int send_crafted_request(modbus_t *ctx, int function,
+ uint8_t *req, int req_size,
+ uint16_t max_value, uint16_t bytes,
+ int backend_length, int backend_offset);
#define BUG_REPORT(_cond, _format, _args ...) \
printf("\nLine %d: assertion error for '%s': " _format "\n", __LINE__, # _cond, ## _args)
@@ -205,7 +209,6 @@ int main(int argc, char *argv[])
ASSERT_TRUE(rc == 1, "FAILED (nb points %d)\n", rc);
ASSERT_TRUE(tab_rp_registers[0] == 0x1234, "FAILED (%0X != %0X)\n",
tab_rp_registers[0], 0x1234);
-
/* End of single register */
/* Many registers */
@@ -509,7 +512,7 @@ int main(int argc, char *argv[])
/* A wait and flush operation is done by the error recovery code of
* libmodbus but after a sleep of current response timeout
- * so 0 can't be too short!
+ * so 0 can be too short!
*/
usleep(old_response_to_sec * 1000000 + old_response_to_usec);
modbus_flush(ctx);
@@ -590,8 +593,8 @@ int main(int argc, char *argv[])
printf("* modbus_read_registers at special address: ");
ASSERT_TRUE(rc == -1 && errno == EMBXSBUSY, "");
- /** RAW REQUEST */
- if (test_raw_request(ctx, use_backend) == -1) {
+ /** SERVER **/
+ if (test_server(ctx, use_backend) == -1) {
goto close;
}
@@ -617,19 +620,23 @@ int main(int argc, char *argv[])
return 0;
}
-int test_raw_request(modbus_t *ctx, int use_backend)
+/* Send crafted requests to test server resilience
+ and ensure proper exceptions are returned. */
+int test_server(modbus_t *ctx, int use_backend)
{
int rc;
- int i, j;
- const int RAW_REQ_LENGTH = 6;
- uint8_t raw_req[] = {
+ int i;
+ /* Read requests */
+ const int READ_RAW_REQ_LEN = 6;
+ uint8_t read_raw_req[] = {
/* slave */
(use_backend == RTU) ? SERVER_ID : 0xFF,
/* function, addr 1, 5 values */
- MODBUS_FC_READ_HOLDING_REGISTERS, 0x00, 0x01, 0x0, 0x05,
+ MODBUS_FC_READ_HOLDING_REGISTERS, 0x00, 0x01, 0x0, 0x05
};
/* Write and read registers request */
- uint8_t raw_rw_req[] = {
+ const int RW_RAW_REQ_LEN = 13;
+ uint8_t rw_raw_req[] = {
/* slave */
(use_backend == RTU) ? SERVER_ID : 0xFF,
/* function, addr to read, nb to read */
@@ -646,104 +653,138 @@ int test_raw_request(modbus_t *ctx, int use_backend)
/* One data to write... */
0x12, 0x34
};
- /* See issue #143, test with MAX_WR_WRITE_REGISTERS */
+ const int WRITE_RAW_REQ_LEN = 13;
+ uint8_t write_raw_req[] = {
+ /* slave */
+ (use_backend == RTU) ? SERVER_ID : 0xFF,
+ /* function will be set in the loop */
+ MODBUS_FC_WRITE_MULTIPLE_REGISTERS,
+ /* Address */
+ UT_REGISTERS_ADDRESS >> 8,
+ UT_REGISTERS_ADDRESS & 0xFF,
+ /* 3 values, 6 bytes */
+ 0x00, 0x03, 0x06,
+ /* Dummy data to write */
+ 0x02, 0x2B, 0x00, 0x01, 0x00, 0x64
+ };
int req_length;
uint8_t rsp[MODBUS_TCP_MAX_ADU_LENGTH];
- int tab_function[] = {
+ int tab_read_function[] = {
MODBUS_FC_READ_COILS,
MODBUS_FC_READ_DISCRETE_INPUTS,
MODBUS_FC_READ_HOLDING_REGISTERS,
MODBUS_FC_READ_INPUT_REGISTERS
};
- int tab_nb_max[] = {
+ int tab_read_nb_max[] = {
MODBUS_MAX_READ_BITS + 1,
MODBUS_MAX_READ_BITS + 1,
MODBUS_MAX_READ_REGISTERS + 1,
MODBUS_MAX_READ_REGISTERS + 1
};
- int length;
- int offset;
- const int EXCEPTION_RC = 2;
+ int backend_length;
+ int backend_offset;
if (use_backend == RTU) {
- length = 3;
- offset = 1;
+ backend_length = 3;
+ backend_offset = 1;
} else {
- length = 7;
- offset = 7;
+ backend_length = 7;
+ backend_offset = 7;
}
printf("\nTEST RAW REQUESTS:\n");
- req_length = modbus_send_raw_request(ctx, raw_req,
- RAW_REQ_LENGTH * sizeof(uint8_t));
+ req_length = modbus_send_raw_request(ctx, read_raw_req, READ_RAW_REQ_LEN);
printf("* modbus_send_raw_request: ");
- ASSERT_TRUE(req_length == (length + 5), "FAILED (%d)\n", req_length);
+ ASSERT_TRUE(req_length == (backend_length + 5), "FAILED (%d)\n", req_length);
printf("* modbus_receive_confirmation: ");
- rc = modbus_receive_confirmation(ctx, rsp);
- ASSERT_TRUE(rc == (length + 12), "FAILED (%d)\n", rc);
-
- /* Try to crash server with raw requests to bypass checks of client. */
-
- /* Address */
- raw_req[2] = 0;
- raw_req[3] = 0;
+ rc = modbus_receive_confirmation(ctx, rsp);
+ ASSERT_TRUE(rc == (backend_length + 12), "FAILED (%d)\n", rc);
/* Try to read more values than a response could hold for all data
* types.
*/
for (i=0; i<4; i++) {
- raw_req[1] = tab_function[i];
-
- for (j=0; j<2; j++) {
- if (j == 0) {
- /* Try to read zero values on first iteration */
- raw_req[4] = 0x00;
- raw_req[5] = 0x00;
- } else {
- /* Try to read max values + 1 on second iteration */
- raw_req[4] = (tab_nb_max[i] >> 8) & 0xFF;
- raw_req[5] = tab_nb_max[i] & 0xFF;
- }
-
- req_length = modbus_send_raw_request(ctx, raw_req,
- RAW_REQ_LENGTH * sizeof(uint8_t));
- if (j == 0) {
- printf("* try to read 0 values with function %d: ", tab_function[i]);
- } else {
- printf("* try an exploit with function %d: ", tab_function[i]);
- }
- rc = modbus_receive_confirmation(ctx, rsp);
- ASSERT_TRUE(rc == (length + EXCEPTION_RC) &&
- rsp[offset] == (0x80 + tab_function[i]) &&
- rsp[offset + 1] == MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, "");
- }
+ rc = send_crafted_request(ctx, tab_read_function[i],
+ read_raw_req, READ_RAW_REQ_LEN,
+ tab_read_nb_max[i], 0,
+ backend_length, backend_offset);
+ if (rc == -1)
+ goto close;
}
/* Modbus write and read multiple registers */
- i = 0;
- tab_function[i] = MODBUS_FC_WRITE_AND_READ_REGISTERS;
- for (j=0; j<2; j++) {
+ rc = send_crafted_request(ctx, MODBUS_FC_WRITE_AND_READ_REGISTERS,
+ rw_raw_req, RW_RAW_REQ_LEN,
+ MODBUS_MAX_WR_READ_REGISTERS + 1, 0,
+ backend_length, backend_offset);
+ if (rc == -1)
+ goto close;
+
+ /* Modbus write multiple registers with large number of values but a set a
+ small number of bytes in requests (not nb * 2 as usual). */
+ rc = send_crafted_request(ctx, MODBUS_FC_WRITE_MULTIPLE_REGISTERS,
+ write_raw_req, WRITE_RAW_REQ_LEN,
+ MODBUS_MAX_WRITE_REGISTERS + 1, 6,
+ backend_length, backend_offset);
+ if (rc == -1)
+ goto close;
+
+ rc = send_crafted_request(ctx, MODBUS_FC_WRITE_MULTIPLE_COILS,
+ write_raw_req, WRITE_RAW_REQ_LEN,
+ MODBUS_MAX_WRITE_BITS + 1, 6,
+ backend_length, backend_offset);
+ if (rc == -1)
+ goto close;
+
+ return 0;
+close:
+ return -1;
+}
+
+
+int send_crafted_request(modbus_t *ctx, int function,
+ uint8_t *req, int req_len,
+ uint16_t max_value, uint16_t bytes,
+ int backend_length, int backend_offset)
+{
+ const int EXCEPTION_RC = 2;
+ uint8_t rsp[MODBUS_TCP_MAX_ADU_LENGTH];
+
+ for (int j=0; j<2; j++) {
+ int rc;
+
+ req[1] = function;
if (j == 0) {
- /* Try to read zero values on first iteration */
- raw_rw_req[4] = 0x00;
- raw_rw_req[5] = 0x00;
+ /* Try to read or write zero values on first iteration */
+ req[4] = 0x00;
+ req[5] = 0x00;
+ if (bytes) {
+ /* Write query */
+ req[6] = 0x00;
+ }
} else {
- /* Try to read max values + 1 on second iteration */
- raw_rw_req[4] = (MODBUS_MAX_WR_READ_REGISTERS + 1) >> 8;
- raw_rw_req[5] = (MODBUS_MAX_WR_READ_REGISTERS + 1) & 0xFF;
+ /* Try to read or write max values + 1 on second iteration */
+ req[4] = (max_value >> 8) & 0xFF;
+ req[5] = max_value & 0xFF;
+ if (bytes) {
+ /* Write query (nb values * 2 to convert in bytes for registers) */
+ req[6] = bytes;
+ }
}
- req_length = modbus_send_raw_request(ctx, raw_rw_req, 13 * sizeof(uint8_t));
+
+ modbus_send_raw_request(ctx, req, req_len * sizeof(uint8_t));
if (j == 0) {
- printf("* try to read 0 values with function %d: ", tab_function[i]);
+ printf("* try function 0x%X: %s 0 values: ", function, bytes ? "write": "read");
} else {
- printf("* try an exploit with function %d: ", tab_function[i]);
+ printf("* try function 0x%X: %s %d values: ", function, bytes ? "write": "read",
+ max_value);
}
rc = modbus_receive_confirmation(ctx, rsp);
- ASSERT_TRUE(rc == length + EXCEPTION_RC &&
- rsp[offset] == (0x80 + tab_function[i]) &&
- rsp[offset + 1] == MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, "");
+ ASSERT_TRUE(rc == (backend_length + EXCEPTION_RC) &&
+ rsp[backend_offset] == (0x80 + function) &&
+ rsp[backend_offset + 1] == MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, "");
}
return 0;
View
7 tests/unit-test-server.c
@@ -156,12 +156,15 @@ int main(int argc, char*argv[])
if (rc == -1) {
/* Connection closed by the client or error */
+ /* We could answer with an exception on EMBBADDATA to indicate
+ illegal data for example */
break;
}
-
- /* Read holding registers */
+ /* Special server behavior to test client */
if (query[header_length] == 0x03) {
+ /* Read holding registers */
+
if (MODBUS_GET_INT16_FROM_INT8(query, header_length + 3)
== UT_REGISTERS_NB_SPECIAL) {
printf("Set an incorrect number of values\n");
Please sign in to comment.
Something went wrong with that request. Please try again.