Failure to decode ethernet frame with trailing pad (Invalid decoding of Fin-Ack to contain payload)

[msantos]

Reported by @josemic in msantos/epcap#11.

The ethernet frame is padded with 6 trailing bytes:

<<0,69,0,0,40,79,99,64,0,56,6,192,67,91,215,100,
139,192,168,178,30,0,80,225,205,214,149,255,
217,25,137,180,211,80,17,0,46,245,145,0,0,0,0,
0,0,0,0>>

[ates]

6 bytes were added because user data can't be less than 46 bytes. In this case the size of IP is 20 bytes and TCP is 20 bytes too, so that's why 6 bytes were added to the end of frame.

[msantos]

That makes sense. So I guess it is up to the user to calculate the length and truncate the frame.

I can add a section about ethernet frames to the README and give an example of calculating the length:

Padding in Ethernet Frames

The minimum size of an ethernet frame is 46 bytes. For example, an ethernet frame containing a TCP/IP packet composed of a 20 byte IP header and 20 byte TCP header and payload will be padded by 6 bytes. To calculate the actual length of the packet, use the length and header length values in the IP header and the offset value in the TCP header:

[#ether{}, #ipv4{len = Len, hl = HL}, #tcp{off = Off}, Payload] = pkt:decapsulate(Frame),
Size = Len - (HL * 4) - (Off * 4),
<<Payload:Size/bytes>>.

@josemic, does that sound ok?

[ates]

I like that 👍 Just add the comment to README would be enough.

[josemic]

Ok. How about IP6?

I am currently using the example from sniffer.erl:

    [Ether, IP, Hdr, Payload] = decode(pkt:link_type(DLT), Packet),

    {Saddr, Daddr, Proto} = case IP of
        #ipv4{saddr = S, daddr = D, p = P} ->
            {S,D,P};

        #ipv6{saddr = S, daddr = D, next = P} ->
            {S,D,P}
    end

...
decode(ether, Packet) ->
    pkt:decapsulate({ether, Packet});
decode(DLT, Packet) ->
    % Add a fake ethernet header
    [_Linktype, IP, Hdr, Payload] = pkt:decapsulate({DLT, Packet}),
    [#ether{}, IP, Hdr, Payload].

[josemic]

@msantos: You suggestion seems to work with ip4. But use more specific variable name like Len, e.g. "PayloadLen".

[msantos]

About the variable name, agreed, I will try to make that clearer. Updating sniff is a good idea too.

IPv6 packets are so large, I wonder if this will be a problem there :) I'll see if I can come with an example.

Thanks for the feedback!

[msantos]

Added to README.

[josemic]

I think the padding could be calculated by pkt, not by the upper layer.

To make 1-to-1 decoding between binary and records possible, I suggest to extend the record with a pad value:

codec(#tcp{
        sport = SPort, dport = DPort,
        seqno = SeqNo,
        ackno = AckNo,
        off = Off, cwr = CWR, ece = ECE, urg = URG, ack = ACK,
        psh = PSH, rst = RST, syn = SYN, fin = FIN, win = Win,
        sum = Sum, urp = Urp,
        opt = Opt, pad = Pad
    }) ->
     <<SPort:16, DPort:16,
      SeqNo:32,
      AckNo:32,
      Off:4, 0:4, CWR:1, ECE:1, URG:1, ACK:1,
      PSH:1, RST:1, SYN:1, FIN:1, Win:16,
      Sum:16, Urp:16,
      Opt/binary, 0:Pad>>.

codec(
    <<SPort:16, DPort:16,
      SeqNo:32,
      AckNo:32,
      Off:4, 0:4, CWR:1, ECE:1, URG:1, ACK:1,
      PSH:1, RST:1, SYN:1, FIN:1, Win:16,
      Sum:16, Urp:16,
      Rest/binary>>
) when Off >= 5 ->
     Pad = XXXX,
    OptLen = (Off - 5) * 4,
    <<Opt:OptLen/binary, Payload/binary>> = Rest,
    {#tcp{
        sport = SPort, dport = DPort,
        seqno = SeqNo,
        ackno = AckNo,
        off = Off, cwr = CWR, ece = ECE, urg = URG, ack = ACK,
        psh = PSH, rst = RST, syn = SYN, fin = FIN, win = Win,
        sum = Sum, urp = Urp,
        opt = Opt, pad = Pad
    }, Payload};

See also section of scapy between decoding and interpretation:
http://www.secdev.org/projects/scapy/

Any comments?

[msantos]

I've been thinking pkt should follow the C headers, i.e., parse fixed length packet headers into a record and provide a function to parse variable length headers/payload. For example, this was why the TCP options weren't separated from the payload originally.

The representation of variable length headers is an issue as well for the ICMP6 and IGMP protocols. ICMP6 also has a number of different types which I've shoved into one record type. Probably these should be broken out into individual records (same with ICMP).

About decoding vs interpretation, not sure what you are getting at.

[josemic]

I see.

If you have a model of the packet (the Erlang representation) it should match the real packet, otherwise:

• you can not earmark packets based on that original type.
• generate packets based on a certain (maybe bad) type.

This applies, when you use it to create or analyse invalid traffic, or want to replay the traffic of a certain type.
It should not matter, if you are interested just in the payload, as the C-Code does.

[msantos]

I agree with what you are saying (the erlang representation should match the packet) but the pad in this case is a convenience when specifying TCP options. If you want to specify the pad (for example, setting the pad to <<1:1, 0:1, 1:1>> instead of <<0:3>>), specify it in the option field. The pad will have to be aligned along an octet.

Now the issue is whether the pad should be allowed to not fall along the byte boundary stated in the TCP off field, either too short (the payload becomes part of the header) or too large (the pad extends into the header).

Again, it depends if you consider the pad to be part of the input or the result of a function run over the input.

Right now padding is a hard coded function. But it could be changed to an arbitrary function that is called when creating the TCP options:

% Opt = tcp:pad(Off, <<1,2,3,4>>), pkt:tcp(#tcp{opt = Opt}).
pad(Off, Opt) ->
    Pad = ((Off - 5) * 4 - byte_size(Opt)) * 8,
    <<0:Pad>>.

Similarly for when the packet is converted from binary to a record.

Regarding analysing invalid traffic: in general, I agree here too to an extent. For example, I think this should crash:

pkt:tcp(<<1:8>>) 

But I think a packet that sets the "evil bit" (http://tools.ietf.org/html/rfc3514) shouldn't crash.

[msantos]

BTW, thought about this and I agree the pad should be in the record structure. To preserve the default behaviour (calculating the correct pad), the pad will be calculated if unset:

Pad = case Pad0 of
    undefined -> pad(...);
    _ -> Pad0;
end

[josemic]

Cool !!!

[ates]

Guys, i think this issue should be closed. No ?

[msantos]

Looks like I still need to make this change. Basically add a pad field to the tcp record and use if set. Otherwise, calculate the padding required.

[meox]

any news on this?