A server must always allocates a buffer of max ADU length #241

Closed
stephane opened this Issue Feb 5, 2015 · 15 comments

Comments

Projects
None yet
3 participants
@stephane
Owner

stephane commented Feb 5, 2015

It's possible to create a Modbus RTU/TCP server with a too small reception buffer and to use it in
modbus_receive(ctx, query);.

A large request can cause a buffer overflow because the code expects
the query buffer to be equal or larger than the max ADU length of the backend (MODBUS_TCP_MAX_ADU_LENGTH, MODBUS_RTU_MAX_ADU_LENGTH).

The file unit-test-server.c provides a reference exemple and I hope nobody does or will do otherwise but if it possible to do it wrong, I'm sure someone will do it!

@stephane stephane added this to the v3.2.0 milestone Feb 5, 2015

@tnemeth

This comment has been minimized.

Show comment
Hide comment
@tnemeth

tnemeth Feb 5, 2015

When the allocated size for a reception buffer is too small (eg: change MODBUS_TCP_MAX_ADU_LENGTH to 3 at lines 62, 65 and 69 of unit-test-server.c), the server segfaults when a too long request is received regarding to the allocated buffer for its storage.

The problem lies in the function :

int _modbus_receive_msg(modbus_t *ctx, uint8_t *msg, msg_type_t msg_type)

where no size indication is given for msg. A call to

rc = ctx->backend->recv(ctx, msg + msg_length, length_to_read);

meaning that if length_to_read exceed the remaining size of msg + msg_lenght, a buffer overflow occurs and a segfault is generated.

tnemeth commented Feb 5, 2015

When the allocated size for a reception buffer is too small (eg: change MODBUS_TCP_MAX_ADU_LENGTH to 3 at lines 62, 65 and 69 of unit-test-server.c), the server segfaults when a too long request is received regarding to the allocated buffer for its storage.

The problem lies in the function :

int _modbus_receive_msg(modbus_t *ctx, uint8_t *msg, msg_type_t msg_type)

where no size indication is given for msg. A call to

rc = ctx->backend->recv(ctx, msg + msg_length, length_to_read);

meaning that if length_to_read exceed the remaining size of msg + msg_lenght, a buffer overflow occurs and a segfault is generated.

tnemeth pushed a commit to tnemeth/libmodbus that referenced this issue Feb 5, 2015

@mhei

This comment has been minimized.

Show comment
Hide comment
@mhei

mhei Feb 8, 2015

Contributor

In your suggested patch, you pass a third argument to modbus_receive. I'm wondering whether this is the best approach. I don't feel comfortable because you have to allocated the buffer outside of libmodbus and thus you have to know which backend is used within the specific context. I can image use cases where you create a context and the pass it around without having the information anymore whether it was RTU or TCP.

There are following possibilities:

  • libmodbus exports a define, e.g. MODBUS_MAX_ADU_LENGTH which is max(MODBUS_TCP_MAX_ADU_LENGTH, MODBUS_RTU_MAX_ADU_LENGTH), then users are expected to use this define
  • add a helper function which allocates a buffer depended of the passed context (and thus backend), then users are expected to pass this buffer
  • modbus_receive* could operate on a buffer which is allocated while creating the context and then return a pointer to this buffer and the actual length (I think always allocating a new buffer for each call could be to expensive)

What do you think?

Contributor

mhei commented Feb 8, 2015

In your suggested patch, you pass a third argument to modbus_receive. I'm wondering whether this is the best approach. I don't feel comfortable because you have to allocated the buffer outside of libmodbus and thus you have to know which backend is used within the specific context. I can image use cases where you create a context and the pass it around without having the information anymore whether it was RTU or TCP.

There are following possibilities:

  • libmodbus exports a define, e.g. MODBUS_MAX_ADU_LENGTH which is max(MODBUS_TCP_MAX_ADU_LENGTH, MODBUS_RTU_MAX_ADU_LENGTH), then users are expected to use this define
  • add a helper function which allocates a buffer depended of the passed context (and thus backend), then users are expected to pass this buffer
  • modbus_receive* could operate on a buffer which is allocated while creating the context and then return a pointer to this buffer and the actual length (I think always allocating a new buffer for each call could be to expensive)

