Invalid decoding of Window size

[josemic]

Here is the code I use to decode the packet:

        Header = epcap_port_lib:header(Hdr),
        {flags, Flags} = lists:keyfind(flags, 1, Header),
        Syn = lists:member(syn, Flags),
        Ack = lists:member(ack, Flags),
        Fin = lists:member(fin, Flags),
        Rst = lists:member(rst, Flags),
            {seq, Seqno} = lists:keyfind(seq, 1, Header),
            {ack, Ackno} = lists:keyfind(ack, 1, Header),
            {win, Win} = lists:keyfind(win, 1, Header),
            [Ether, IP, Hdr, Payload] = epcap_port_lib:decode(pkt:link_type(DLT), Packet),
            PayloadLength = byte_size(Payload),

The windows-size does not seem to be decoded correctly: {Ack=true,Syn=false,Fin=false,Rst=false,SeqNo=2200786594,AckNo=2223605307,Win=331}, {{"192.168.178.30",52024},{"173.194.113.155",80}}, 1, {1381,693845,846187}, 66, Payload=<<156,199,166,109,119,220,0,37,34,169,124,93,8,0,69, 0,0,52,150,193,64,0,64,6,17,222,192,168,178,30, 173,194,113,155,203,56,0,80,131,45,86,162,132,137, 134,59,128,16,1,75,80,38,0,0,1,1,8,10,0,97,251,10, 203,114,28,43>>, PayloadLen= 0},

Wireshark shows the Win-size as: 42368 instead of the window size 31 shown by Epcap. In wireshark screenshot below the packet is highlighted.

Here is a screenshot from both:

PS: I guess this applies to other packets too, but I have not checked it.

[ates]

pkt is correctly decodes the Window Size field in TCP packet. But actual value of Window Size is calculated in runtime as Window Size (331) * Scaling Factor (128) = 42368

The value of Scaling Factor should be present in some previous SYN packet as an option.

[msantos]

RFC 1323 discusses window scaling:

http://www.ietf.org/rfc/rfc1323.txt

[josemic]

Thanks. I was not aware of that when reading RFC 793.

RFC 1323 on Scaling factor:

      TCP Window Scale Option (WSopt):

         Kind: 3 Length: 3 bytes

                +---------+---------+---------+
                | Kind=3  |Length=3 |shift.cnt|
                +---------+---------+---------+

Here the msanto/pkt code:

    <<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 ->
    {Opt, Payload} = options(Off, 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
    }, Payload};
tcp(#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 = ((Off - 5) * 4 - byte_size(Opt)) * 8,
    <<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>>.

options(Offset, Payload) ->
    N = (Offset-5)*4,
    <<Opt:N/binary, Payload1/binary>> = Payload,
    {Opt, Payload1}.

Payload1 consists of elements of the format: <<Kind:8, Length:8, PayloadKind/Length:binary>> , where the Scale factor matches: <<Kind=3:8, N=1:8, Scalefactor/N:binary>>.

Is there a smart way to decode this than looping the options fields for the TCP options for items like the Scale-factor in the application? Any chance to extent msanto/pkt to decode the scale factor?

[ates]

Added to my local repo the code to decode TCP options:

1> Data = <<203,56,0,80,131,45,69,72,0,0,0,0,160,2,57,8,20,142,0,0,2,4,5,180,4,2,8,10,0,
  97,211,229,0,0,0,0,1,3,3,7>>.

2> pkt:tcp(Data).
{#tcp{sport = 52024,dport = 80,seqno = 2200782152,ackno = 0,
      off = 10,cwr = 0,ece = 0,urg = 0,ack = 0,psh = 0,rst = 0,
      syn = 1,fin = 0,win = 14600,sum = 5262,urp = 0,
      opt = [{window_scale,[{shift_count,7},{multiplier,128}]},
             {tsopt,[{timestamp,6411237},{timestamp_echo_reply,0}]},
             {sack_permitted,true},
             {maximum_segment_size,1460}]},
 <<>>}

[ates]

@josemic, is it ok for you? If yes, I will implement decoding of all possible options and make a commit.

[ates]

Added decoding of TCP options to new branch (tcp_opts) in msantos/pkt

[josemic]

Ok. I'll test it and provide feedback.

[josemic]

