Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fix VD-1301 and VD-1302 vulnerabilities
This patch was contributed by Maor Vermucht and Or Peles from
VDOO Connected Trust.
  • Loading branch information
stephane committed Jul 29, 2019
1 parent 076992f commit 5ccdf5e
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/modbus.c
Expand Up @@ -839,9 +839,10 @@ int modbus_reply(modbus_t *ctx, const uint8_t *req,
break;
case MODBUS_FC_WRITE_MULTIPLE_COILS: {
int nb = (req[offset + 3] << 8) + req[offset + 4];
int nb_bits = req[offset + 5];
int mapping_address = address - mb_mapping->start_bits;

if (nb < 1 || MODBUS_MAX_WRITE_BITS < nb) {
if (nb < 1 || MODBUS_MAX_WRITE_BITS < nb || nb_bits * 8 < nb) {
/* 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. */
Expand Down Expand Up @@ -870,9 +871,10 @@ 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];
int nb_bytes = req[offset + 5];
int mapping_address = address - mb_mapping->start_registers;

if (nb < 1 || MODBUS_MAX_WRITE_REGISTERS < nb) {
if (nb < 1 || MODBUS_MAX_WRITE_REGISTERS < nb || nb_bytes * 8 < nb) {

This comment has been minimized.

Copy link
@karlp

karlp Jul 31, 2019

Contributor

That doesn't look right at all. I see what the test is for, and how it works on the write multiple coils, to validate that the bytes/coils counts are consistent, but write multiple registers has a count of registers and a byte count, so it should be * 2, not * 8 here.

Honestly, it's not even a great check, for write multiple registers, it should just always be nb * 2 == nb_bytes, anything else is inconsistent fuzzing attacks

This comment has been minimized.

Copy link
@stephane

stephane Jul 31, 2019

Author Owner

OMG It's a fucking typo!

This comment has been minimized.

Copy link
@stephane

stephane Jul 31, 2019

Author Owner

Seriously I don't know how I could write that shit.

rsp_length = response_exception(
ctx, &sft, MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp, TRUE,
"Illegal number of values %d in write_registers (max %d)\n",
Expand Down

1 comment on commit 5ccdf5e

@dodsonmg
Copy link

@dodsonmg dodsonmg commented on 5ccdf5e Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can someone help me understand the vulnerability here?

This commit seems to ensure the number of bits or registers you say you're going to write (nb = (req[offset + 3] << 8) + (req[offset + 4])) matches the number of things you actually provided to write (nb_bits/bytes = req[offset + 5]). Presumably, the concern is that the function will read past the input buffer and start writing garbage into your coils or registers. That's bad, so I'm on board with the sanitisation check, but since there's nothing in the code restricting a client from writing to as many bits or registers as they want (provided they don't exceed mb_mapping->nb_bits or mb_mapping->nb_registers), I don't see how an attacker could actually have used this 'vulnerability' to do anything bad. It hardly seems to justify a CVSSv3 > 9...

The next part of the conditional checks that you don't exceed mb_mapping->nb_bits or mb_mapping->nb_registers, which seems sufficient to ensure you don't actually overwrite past the allocation for either bits or registers.

For example, in the MODBUS_FC_WRITE_MULTIPLE_COILS case:

libmodbus/src/modbus.c

Lines 853 to 854 in 5ccdf5e

} else if (mapping_address < 0 ||
(mapping_address + nb) > mb_mapping->nb_bits) {

While I agree it's bad to let the client write garbage into the coils or registers, given that the client is free to write into those coils or registers anyway, and is prevented from writing past the allocation already, I'm struggling to see why this is a big deal from a vulnerability standpoint.

I just did some limited testing, and if you revert this commit (and the one that fixed the typo), but still implement the unit tests for VD-1301 and VD-1302 in the subsequent commit, that second part of the conditional seems to prevent anything bad from happening for both MODBUS_FC_WRITE_MULTIPLE_COILS and MODBUS_FC_WRITE_MULTIPLE_REGISTERS.

I assume I'm just missing some part of the significance, however, and would be glad to be put right.

Please sign in to comment.