What do you think?

@tnemeth

This comment has been minimized.

Show comment
Hide comment
@tnemeth

tnemeth Feb 9, 2015

This may not be the best approach for everyone :) I like to control the size of my buffers depending on my needs, that's why I added a 3rd argument. Moreover there was several different sizes used throughout the code. Stephane told me the patch was too intrusive so he was not going to accept it as-is.

From the 1st possibility, I would say that even if users are expected to use a fixed size, some of them won't even read the doc (too bad for them). This is the easiest solution.

The second solution may be a bit overkill, don't you think ? Way more intrusive (IMHO) than my posted solution :)

For the last possibility, I think just like you : that could be too expensive.

I thought of my solution as the best fit for the problem, your 1st possibility beeing the 2nd. I tend to think that users will encounter problems with it anyway, causing crash dumps. As I said a lot of people don't read the documentation. Another solution (that is NOT thread safe) is to use an internally allocated reception buffer, may be in the modbus_t structure...

tnemeth commented Feb 9, 2015

This may not be the best approach for everyone :) I like to control the size of my buffers depending on my needs, that's why I added a 3rd argument. Moreover there was several different sizes used throughout the code. Stephane told me the patch was too intrusive so he was not going to accept it as-is.

From the 1st possibility, I would say that even if users are expected to use a fixed size, some of them won't even read the doc (too bad for them). This is the easiest solution.

The second solution may be a bit overkill, don't you think ? Way more intrusive (IMHO) than my posted solution :)

For the last possibility, I think just like you : that could be too expensive.

I thought of my solution as the best fit for the problem, your 1st possibility beeing the 2nd. I tend to think that users will encounter problems with it anyway, causing crash dumps. As I said a lot of people don't read the documentation. Another solution (that is NOT thread safe) is to use an internally allocated reception buffer, may be in the modbus_t structure...

@mhei

This comment has been minimized.

Show comment
Hide comment
@mhei

mhei Feb 11, 2015

Contributor

Hehe, having control over the buffers is always a good idea (tm) :-) And I also understand that users might forget to RTFM. But for the brave, I still recommend to add the proposed define.
Let's see what @stephane thinks...

Contributor

mhei commented Feb 11, 2015

Hehe, having control over the buffers is always a good idea (tm) :-) And I also understand that users might forget to RTFM. But for the brave, I still recommend to add the proposed define.
Let's see what @stephane thinks...

@mhei

This comment has been minimized.

Show comment
Hide comment
@mhei

mhei Feb 12, 2015

Contributor

Sorry, I have another question: when receiving a packet and buffer is too small, what happens with the remaining bytes on the line? Should be discarded, right? Did not look at the code in detail so far, but wondering whether this is ensured...

Contributor

mhei commented Feb 12, 2015

Sorry, I have another question: when receiving a packet and buffer is too small, what happens with the remaining bytes on the line? Should be discarded, right? Did not look at the code in detail so far, but wondering whether this is ensured...

@tnemeth

This comment has been minimized.

Show comment
Hide comment
@tnemeth

tnemeth Feb 12, 2015

Le Wed, Feb 11, 2015 at 09:49:48PM -0800, Michael Heimpold a tapot� :

Sorry, I have another question: when receiving a packet and buffer is too
small, what happens with the remaining bytes on the line?

Ahem...

Should be discarded, right?

Indeed. Should be.

Did not look at the code in detail so far, but wondering whether this is
ensured...

Unfortunately not.

Thomas.

tnemeth commented Feb 12, 2015

Le Wed, Feb 11, 2015 at 09:49:48PM -0800, Michael Heimpold a tapot� :

Sorry, I have another question: when receiving a packet and buffer is too
small, what happens with the remaining bytes on the line?

Ahem...

Should be discarded, right?

Indeed. Should be.

Did not look at the code in detail so far, but wondering whether this is
ensured...

Unfortunately not.

Thomas.

@stephane stephane modified the milestones: v3.4.0, v3.2.0 Feb 13, 2015

@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Feb 13, 2015

Owner

@tnemeth I don't think your patch really improves the situation (because nothing prevents the user to use