What is the purpose of that fixed constant?:

• {multiplier, round(math:pow(2, 7))}

Likely you mean:

• {multiplier, round(math:pow(ShiftCount, 7))}

But using math provides a float and is slow. Please use integer only operations: such as bsl: http://erlang.org/pipermail/erlang-questions/2003-June/009193.html E.g.:

• {multiplier, (1 bsl ShiftCount)}

[ates]

My fault, yep, should be used shift count value. I wil fix this today

[ates]

Done, please test.

[josemic]

According to this document http://www.ietf.org/rfc/rfc1323.txt page 10, the scale factor shall be limited to 14. I think this should be handled by pkt library too, but it might be useful to set an error flag in case the bit has been cut off at the value 14.

I am not sure whether the multiplier value is useful, as the scaleshift can be directly applied to the window.

[ates]

Do you have ideas how to do that in the better way? Which error flag should be set? Should pkt library raise the exception in this case (badarg, function_clause)?

[msantos]

The change looks good to me!

Some comments:

• I think it should be up to the user to decide "valid" values. For example, if someone were writing a port scanner, they might want to pass in an invalid scale factor to fingerprint the behaviour of a system.
• there should be encode functions to match the decode funs, e.g., Bin = pkt:tcp(pkt:tcp(Bin))

Naming the keys in the option proplist is tricky. It looks like they follow the options names in the RFCs with spaces and dashes converted to underscores. As long as it is consistent, I think that works well.

[ates]

I agree with the first comment. I will update encode function for TCP. Also I will try to add some tests.

[ates]

@msantos what do you think about some refactoring of pkt project? I don't like the idea of having all the functions in one module. I believe what have separate modules per protocol would be more better, something like tcp_codec, sctp_codec, which has encode/decode functions.

What do you think about that?

[josemic]

@msantos: Ok. That makes sense too. Nevertheless it might make sense to put a comment into the code and refer to RFC 1323 to cut it off at 14 in the application.

To make it work with real traffic, I had to add the following change: decode_tcp_options(<<0:8, 0, Rest/binary>>, Acc) -> decode_tcp_options(Rest, Acc);

Additionally I believe we should add some change like the following to make it future proof: decode_tcp_options(<<_Any:8, Len:8, _Payload:Len/binary-unit:8, Rest/binary>>, Acc) -> decode_tcp_options(Rest, Acc).

Still I am a little bit concerned that a packet with invalid option field e.g. during payload transfer cashes the decoding of the whole packet. This could be used to bring the epcap-based IDS like eNose down or out of sync for this connection, while the real clients remain active.

[msantos]

@ates sure, I think that is a good idea if we can keep the top level functions in pkt (ether/1, tcp/1, ...). There aren't that many functions in pkt.erl, so we can maintain it by hand or generate the module if it grows.

[msantos]

@josemic agreed. The encode should also pass through binary options.

About the crashes, yes, that is a huge issue. There are many places in pkt though where an invalid packet will not match. For example, if someone sets the "evil" bit in a TCP header, tcp/1 will throw a function_clause exception.

The functions could be wrapped in a try/catch. Maybe pkt should have something like that?

decode(Bin) ->
    try decapsulate(Bin)
    catch
        error:_Error ->
            {error, einval}
   end;

decode({Fun, Data}) ->
    try Fun(Bin)
    catch
        error:_Error ->
             {error, einval}
    end.

It still means eNose will have to recover somehow. I suppose eNose could trigger by default on indecipherable packets.

[ates]

@josemic maybe, but in case if we receive not implemented options we should implement they. Right now, decode_tcp_options/2 function does not handle all the options, because I not found all of them in pcap file provided by you.

I believe it's not good idea to just skip unknown options, we should crash in that case, so usage of try-catch would be not bad.

[ates]

@josemic could you please send me the pcap file with packets which has options not decoded by pkt?

[josemic]

@ates: It's on the way. For now it is ok. But for an IDS it is not an option. I guess there it must be configurable. It might be rule dependent, whether specific invalid data is ignored or evaluated as good as possible.

[ates]

Add decoding of End of Option List TCP option: msantos/pkt@64f2ed028788f307225b104c07dae44678542a6a

[josemic]

Works. Please merge with master.

[ates]

