Link-layer validations

[pesco]

Opening a proper issue to track this topic.

The main issue is that the link layer must consume invalid data so it can continue. Therefore, unlike the transport and application layer parsers, dnp3_p_link_frame always yields a DNP3_Frame structure if start bytes and header crc are valid.

A separate function dnp3_link_validate_frame is exported and used by the dnp3_dissector to determine whether to process or ignore the frame.

[pesco]

Cf. comments on commit 470c196900a3c44f6174774db63013cf04303321, including the following pathological case of overlapping frames:

05 64 03 F2 05 64 05 64 A9 8E 05 64 9E 05 70 DE
00007891000000000000000000000000 17A7
0000B677000000000000000000000000 E5A0
0000955D000000000000000000000000 6E8E
0000FCD6000000000000000000000000 F5CB
00005E71000000000000000000000000 E48F
00001EFE000000000000000000000000 F50E
0000CED7000000000000000000000000 B3A0
00005EE8000000000000000000000000 6EA5
00007FD1000000000000000000000000 1CD1
00005567000000000000000000FFFF00 1825
00000000 FFFF

L> primary frame from master 25605 to 25605: TEST_LINK_STATES
L> primary frame from outstation 25605 to 36521: UNCONFIRMED_USER_DATA
L> secondary frame from master 1438 to 25605: function 14 (obsolete): 00 00 78 91 00 00 00 00 00 00 00 00 00 00 00 00 00 00 B6 77 00 00 00 00 00 00 00 00 00 00 00 00 00 00 95 5D 00 00 00 00 00 00 00 00 00 00 00 00 00 00 FC D6 00 00 00 00 00 00 00 00 00 00 00 00 00 00 5E 71 00 00 00 00 00 00 00 00 00 00 00 00 00 00 1E FE 00 00 00 00 00 00 00 00 00 00 00 00 00 00 CE D7 00 00 00 00 00 00 00 00 00 00 00 00 00 00 5E E8 00 00 00 00 00 00 00 00 00 00 00 00 00 00 7F D1 00 00 00 00 00 00 00 00 00 00 00 00 00 00 55 67 00 00 00 00 00 00 00 00 00 FF FF 00 00 00 00 00
L> secondary frame from outstation 0 to 56944: function 5 (reserved): 00 00 00 00 00 00 00 00 00 00 00 00 17 A7 00 00 00 00 00 00 00 00 00 00 00 00 00 00 E5 A0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 6E 8E 00 00 00 00 00 00 00 00 00 00 00 00 00 00 F5 CB 00 00 00 00 00 00 00 00 00 00 00 00 00 00 E4 8F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 F5 0E 00 00 00 00 00 00 00 00 00 00 00 00 00 00 B3 A0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 6E A5 00 00 00 00 00 00 00 00 00 00 00 00 00 00 1C D1 00 00 00 00 00 00 00 00 00 00 00

the first packet claims a length of 3. Depending on the decoder implementation, any of the other three could be hit next:

1. Decoder consumes the full first frame header. The fourth frame is seen next.
2. Decoder consumes the first start marker and scans for the next. It finds the second frame.
3. Decoder consumes start, length, and 3 (length) more bytes. It finds the third frame.

There could be a fourth case where the decoder consumes start, len, len bytes, and a CRC.

Anyway, the point was to demonstrate the ambiguity that we decided to resolve as case 1.

as long as the start bytes and CRCs check out, the frame is technically a frame and should be skipped as a whole

[pesco]

It turns out AN2013-004b almost clears the above up. It states that "discarded" (i.e. invalid) data should be consumed. Unfortunately it says not how much data that is.

Table 1 details conditions that shall cause incoming data to be ignored (discarded) by the Data Link Layer. After data has been discarded, the monitoring of the incoming data to locate a new start-of-message (0x05 0x64) shall recommence with the data received immediately after the discarded data.

[pesco]

Open question: In secondary frames, FCB should be 0 - is a secondary frame with FCB=1 invalid or is the bit ignored?

[jadamcrain]

Some sections of the spec:

The FCB and FCV bit fields function together to maintain synchronization for Confirmed User Data
services. Typically devices should use Unconfirmed User Data services. For more information, see 10.2.
The FCB is only valid in Data Link Layer request messages sent from the Primary Station to the Secondary

⎯ FCV = 1 indicates the state of the FCB bit is valid and that the state of the FCB bit in the received
message shall be checked against its expected state.
⎯ FCV = 0 indicates the state of the FCB bit is ignored.

To me all of this means that FCB should be zero UNLESS it is PRI_TO_SEC function w/ FCV=1, in which case it could be 0 or 1.

[pesco]

I agree, it should be 0. But I'm not sure that PRM=0,FCB=1 should be discarded. For instance, the table in AN2013-004b that I'm looking at (page 5) does not list it.

[pesco]

Apart from the above, 5e2cf2bc5f15b3c91ed5b3f87b66af94c725e828 implements the validations and adds an link_invalid callback to the dissector.

[jadamcrain]

Cool. I'll try this out in the proxy later this afternoon.

AN2013-004b isn't normative. It was a reaction to the 30 or so ICS-CERT advisories that we reported. I'm sure that it has inconsistencies or there are nuances it fails to capture.

In the grand scheme of things, how aggressively the proxy validates the FCB bit is fairly immaterial. It's unlikely to trigger any bugs in real word implementations, as they too will simply ignore the bit if it isn't relevant.

[pesco]

Adam Crain notifications@github.com writes:

AN2013-004b isn't normative. It was a reaction to the 30 or so ICS-CERT advisories that we reported. I'm sure that it has inconsistencies or nuances it doesn't capture.

Oh? I thought it was. At least in the sense that it clears up things that arguably should be normative.

Anyway...

In the grand scheme of things, how aggressively the proxy validates the FCB bit is fairly immaterial. It's unlikely to trigger any bugs in real word implementations, as they too will simply ignore the bit if it isn't relevant.

That's what I think as well. So I'd say let the parser ignore the bit but when we eventually synthesize our own output we will always produce a 0 there.