rc = modbus_receive_confirmation(ctx, rsp, MODBUS_RTU_MAX_ADU_LENGTH);

when the backend is TCP (or another inappropriate value).

As said by @mhei, I think we could just provides MODBUS_MAX_ADU_LENGTH (modbus.h) and to add more information in the documentation of modbus_receive_confirmation to explain how to allocate the buffer.

On IRC, I talked about providing a helper but I don't think it's better than good documentation and I always tend to prefer the KISS way to do thing.

Owner

stephane commented Feb 13, 2015

@tnemeth I don't think your patch really improves the situation (because nothing prevents the user to use

rc = modbus_receive_confirmation(ctx, rsp, MODBUS_RTU_MAX_ADU_LENGTH);

when the backend is TCP (or another inappropriate value).

As said by @mhei, I think we could just provides MODBUS_MAX_ADU_LENGTH (modbus.h) and to add more information in the documentation of modbus_receive_confirmation to explain how to allocate the buffer.

On IRC, I talked about providing a helper but I don't think it's better than good documentation and I always tend to prefer the KISS way to do thing.

@stephane

This comment has been minimized.

Show comment
Hide comment

@stephane stephane added the needinfo label Feb 18, 2015

@tnemeth

This comment has been minimized.

Show comment
Hide comment
@tnemeth

tnemeth Feb 18, 2015

Le Wed, Feb 18, 2015 at 04:44:24AM -0800, St�phane Raimbault a tapot� :

What do you think about?
https://github.com/stephane/libmodbus/tree/max-adu-241

Proper documentation and exports are fine. I would just remove the
"the" in : "If you want to write code compatible with _the_ both,"
(line 24 in SYNOPSYS) ;)

However comparing master to the max-adu-241 branch gives only that. No
enforcing in the reception function and no garbage dumps if (eventually
forged) received request overflows the reception buffer in order for the
next reception to start with a fresh request.

tnemeth commented Feb 18, 2015

Le Wed, Feb 18, 2015 at 04:44:24AM -0800, St�phane Raimbault a tapot� :

What do you think about?
https://github.com/stephane/libmodbus/tree/max-adu-241

Proper documentation and exports are fine. I would just remove the
"the" in : "If you want to write code compatible with _the_ both,"
(line 24 in SYNOPSYS) ;)

However comparing master to the max-adu-241 branch gives only that. No
enforcing in the reception function and no garbage dumps if (eventually
forged) received request overflows the reception buffer in order for the
next reception to start with a fresh request.
@mhei

This comment has been minimized.

Show comment
Hide comment
@mhei

mhei Feb 18, 2015

Contributor

The define and the improved documentation is fine and definitely a step in the right direction.

As @tnemeth already mentioned, me too, I think libmodbus should tidy up in case there are more bytes to retrieve. At minimum, we could mention this case in the function description.

And while writing, I think the best solution is to hide the whole reception from libmodbus users as suggested in my 3rd point above:

  • depending on the context a statically allocated buffer is used
  • user can obtain a pointer to this buffer
  • receive function reads the whole message
  • receive function returns actual message length to caller

I'll see whether I could hack a RFC implementation in the next days...

Contributor

mhei commented Feb 18, 2015

The define and the improved documentation is fine and definitely a step in the right direction.

As @tnemeth already mentioned, me too, I think libmodbus should tidy up in case there are more bytes to retrieve. At minimum, we could mention this case in the function description.

And while writing, I think the best solution is to hide the whole reception from libmodbus users as suggested in my 3rd point above:

  • depending on the context a statically allocated buffer is used
  • user can obtain a pointer to this buffer
  • receive function reads the whole message
  • receive function returns actual message length to caller

I'll see whether I could hack a RFC implementation in the next days...

@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Feb 19, 2015

Owner

@tnemeth typo fixed. Thank you. Branch merged (it's just a first step).

I wrote this function to play with the libmodbus code (and documentation says for "debugging purpose") but yes, we can improve it if you have nice ideas (I mean clean and small).

Owner

stephane commented Feb 19, 2015

@tnemeth typo fixed. Thank you. Branch merged (it's just a first step).

I wrote this function to play with the libmodbus code (and documentation says for "debugging purpose") but yes, we can improve it if you have nice ideas (I mean clean and small).

@tnemeth

This comment has been minimized.

Show comment
Hide comment
@tnemeth

tnemeth Feb 19, 2015

Le Thu, Feb 19, 2015 at 01:34:09AM -0800, St�phane Raimbault a tapot� :

I wrote this function to play with the libmodbus code (and documentation says
for "debugging purpose") but yes, we can improve it if you have nice ideas (I
mean clean and small).

IHMO, the most efficient way to enforce the buffer size and throw junk
bytes is to have a local (to the reception function) buffer that stores
what is received and add received data to the returned buffer, continuing
reception until all received bytes have been either put into the returned
buffer or kept in the local buffer as junk.

As for the returned buffer, I join myself to the solution from @mhei which
is to allocate the reception buffer inside the context structure and use
that buffer as return value (or let the user know that the buffer is to
be used as a reception result).

tnemeth commented Feb 19, 2015

Le Thu, Feb 19, 2015 at 01:34:09AM -0800, St�phane Raimbault a tapot� :

I wrote this function to play with the libmodbus code (and documentation says
for "debugging purpose") but yes, we can improve it if you have nice ideas (I
mean clean and small).

IHMO, the most efficient way to enforce the buffer size and throw junk
bytes is to have a local (to the reception function) buffer that stores
what is received and add received data to the returned buffer, continuing
reception until all received bytes have been either put into the returned
buffer or kept in the local buffer as junk.

As for the returned buffer, I join myself to the solution from @mhei which
is to allocate the reception buffer inside the context structure and use
that buffer as return value (or let the user know that the buffer is to
be used as a reception result).

mhei added a commit to mhei/libmodbus that referenced this issue Feb 20, 2015

Hide receive details from servers (refs #241)
This approach should only show a possible implementation.

One drawback is, that this new receive buffer is not used
thorughout all functions and thus we have a increased memory
footprint.

Also documentation is not updated yet.

Compile tested and unit-tested for tcp.

Signed-off-by: Michael Heimpold <mhei@heimpold.de>
@mhei

This comment has been minimized.

Show comment
Hide comment
@mhei

mhei Feb 20, 2015

Contributor

@tnemeth, @stephane: please spend a look, whether this is clean and small enough 😄

Contributor

mhei commented Feb 20, 2015

@tnemeth, @stephane: please spend a look, whether this is clean and small enough 😄

@tnemeth

This comment has been minimized.

Show comment
Hide comment
@tnemeth

tnemeth Feb 24, 2015

@mhei: you made no check in _modbus_receive_msg that protect against
a too big request (maybe forged), nor throw out unneeded bytes ?

In another related problem, as I see the protocol, there is little
safeguard (that can be implemented) against forged requests. A
request with a little proclaimed size and overflowing data will
certainly break the system. A request with a proclaimed size larger
than the provided data will fail on timeout if no other data comes
through the same socket/uart fast enough (which might also break the
system).

tnemeth commented Feb 24, 2015

@mhei: you made no check in _modbus_receive_msg that protect against
a too big request (maybe forged), nor throw out unneeded bytes ?

In another related problem, as I see the protocol, there is little
safeguard (that can be implemented) against forged requests. A
request with a little proclaimed size and overflowing data will
certainly break the system. A request with a proclaimed size larger
than the provided data will fail on timeout if no other data comes
through the same socket/uart fast enough (which might also break the
system).
@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane May 17, 2016

Owner

@tnemeth the recent versions contain protections against vicious requests (https://github.com/stephane/libmodbus/blame/master/tests/unit-test-client.c#L687) and I do my best to harden the library release after release (eg. #207)

@mhei sorry but I'm not willing to increase the memory footprint and to add so many checks just for this.

98c4aca has been merged.

Owner

stephane commented May 17, 2016

@tnemeth the recent versions contain protections against vicious requests (https://github.com/stephane/libmodbus/blame/master/tests/unit-test-client.c#L687) and I do my best to harden the library release after release (eg. #207)

@mhei sorry but I'm not willing to increase the memory footprint and to add so many checks just for this.

98c4aca has been merged.

@stephane stephane closed this May 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment