Skip to content

modbus: stop variable arrays over-reading into the next ADU #5067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ArockiaRajamanickam wants to merge 1 commit into
base: master
Choose a base branch
from
+46 −7
Open

modbus: stop variable arrays over-reading into the next ADU #5067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions scapy/contrib/modbus.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class ModbusPDU01ReadCoilsRequest(_ModbusPDUNoPayload):
XShortField("quantity", 0x0001)]


class ModbusPDU01ReadCoilsResponse(_ModbusPDUNoPayload):
class ModbusPDU01ReadCoilsResponse(Packet):
name = "Read Coils Response"
fields_desc = [XByteField("funcCode", 0x01),
BitFieldLenField("byteCount", None, 8,
Expand Down Expand Up @@ -102,7 +102,7 @@ class ModbusPDU03ReadHoldingRegistersResponse(Packet):
adjust=lambda pkt, x: x * 2),
FieldListField("registerVal", [0x0000],
ShortField("", 0x0000),
count_from=lambda pkt: pkt.byteCount,
count_from=lambda pkt: pkt.byteCount // 2,
max_count=123)]


Expand All @@ -127,7 +127,7 @@ class ModbusPDU04ReadInputRegistersResponse(Packet):
adjust=lambda pkt, x: x * 2),
FieldListField("registerVal", [0x0000],
ShortField("", 0x0000),
count_from=lambda pkt: pkt.byteCount)]
count_from=lambda pkt: pkt.byteCount // 2)]


class ModbusPDU04ReadInputRegistersError(Packet):
Expand Down Expand Up @@ -313,7 +313,7 @@ class ModbusPDU10WriteMultipleRegistersRequest(Packet):
adjust=lambda pkt, x: x * 2),
FieldListField("outputsValue", [0x0000],
XShortField("", 0x0000),
count_from=lambda pkt: pkt.byteCount)]
count_from=lambda pkt: pkt.byteCount // 2)]


class ModbusPDU10WriteMultipleRegistersResponse(Packet):
Expand Down Expand Up @@ -518,7 +518,7 @@ class ModbusPDU17ReadWriteMultipleRegistersRequest(Packet):
adjust=lambda pkt, x: x * 2),
FieldListField("writeRegistersValue", [0x0000],
XShortField("", 0x0000),
count_from=lambda pkt: pkt.byteCount)]
count_from=lambda pkt: pkt.byteCount // 2)]


class ModbusPDU17ReadWriteMultipleRegistersResponse(Packet):
Expand All @@ -529,7 +529,7 @@ class ModbusPDU17ReadWriteMultipleRegistersResponse(Packet):
adjust=lambda pkt, x: x * 2),
FieldListField("registerVal", [0x0000],
ShortField("", 0x0000),
count_from=lambda pkt: pkt.byteCount)]
count_from=lambda pkt: pkt.byteCount // 2)]


class ModbusPDU17ReadWriteMultipleRegistersError(Packet):
Expand All @@ -552,7 +552,7 @@ class ModbusPDU18ReadFIFOQueueResponse(Packet):
adjust=lambda pkt, p: p * 2 + 2),
BitFieldLenField("FIFOCount", None, 16, count_of="FIFOVal"),
FieldListField("FIFOVal", [], ShortField("", 0x0000),
count_from=lambda pkt: pkt.byteCount)]
count_from=lambda pkt: pkt.FIFOCount)]


class ModbusPDU18ReadFIFOQueueError(Packet):
Expand Down
39 changes: 39 additions & 0 deletions test/contrib/modbus.uts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,3 +541,42 @@ pkt = ModbusPDUUserDefinedFunctionCodeRequest(b'M\x00\x05\x00\n')
pkt = next(iter(pkt))
assert pkt.mb_payload == b'\x00\x05\x00\n'


+ Test back-to-back ADUs in one buffer

= Register arrays are not over-read when a second ADU follows
# GH4096: byteCount is a byte count, but count_from expects an element count,
# so register arrays consumed twice as many items as they should and swallowed
# the header of the following ADU.
adu = raw(ModbusADUResponse(transId=1, unitId=0xff) / ModbusPDU04ReadInputRegistersResponse(registerVal=[0x1111, 0x2222, 0x3333]))
pkt = ModbusADUResponse(adu + adu)
assert pkt[ModbusPDU04ReadInputRegistersResponse].registerVal == [0x1111, 0x2222, 0x3333]
assert raw(pkt[ModbusPDU04ReadInputRegistersResponse].payload) == adu

= Read Coils response keeps the ADU that follows it
# GH4096: ModbusPDU01ReadCoilsResponse derived from _ModbusPDUNoPayload, whose
# extract_padding discards everything after the PDU, so the second ADU was lost.
adu = raw(ModbusADUResponse(transId=1, unitId=0xff) / ModbusPDU01ReadCoilsResponse(coilStatus=[0x11, 0x22, 0x33]))
pkt = ModbusADUResponse(adu + adu)
assert pkt[ModbusPDU01ReadCoilsResponse].coilStatus == [0x11, 0x22, 0x33]
assert raw(pkt[ModbusPDU01ReadCoilsResponse].payload) == adu

= Every byteCount-driven array round-trips across concatenated ADUs
# GH4096: covers each PDU whose variable array is sized from byteCount.
cases = [
(ModbusADUResponse, ModbusPDU01ReadCoilsResponse, "coilStatus", [0x11, 0x22, 0x33]),
(ModbusADUResponse, ModbusPDU02ReadDiscreteInputsResponse, "inputStatus", [0x11, 0x22, 0x33]),
(ModbusADUResponse, ModbusPDU03ReadHoldingRegistersResponse, "registerVal", [0x1111, 0x2222, 0x3333]),
(ModbusADUResponse, ModbusPDU04ReadInputRegistersResponse, "registerVal", [0x1111, 0x2222, 0x3333]),
(ModbusADURequest, ModbusPDU0FWriteMultipleCoilsRequest, "outputsValue", [0x11, 0x22, 0x33]),
(ModbusADURequest, ModbusPDU10WriteMultipleRegistersRequest, "outputsValue", [0x1111, 0x2222, 0x3333]),
(ModbusADURequest, ModbusPDU17ReadWriteMultipleRegistersRequest, "writeRegistersValue", [0x1111, 0x2222, 0x3333]),
(ModbusADUResponse, ModbusPDU17ReadWriteMultipleRegistersResponse, "registerVal", [0x1111, 0x2222, 0x3333]),
(ModbusADUResponse, ModbusPDU18ReadFIFOQueueResponse, "FIFOVal", [0x1111, 0x2222, 0x3333]),
]
for adu_cls, pdu_cls, field, values in cases:
adu = raw(adu_cls(transId=1, unitId=0xff) / pdu_cls(**{field: values}))
assert list(getattr(adu_cls(adu)[pdu_cls], field)) == values, pdu_cls.__name__
pkt = adu_cls(adu + adu)
assert list(getattr(pkt[pdu_cls], field)) == values, pdu_cls.__name__
assert raw(pkt[pdu_cls].payload) == adu, pdu_cls.__name__