modbus_reply() doesn't check max read/write register counts. #25

Closed
LetoThe2nd opened this Issue Sep 22, 2011 · 3 comments

Comments

Projects
None yet
2 participants
@LetoThe2nd

As discussed on IRC Sept. 22 2011, the title says it all.
Please find a patch suggestion attached.

From 58cf895 Mon Sep 17 00:00:00 2001
From: Josef Holzmayr holzmayr@rsi-elektrotechnik.de
Date: Thu, 22 Sep 2011 14:31:39 +0200
Subject: [PATCH] Add register count checks to modbus_reply

Add checks so modbus_reply returns a
MODBUS_EXCEPTION_ILLEGAL_DATA_ADDRESS if the count of requested

registers exceeds the spec as noted in modbus.h, line 73ff.

src/modbus.c | 22 +++++++++++++++-------
1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/modbus.c b/src/modbus.c
index 2860d29..64b9d92 100644
--- a/src/modbus.c
+++ b/src/modbus.c
@@ -662,7 +662,8 @@ 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 ((address + nb) > mb_mapping->nb_bits ||
    
  •       nb > MODBUS_MAX_READ_REGISTERS) {
         if (ctx->debug) {
             fprintf(stderr, "Illegal data address %0X in read_bits\n",
                     address + nb);
    

    @@ -684,7 +685,8 @@ 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 ((address + nb) > mb_mapping->nb_input_bits ||
    
  •       nb > MODBUS_MAX_READ_REGISTERS) {
         if (ctx->debug) {
             fprintf(stderr, "Illegal data address %0X in read_input_bits\n",
                     address + nb);
    

    @@ -704,7 +706,8 @@ 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 ((address + nb) > mb_mapping->nb_registers ||
    
  •       nb > MODBUS_MAX_READ_REGISTERS) {
         if (ctx->debug) {
             fprintf(stderr, "Illegal data address %0X in read_registers\n",
                     address + nb);
    

    @@ -729,7 +732,8 @@ 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 ((address + nb) > mb_mapping->nb_input_registers ||
    
  •       nb > MODBUS_MAX_READ_REGISTERS) {
         if (ctx->debug) {
             fprintf(stderr, "Illegal data address %0X in read_input_registers\n",
                     address + nb);
    

    @@ -797,7 +801,8 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
    case _FC_WRITE_MULTIPLE_COILS: {
    int nb = (req[offset + 3] << 8) + req[offset + 4];

  •    if ((address + nb) > mb_mapping->nb_bits) {
    
  •    if ((address + nb) > mb_mapping->nb_bits ||
    
  •       nb > MODBUS_MAX_WRITE_REGISTERS) {
         if (ctx->debug) {
             fprintf(stderr, "Illegal data address %0X in write_bits\n",
                     address + nb);
    

    @@ -819,7 +824,8 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
    case _FC_WRITE_MULTIPLE_REGISTERS: {
    int nb = (req[offset + 3] << 8) + req[offset + 4];

  •    if ((address + nb) > mb_mapping->nb_registers) {
    
  •    if ((address + nb) > mb_mapping->nb_registers ||
    
  •       nb > MODBUS_MAX_WRITE_REGISTERS) {
         if (ctx->debug) {
             fprintf(stderr, "Illegal data address %0X in write_registers\n",
                     address + nb);
    

    @@ -873,7 +879,9 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
    int nb_write = (req[offset + 7] << 8) + req[offset + 8];

     if ((address + nb) > mb_mapping->nb_registers ||
    
  •        (address_write + nb_write) > mb_mapping->nb_registers) {
    
  •        (address_write + nb_write) > mb_mapping->nb_registers ||
    
  •       nb > MODBUS_MAX_RW_WRITE_REGISTERS ||
    
  •       nb_write > MODBUS_MAX_RW_WRITE_REGISTERS) {
         if (ctx->debug) {
             fprintf(stderr,
                     "Illegal data read address %0X or write address %0X write_and_read_registers\n",
    

    --
    1.7.4.1

@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Jan 28, 2012

Owner

Could you send me an updated patch by mail, please?

Owner

stephane commented Jan 28, 2012

Could you send me an updated patch by mail, please?

@ghost ghost assigned stephane Jan 28, 2012

@LetoThe2nd

This comment has been minimized.

Show comment
Hide comment
@LetoThe2nd

LetoThe2nd Jan 30, 2012

I'd love to, but as I never received any answer or suggestions to my
questions (see #26 (comment)),
I have not worked on the patch any more. As long as I don't know what
you'd want the patch to look like, I am obviously unable to send one.

2012/1/28 Stéphane Raimbault
reply@reply.github.com:

Could you send me an updated patch by mail, please?


Reply to this email directly or view it on GitHub:
#25 (comment)

I'd love to, but as I never received any answer or suggestions to my
questions (see #26 (comment)),
I have not worked on the patch any more. As long as I don't know what
you'd want the patch to look like, I am obviously unable to send one.

2012/1/28 Stéphane Raimbault
reply@reply.github.com:

Could you send me an updated patch by mail, please?


Reply to this email directly or view it on GitHub:
#25 (comment)

@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Oct 6, 2013

Owner

Thank you

Owner

stephane commented Oct 6, 2013

Thank you

mk8 added a commit to mk8/libmodbus that referenced this issue Jan 29, 2014

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment