StrFixedLenField with incomplete packet

[LRGH]

Brief description

Interactions between StrFixedLenField and raw packet cache cause some inconsistencies when some packet field is or is not modified. This should probably be fixed by a modification of StrFixedLenField but a choice needs to be made between two possible behaviours.

Scapy version

all (at least from 2.3.1 to 2.6.0rc1)

Python version

all

Operating system

all

Additional environment information

No response

How to reproduce

from scapy.packet import Packet
from scapy.fields import StrFixedLenField

class P(Packet):
    fields_desc = [
        StrFixedLenField('a', None, length=1),
        StrFixedLenField('b', None, length=1),
        ]

p = P(b'0')
s0 = p.build()
p.a = b'1'
s1 = p.build()
print(s0, s1)

Actual result

b'0' b'1\x00'

Expected result

Either b'0\x00' b'1\x00' (if StrFixedLenField shall always be of the given length) or b'0' b'1' (if incomplete StrFixedLenField are to be remembered as incomplete)

Related resources

No response

[gpotter2]

I would argue that using a StrFixedLenField while trying to dissect something smaller than the minimum size is an undefined behavior, so not really a bug.

If you have a LongField (8B) and try to dissect 2 bytes, Scapy discards the packet as "Raw". We currently don't do that for StrFixedLenField, and maybe we should, but I really don't believe that we should have the behavior you expected.

[LRGH]

I don't like undefined behaviors... I agree that the expected behaviour could be that the dissection fails, and that there could be a StrMaxLenField that would return b'0' b'1' with a semantics that the field gobbles up to length bytes.

[polybassa]

In my opinion, the Packet s0 is build as one would expect it in the current implementation. I think it’s not desired to magically add fields to a captured packet. This would eliminate the use case to capture malformed packets and replay them.

Maybe it would be a good idea, to add a „malformed Packet“ information to the Packet whenever the dissector failed or the length doesn’t match the expected one. Similar as wireshark would do it.

With that in mind, I could see even a third possible behaviour after setting the field „a“ in your example, the packet could stay malformed, „a“ is changed to 1 and no field „b“ would be added.

[gpotter2]

In my opinion, the Packet s0 is build as one would expect it in the current implementation. I think it’s not desired to magically add fields to a captured packet.

I agree with that.

With that in mind, I could see even a third possible behaviour after setting the field „a“ in your example, the packet could stay malformed, „a“ is changed to 1 and no field „b“ would be added.

This would, in my opinion, add too much complexity. We would have to somehow remember what fields were parsed, etc. I also thinks it makes things more confusing that it should, as you end up having a 'hidden state' that wouldn't really be explicit to anyone.

I don't think that the comparison to wireshark makes much sense, they don't build packets.

Generally I'm also against adding a general case that shows "Invalid something" in a Scapy (except when you're absolutely sure of what you're doing), because at the end of the day there are so many protocols that we'll always have one wrong dissector. We might want to stay humble and say Scapy failed to parse that, rather than "it's invalid".

My current stance on this specific issue is that Scapy is working as intended.

[LRGH]

I agree that having a "malformed packet" information is likely to make many things more complex. However, I support the approach mentioned above If you have a LongField (8B) and try to dissect 2 bytes, Scapy discards the packet as "Raw" It does not seem to be complex to implement in StrFixedLenField, we could just add in getfieldthe following test:

        if len(s) < len_pkt:
            raise Scapy_Exception("remaining packet length %s too small for field %s of length %s" % (len(s), self.name, len_pkt))

And the behaviour that I expected would be for another type of field:

class StrMaxLenField(StrFixedLenField):
    def addfield(self, pkt, s, val):
        return s + self.i2m(pkt, val)