@msantos if no objections, I will squash changes from tcp_opts branch into one commit and merge to master.

[msantos]

Still needs the encode clause:

1> P = <<224,105,149,59,163,24,0,22,182,181,62,198,8,0,69,0,0,54,2,108,64,
1>               0,53,6,172,243,173,192,82,195,192,168,213,54,0,80,143,166,75,154,
1>               212,181,116,33,53,92,128,24,0,126,60,199,0,0,1,1,8,10,92,104,96,
1>               16,22,69,237,136,137,0>>.
<<224,105,149,59,163,24,0,22,182,181,62,198,8,0,69,0,0,54,
  2,108,64,0,53,6,172,243,173,192,82,...>>
2> [#ether{}, #ipv4{}, #tcp{} = TCP, _] = pkt:decapsulate(P).                     
[#ether{dhost = <<224,105,149,59,163,24>>,
        shost = <<0,22,182,181,62,198>>,
        type = 2048,crc = 0},
 #ipv4{v = 4,hl = 5,tos = 0,len = 54,id = 620,df = 1,mf = 0,
       off = 0,ttl = 53,p = 6,sum = 44275,
       saddr = {173,192,82,195},
       daddr = {192,168,213,54},
       opt = <<>>},
 #tcp{sport = 80,dport = 36774,seqno = 1268438197,
      ackno = 1948333404,off = 8,cwr = 0,ece = 0,urg = 0,ack = 1,
      psh = 1,rst = 0,syn = 0,fin = 0,win = 126,sum = 15559,
      urp = 0,
      opt = [{tsopt,[{timestamp,1550344208},
                     {timestamp_echo_reply,373681544}]}]},
 <<137,0>>]
3> pkt:tcp(TCP).
** exception error: bad argument
     in function  byte_size/1
        called as byte_size([{tsopt,[{timestamp,1550344208},
                                     {timestamp_echo_reply,373681544}]}])
     in call from pkt:tcp/1 (src/pkt.erl, line 344)

It looks like we need to allow passing through options as a binary too. For example, if someone wants to use quickcheck to test a TCP/IP stack they would want to test TCP options with values from <<0...0>> to <<255...255>>. At the moment, they wouldn't have a way of expressing that. And if we allow binaries in an encode, we have to allow them in a decode:

#tcp{opt = Bin} = pkt:tcp(pkt:tcp(#tcp{opt = Bin})).

So I think we need to add the encode before we merge to master, since this will break functionality for some users. I can take a look at it this weekend and can begin adding some tests.

[josemic]

I have not tested with IP6 packets jet? Are there any IP6 specific TCP-options? I can switch my router to IP6 to test that. Should we do that before merging?

[josemic]

[_EtherIgnore, IP, Hdr, Payload] = pkt:decapsulate({pkt:link_type(DLT), Packet}),
case IP of
        #ipv4{len = Len1, hl = HL} = IP ->
                #tcp{off = Off} = Hdr,
                PayloadSize = Len1 - (HL * 4) - (Off * 4);
    #ipv6{} ->
        PayloadSize = byte_size(Payload)
end,

With the above workaround (hack) I have successfully tested ipv6 packets. No packet decoding failures occurred so far.

[msantos]

@josemic that is pretty much what you have to do. You can wrap it in a function:

[_EtherIgnore, IP, Hdr, Payload] = pkt:decapsulate({pkt:link_type(DLT), Packet}),
PayloadSize = payloadsize(IP, TCP, Payload),
<<Payload:PayloadSize/binary>>.

payloadsize(#ipv4{len = Len, hl = HL}, #tcp{off = Off}, _Payload) ->
    Len - (HL * 4) - (Off * 4);
payloadsize(#ipv6{}, #tcp{}, Payload) ->
    byte_size(Payload).

The minimum size of an IPv6 header is 40 bytes + 20 bytes TCP means you shouldn't see the frame padding.

The correct way would be something like:

[_EtherIgnore, IP, Hdr, Payload] = pkt:decapsulate({pkt:link_type(DLT), Packet}),
PayloadSize = payloadsize(IP, TCP),
<<Payload:Payloadsize/binary>>.

payloadsize(#ipv4{len = Len, hl = HL}, #tcp{off = Off}) ->
    Len - (HL * 4) - (Off * 4);
% jumbo packet
payloadsize(#ipv6{len = 0, next = Next}, #tcp{off = Off}) ->
    % XXX handle jumbo packet here
    0;
payloadsize(#ipv6{len = Len, next = ?IPPROTO_TCP}, #tcp{off = Off}) ->
    Len - (Off * 4);
% additional extension headeres
payloadsize(#ipv6{len = Len, next = Next}, #tcp{off = Off}) ->
    % XXX handle extension headers here
    0.

[msantos]

What do you think of keeping the options field as a binary and exporting a function to encode/decode it (tcp_options/1 or options/1)?

The fork of pkt in the flowforwarding linc switch removed some of the conversions of binaries to other types, for example, IP address to tuple. I am thinking of doing this too in the future:

• probably faster
• more consistent
• addresses some of the reliability issues in conversion that @josemic was concerned about
• allows arbitrary input

So instead of having the IPv4 saddr and daddr converted to a tuple automatically, pkt would provide helper functions:

addr(<<A,B,C,D>>) ->
    {A,B,C,D};
addr(<<A:16,B:16,C:16,D:16,E:16,F:16,G:16,H:16>>) ->
    {A,B,C,D,E,F,G,H};
addr({A,B,C,D}) ->
    <<A,B,C,D>>;
addr({A,B,C,D,E,F,G,H}) ->
    <<A:16,B:16,C:16,D:16,E:16,F:16,G:16,H:16>>.

[josemic]

@msantos : to message 1) :

Should likely be:

[_EtherIgnore, IP, TCP, PayloadPadded] = pkt:decapsulate({pkt:link_type(DLT), Packet}),
PayloadSize = payloadsize(IP, TCP),
Payload = <<PayloadPadded:PayloadSize/binary>>.

to message 2): I like your suggestions. I am looking for a similar option that check the package checksum. However I am not sure, whether it is better placed in pkt as erlang code or in epcap as C-code.

[ates]

@msantos I like idea of keeping the tcp options as a binary, if no objections I want to do that.

[ates]

Updated tcp_opts branch with adding tcp_options/1 functions. Please review and test.

[josemic]

@ates: You have removed the multiplier. That makes things simpler. Reviewed and tested. Works fine.

[msantos]

@josemic thanks for the correction! I will either add an example to the README or add a new function (payloadsize/1) to pkt.

About the checksum, there is a makesum/1 function:

-module(mksum).
-include_lib("pkt/include/pkt.hrl").

-export([test/0]).

test() ->
    Frame = <<224,105,149,59,163,24,0,22,182,181,62,198,8,0,69,0,0,54,2,108,64,
              0,53,6,172,243,173,192,82,195,192,168,213,54,0,80,143,166,75,154,
              212,181,116,33,53,92,128,24,0,126,60,199,0,0,1,1,8,10,92,104,96,
              16,22,69,237,136,137,0>>,

    [#ether{}, #ipv4{sum = IPSum} = IP, #tcp{sum = TCPSum} = TCP, Payload] = pkt:decapsulate(Frame),

    IPSum = pkt:makesum(IP#ipv4{sum = 0}),
    TCPSum = pkt:makesum([IP, TCP#tcp{sum = 0}, Payload]),
    {IPSum, TCPSum}.

Calling makesum/1 with the original header should return 0:

0 = pkt:makesum(IP),
0 = pkt:makesum([IP, TCP, Payload]).

But the makesum/1 function sets the sum record field of the header to 0 for the caller. I am going to push a change to remove that.

[msantos]

@ates looks great! Thanks!

[ates]

I'm happy :)

[ates]

Guys, what's next? Is this issue completed?

[msantos]

@ates sure go ahead and merge and close this issue.

@josemic if you come across other issues with pkt, please file under the pkt repo.

For reference, the other issues that were brought up:

• pkt: add caclulation of IPv4/IPv6 payload size to examples or as payloadsize/1
• pkt: do not set the header sum value to 0 in makesum/1
• pkt: add a "safe" decode that returns an error tuple rather than an exception
• pkt: break up the pkt module into codecs: pkt_tcp, pkt_icmp, etc
• pkt: add tests and documentation
• pkt: (major version change) move to use of binaries only

[ates]

I will merge and close this issue today.

[ates]

Merged.