Fix remote buffer overflow vulnerability (closes #25, #105)

It's strongly recommended to update your libmodbus library if you use it in a slave/server application in a not trusted environment. Debian package of libmodbus 3.0.4 already contains a patch to mitigate the exploit but the patch isn't as strong than this one.

Fix remote buffer overflow vulnerability (closes #25, #105)
It's strongly recommended to update your libmodbus library if you use it in a slave/server application in a not trusted environment. Debian package of libmodbus 3.0.4 already contains a patch to mitigate the exploit but the patch isn't as strong than this one.
Stéphane Raimbault
1 parent 2a9a7ddf
Showing 6 changed files with 131 additions and 31 deletions
src/modbus.c
src/modbus.h
tests/bandwidth-client.c
tests/bandwidth-server-one.c
tests/unit-test-client.c
tests/unit-test.h.in
@@ -706,7 +706,16 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
     case _FC_READ_COILS: {
         int nb = (req[offset + 3] << 8) + req[offset + 4];
-        if ((address + nb) > mb_mapping->nb_bits) {
+        if (nb < 1 || MODBUS_MAX_READ_BITS < nb) {
+            if (ctx->debug) {
+                fprintf(stderr,
+                        "Illegal nb of values %d in read_bits (max %d)\n",
+                        nb, MODBUS_MAX_READ_BITS);
+            }
+            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 read_bits\n",
                         address + nb);
@@ -728,7 +737,16 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
          * function) */
         int nb = (req[offset + 3] << 8) + req[offset + 4];
-        if ((address + nb) > mb_mapping->nb_input_bits) {
+        if (nb < 1 || MODBUS_MAX_READ_BITS < nb) {
+            if (ctx->debug) {
+                fprintf(stderr,
+                        "Illegal nb of values %d in read_input_bits (max %d)\n",
+                        nb, MODBUS_MAX_READ_BITS);
+            }
+            rsp_length = response_exception(
+                ctx, &sft,
+                MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp);
+        } else if ((address + nb) > mb_mapping->nb_input_bits) {
             if (ctx->debug) {
                 fprintf(stderr, "Illegal data address %0X in read_input_bits\n",
                         address + nb);
@@ -748,7 +766,16 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
     case _FC_READ_HOLDING_REGISTERS: {
         int nb = (req[offset + 3] << 8) + req[offset + 4];
-        if ((address + nb) > mb_mapping->nb_registers) {
+        if (nb < 1 || MODBUS_MAX_READ_REGISTERS < nb) {
+            if (ctx->debug) {
+                fprintf(stderr,
+                        "Illegal nb of values %d in read_holding_registers (max %d)\n",
+                        nb, MODBUS_MAX_READ_REGISTERS);
+            }
+            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 read_registers\n",
                         address + nb);
@@ -773,7 +800,16 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
          * function) */
         int nb = (req[offset + 3] << 8) + req[offset + 4];
-        if ((address + nb) > mb_mapping->nb_input_registers) {
+        if (nb < 1 || MODBUS_MAX_READ_REGISTERS < nb) {
+            if (ctx->debug) {
+                fprintf(stderr,
+                        "Illegal number of values %d in read_input_registers (max %d)\n",
+                        nb, MODBUS_MAX_READ_REGISTERS);
+            }
+            rsp_length = response_exception(
+                ctx, &sft,
+                MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp);
+        } else if ((address + nb) > mb_mapping->nb_input_registers) {
             if (ctx->debug) {
                 fprintf(stderr, "Illegal data address %0X in read_input_registers\n",
                         address + nb);
@@ -796,7 +832,8 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
     case _FC_WRITE_SINGLE_COIL:
         if (address >= mb_mapping->nb_bits) {
             if (ctx->debug) {
-                fprintf(stderr, "Illegal data address %0X in write_bit\n",
+                fprintf(stderr,
+                        "Illegal data address %0X in write_bit\n",
                         address);
             }
             rsp_length = response_exception(
@@ -934,9 +971,22 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
         int nb = (req[offset + 3] << 8) + req[offset + 4];
         uint16_t address_write = (req[offset + 5] << 8) + req[offset + 6];
         int nb_write = (req[offset + 7] << 8) + req[offset + 8];
+        int nb_write_bytes = req[offset + 9];
-        if ((address + nb) > mb_mapping->nb_registers ||
-            (address_write + nb_write) > mb_mapping->nb_registers) {
+        if (nb_write < 1 || MODBUS_MAX_WR_WRITE_REGISTERS < nb_write ||
+            nb < 1 || MODBUS_MAX_WR_READ_REGISTERS < nb ||
+            nb_write_bytes != nb_write * 2) {
+            if (ctx->debug) {
+                fprintf(stderr,
+                        "Illegal nb of values (W%d, R%d) in write_and_read_registers (max W%d, R%d)\n",
+                        nb_write, nb,
+                        MODBUS_MAX_WR_WRITE_REGISTERS, MODBUS_MAX_WR_READ_REGISTERS);
+            }
+            rsp_length = response_exception(
+                ctx, &sft,
+                MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp);
+        } else if ((address + nb) > mb_mapping->nb_registers ||
+                   (address_write + nb_write) > mb_mapping->nb_registers) {
             if (ctx->debug) {
                 fprintf(stderr,
                         "Illegal data read address %0X or write address %0X write_and_read_registers\n",
@@ -1420,21 +1470,21 @@ int modbus_write_and_read_registers(modbus_t *ctx,
         return -1;
     }
-    if (write_nb > MODBUS_MAX_RW_WRITE_REGISTERS) {
+    if (write_nb > MODBUS_MAX_WR_WRITE_REGISTERS) {
         if (ctx->debug) {
             fprintf(stderr,
                     "ERROR Too many registers to write (%d > %d)\n",
-                    write_nb, MODBUS_MAX_RW_WRITE_REGISTERS);
+                    write_nb, MODBUS_MAX_WR_WRITE_REGISTERS);
         }
         errno = EMBMDATA;
         return -1;
     }
-    if (read_nb > MODBUS_MAX_READ_REGISTERS) {
+    if (read_nb > MODBUS_MAX_WR_READ_REGISTERS) {
         if (ctx->debug) {
             fprintf(stderr,
                     "ERROR Too many registers requested (%d > %d)\n",
-                    read_nb, MODBUS_MAX_READ_REGISTERS);
+                    read_nb, MODBUS_MAX_WR_READ_REGISTERS);
         }
         errno = EMBMDATA;
         return -1;
@@ -90,7 +90,8 @@ MODBUS_BEGIN_DECLS
  */
 #define MODBUS_MAX_READ_REGISTERS          125
 #define MODBUS_MAX_WRITE_REGISTERS         123
-#define MODBUS_MAX_RW_WRITE_REGISTERS      121
+#define MODBUS_MAX_WR_WRITE_REGISTERS      121
+#define MODBUS_MAX_WR_READ_REGISTERS       125
 /* Random number to avoid errno conflicts */
 #define MODBUS_ENOBASE 112345678
@@ -179,9 +179,9 @@ int main(int argc, char *argv[])
     printf("* %d KiB/s\n", rate);
     printf("\n\n");
-    printf("READ AND WRITE REGISTERS\n\n");
+    printf("WRITE AND READ REGISTERS\n\n");
-    nb_points = MODBUS_MAX_RW_WRITE_REGISTERS;
+    nb_points = MODBUS_MAX_WR_WRITE_REGISTERS;
     start = gettime_ms();
     for (i=0; i<n_loop; i++) {
         rc = modbus_write_and_read_registers(ctx,
@@ -59,6 +59,7 @@ int main(int argc, char *argv[])
     if (use_backend == TCP) {
         ctx = modbus_new_tcp("127.0.0.1", 1502);
+        modbus_set_debug(ctx, TRUE);
         s = modbus_tcp_listen(ctx, 1);
         modbus_tcp_accept(ctx, &s);
@@ -68,8 +69,7 @@ int main(int argc, char *argv[])
         modbus_connect(ctx);
     }
-    mb_mapping = modbus_mapping_new(MODBUS_MAX_READ_BITS, 0,
-                                    MODBUS_MAX_READ_REGISTERS, 0);
+    mb_mapping = modbus_mapping_new(1000, 1000, 1000, 1000);
     if (mb_mapping == NULL) {
         fprintf(stderr, "Failed to allocate the mapping: %s\n",
                 modbus_strerror(errno));
 /*
- * Copyright © 2008-2010 Stéphane Raimbault <stephane.raimbault@gmail.com>
+ * Copyright © 2008-2013 Stéphane Raimbault <stephane.raimbault@gmail.com>
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -255,8 +255,8 @@ int main(int argc, char *argv[])
     rc = modbus_read_registers(ctx, UT_REGISTERS_ADDRESS,
                                0, tab_rp_registers);
     printf("3/5 modbus_read_registers (0): ");
-    if (rc != 0) {
-        printf("FAILED (nb points %d)\n", rc);
+    if (rc != -1) {
+        printf("FAILED (nb_points %d)\n", rc);
         goto close;
     }
     printf("OK\n");
@@ -270,7 +270,8 @@ int main(int argc, char *argv[])
        into tab_rp_registers. So the read registers must set to 0, except the
        first one because there is an offset of 1 register on write. */
     rc = modbus_write_and_read_registers(ctx,
-                                         UT_REGISTERS_ADDRESS + 1, UT_REGISTERS_NB - 1,
+                                         UT_REGISTERS_ADDRESS + 1,
+                                         UT_REGISTERS_NB - 1,
                                          tab_rp_registers,
                                          UT_REGISTERS_ADDRESS,
                                          UT_REGISTERS_NB,
@@ -430,8 +431,8 @@ int main(int argc, char *argv[])
         goto close;
     }
-    rc = modbus_write_registers(ctx, UT_REGISTERS_ADDRESS + UT_REGISTERS_NB,
-                                UT_REGISTERS_NB, tab_rp_registers);
+    rc = modbus_read_registers(ctx, UT_REGISTERS_ADDRESS + UT_REGISTERS_NB,
+                               UT_REGISTERS_NB, tab_rp_registers);
     printf("* modbus_write_registers: ");
     if (rc == -1 && errno == EMBXILADD) {
         printf("OK\n");
@@ -440,7 +441,6 @@ int main(int argc, char *argv[])
         goto close;
     }
-
     /** TOO MANY DATA **/
     printf("\nTEST TOO MANY DATA ERROR:\n");
@@ -535,7 +535,7 @@ int main(int argc, char *argv[])
          * indication for another slave is received, a confirmation must follow */
-        /* Send a pair of indication/confimration to the slave with a different
+        /* Send a pair of indication/confirmation to the slave with a different
          * slave ID to simulate a communication on a RS485 bus. At first, the
          * slave will see the indication message then the confirmation, and it must
          * ignore both. */
@@ -699,13 +699,23 @@ int main(int argc, char *argv[])
     }
     /** RAW REQUEST */
-    printf("\nTEST RAW REQUEST:\n");
+    printf("\nTEST RAW REQUESTS:\n");
     {
+        int j;
         const int RAW_REQ_LENGTH = 6;
-        uint8_t raw_req[] = { (use_backend == RTU) ? SERVER_ID : 0xFF,
-                              0x03, 0x00, 0x01, 0x0, 0x05 };
+        uint8_t raw_req[] = {
+            (use_backend == RTU) ? SERVER_ID : 0xFF,
+            0x03, 0x00, 0x01, 0x0, 0x05,
+        };
         int req_length;
         uint8_t rsp[MODBUS_TCP_MAX_ADU_LENGTH];
+        int tab_function[] = {0x01, 0x02, 0x03, 0x04};
+        int tab_nb_max[] = {
+            MODBUS_MAX_READ_BITS + 1,
+            MODBUS_MAX_READ_BITS + 1,
+            MODBUS_MAX_READ_REGISTERS + 1,
+            MODBUS_MAX_READ_REGISTERS + 1
+        };
         req_length = modbus_send_raw_request(ctx, raw_req,
                                              RAW_REQ_LENGTH * sizeof(uint8_t));
@@ -730,6 +740,44 @@ int main(int argc, char *argv[])
             printf("FAILED (%d)\n", rc);
             goto close;
         }
+
+        /* Try to crash server with raw requests to bypass checks of client. */
+
+        /* Address */
+        raw_req[2] = 0;
+        raw_req[3] = 0;
+
+        /* 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));
+                printf("* try an exploit on function %d: ", tab_function[i]);
+                rc  = modbus_receive_confirmation(ctx, rsp);
+                if (rc == 9 &&
+                    rsp[7] == (0x80 + tab_function[i]) &&
+                    rsp[8] == MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE) {
+                    printf("OK\n");
+                } else {
+                    printf("FAILED\n");
+                    goto close;
+                }
+            }
+        }
     }
     printf("\nALL TESTS PASS WITH SUCCESS.\n");
@@ -36,15 +36,16 @@
 #define SERVER_ID         17
 #define INVALID_SERVER_ID 18
-const uint16_t UT_BITS_ADDRESS = 0x13;
+/* Server allocates address + nb */
+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_INPUT_BITS_ADDRESS = 0xC4;
+const uint16_t UT_INPUT_BITS_ADDRESS = 0x1C4;
 const uint16_t UT_INPUT_BITS_NB = 0x16;
 const uint8_t UT_INPUT_BITS_TAB[] = { 0xAC, 0xDB, 0x35 };
-const uint16_t UT_REGISTERS_ADDRESS = 0x6B;
+const uint16_t UT_REGISTERS_ADDRESS = 0x16B;
 /* Raise a manual exception when this address is used for the first byte */
 const uint16_t UT_REGISTERS_ADDRESS_SPECIAL = 0x6C;
 /* The response of the server will contains an invalid TID or slave */
@@ -57,7 +58,7 @@ const uint16_t UT_REGISTERS_TAB[] = { 0x022B, 0x0001, 0x0064 };
    UT_REGISTERS_NB_POINTS to try to raise a segfault. */
 const uint16_t UT_REGISTERS_NB_SPECIAL = 0x2;
-const uint16_t UT_INPUT_REGISTERS_ADDRESS = 0x08;
+const uint16_t UT_INPUT_REGISTERS_ADDRESS = 0x108;
 const uint16_t UT_INPUT_REGISTERS_NB = 0x1;
 const uint16_t UT_INPUT_REGISTERS_TAB[] = { 0x000A };