diff --git a/src/modbus.c b/src/modbus.c index 9b894d9..88be932 100644 --- a/src/modbus.c +++ b/src/modbus.c @@ -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; diff --git a/src/modbus.h b/src/modbus.h index 7e7ad4f..e14607c 100644 --- a/src/modbus.h +++ b/src/modbus.h @@ -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 diff --git a/tests/bandwidth-client.c b/tests/bandwidth-client.c index b4a173f..90857cb 100644 --- a/tests/bandwidth-client.c +++ b/tests/bandwidth-client.c @@ -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 + * Copyright © 2008-2013 Stéphane Raimbault * * 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"); diff --git a/tests/unit-test.h.in b/tests/unit-test.h.in index 0c15bfd..4c6c9f5 100644 --- a/tests/unit-test.h.in +++ b/tests/unit-test.h.in @@ -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 };