Suppose bug with fragment structure decoding

[pavel-odintsov]

Hello, Thomas!

I've following BGP message:

ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 32 02 00 00 00 1b 80 0e 09 00 01 85 00 00 03 0c 80 0c 40 01 01 02 c0 10 08 80 06 00 00 00 00 00 00

It has single flow spec element with fragmentation options first-fragment and last-fragment enabled.

tshark could decode it correctly:

Border Gateway Protocol - UPDATE Message
    Marker: ffffffffffffffffffffffffffffffff
    Length: 50
    Type: UPDATE Message (2)
    Withdrawn Routes Length: 0
    Total Path Attribute Length: 27
    Path attributes
        Path Attribute - MP_REACH_NLRI
            Flags: 0x80, Optional: Optional, Non-transitive, Complete
                1... .... = Optional: Optional
                .0.. .... = Transitive: Non-transitive
                ..0. .... = Partial: Complete
                ...0 .... = Length: Regular length
            Type Code: MP_REACH_NLRI (14)
            Length: 9
            Address family identifier (AFI): IPv4 (1)
            Subsequent address family identifier (SAFI): Flow Spec Filter (133)
            Next hop network address (0 bytes)
            Number of Subnetwork points of attachment (SNPA): 0
            Network layer reachability information (4 bytes)
                FLOW_SPEC_NLRI (4 bytes)
                    NRLI length: 3
                    Filter: IP fragment filter ( FFLF)
                        Filter type: IP fragment filter (12)
                        Operator flags: 0x80, end-of-list, Value length: 1 byte: 1 <<
                            1... .... = end-of-list: Set
                            .0.. .... = and: Not set
                            ..00 .... = Value length: 1 byte: 1 << (0)
                            .... 0... = Reserved: Not set
                            .... .0.. = Reserved: Not set
                            .... ..0. = logical negation: Not set
                            .... ...0 = Match bit: Not set
                        Fragment Flag: 0x0c, Last fragment, First fragment
                            .... 1... = Last fragment: Set
                            .... .1.. = First fragment: Set
                            .... ..0. = Is a fragment: Not set
                            .... ...0 = Don't fragment: Not set
        Path Attribute - ORIGIN: INCOMPLETE
            Flags: 0x40, Transitive: Well-known, Transitive, Complete
                0... .... = Optional: Well-known
                .1.. .... = Transitive: Transitive
                ..0. .... = Partial: Complete
                ...0 .... = Length: Regular length
            Type Code: ORIGIN (1)
            Length: 1
            Origin: INCOMPLETE (2)
        Path Attribute - EXTENDED_COMMUNITIES
            Flags: 0xc0, Optional, Transitive: Optional, Transitive, Complete
                1... .... = Optional: Optional
                .1.. .... = Transitive: Transitive
                ..0. .... = Partial: Complete
                ...0 .... = Length: Regular length
            Type Code: EXTENDED_COMMUNITIES (16)
            Length: 8
            Carried extended communities: (1 community)
                Community Transitive Experimental Flow spec traffic-rate
                    Community type high: Transitive Experimental (0x80)
                    Subtype Experimental: Flow spec traffic-rate (0x06)
                    Community AS: 0
                    Rate shaper: 0

But ExaBGP could not:

 update json { "exabgp": "3.5.0", "host" : "Ubuntu-1404-trusty-64-minimal", "counter": 1, "type": "update", "neighbor": { "address": { "local": "127.0.0.1", "peer": "127.0.0.1" }, "asn": { "local": "1", "peer": "1" }, "direction": "in", "message": { "update": { "attribute": { "origin": "incomplete", "extended-community": [ 9225060886715039744 ] }, "announce": { "ipv4 flow": { "no-nexthop": { "flow-11": { "fragment": [ "unknown fragment type 12" ], "string": "flow fragment unknown fragment type 12" } } } } } } } }

Full pcap dump here: https://www.dropbox.com/s/p9ozjmecyiun8t5/last_and_first_fragment_flow_spec_dump.pcap?dl=0 (perfectly decoded with up to date Wireshark version).

Could you take a look on it?

[thomas-mangin]

This patch (incomplete as it does not generate a correct JSON list for the fragment, does parse the new generated syntax and also only address fragment when other type may have the same issue) make things work but I do not know if what you generated is valid or not ..

git diff
diff --git a/lib/exabgp/bgp/message/update/nlri/flow.py b/lib/exabgp/bgp/message/update/nlri/flow.py
index bab0e57..b3ede31 100644
--- a/lib/exabgp/bgp/message/update/nlri/flow.py
+++ b/lib/exabgp/bgp/message/update/nlri/flow.py
@@ -71,6 +71,7 @@ class CommonOperator (object):

 class NumericOperator (CommonOperator):
        # reserved= 0x08  # 0b00001000
+       ABSENT    = 0x00  # 0b00000000
        LT        = 0x04  # 0b00000100
        GT        = 0x02  # 0b00000010
        EQ        = 0x01  # 0b00000001
@@ -78,6 +79,7 @@ class NumericOperator (CommonOperator):

 class BinaryOperator (CommonOperator):
        # reserved= 0x0C  # 0b00001100
+       ABSENT    = 0x00  # 0b00000000
        NOT       = 0x02  # 0b00000010
        MATCH     = 0x01  # 0b00000001

@@ -206,6 +208,7 @@ class NumericString (object):
        value = None

        _string = {
+               NumericOperator.ABSENT: '',
                NumericOperator.LT: '<',
                NumericOperator.GT: '>',
                NumericOperator.EQ: '=',
diff --git a/lib/exabgp/protocol/ip/fragment.py b/lib/exabgp/protocol/ip/fragment.py
index 540e110..69d4599 100644
--- a/lib/exabgp/protocol/ip/fragment.py
+++ b/lib/exabgp/protocol/ip/fragment.py
@@ -29,19 +29,25 @@ class Fragment (int):
        LAST     = 0x08
        # reserved = 0xF0

+       _strings = {
+               NOT:   'not-a-fragment',
+               DONT:  'dont-fragment',
+               IS:    'is-fragment',
+               FIRST: 'first-fragment',
+               LAST:  'last-fragment',
+       }
+
        def __str__ (self):
-               if self == self.NOT:
-                       return 'not-a-fragment'
-               if self == self.DONT:
-                       return 'dont-fragment'
-               if self == self.IS:
-                       return 'is-fragment'
-               if self == self.FIRST:
-                       return 'first-fragment'
-               if self == self.LAST:
-                       return 'last-fragment'
-               return 'unknown fragment value %d' % int(self)
+               def found ():
+                       value = int(self)
+                       for fragment in self._strings.keys():
+                               if value & fragment:
+                                       yield self._strings[fragment]
+                                       value -= fragment
+                       if value:
+                               yield 'unknown-fragment-%d' % value

+               return '|'.join(found())

 def NamedFragment (name):
        fragment = name.lower()

Running the modified code:

./sbin/exabgp qa/conf/flow.conf -d --decode "ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 32 02 00 00 00 1b 80 0e 09 00 01 85 00 00 03 0c 80 0c 40 01 01 02 c0 10 08 80 06 00 00 00 00 00 00"
[...]
Fri, 26 Feb 2016 09:24:22 | INFO     | 46325  | parser        | decoded update 1 flow fragment first-fragment|last-fragment origin incomplete extended-community rate-limit 0
Fri, 26 Feb 2016 09:24:22 | INFO     | 46325  | parser        | update json { "exabgp": "3.4.0", "time": 1456478662, "host" : "Thomas.local", "pid" : "46325", "ppid" : "42095", "counter": 1, "type": "update", "neighbor": { "ip": "127.0.0.1", "address": { "local": "127.0.0.1", "peer": "127.0.0.1"}, "asn": { "local": "1", "peer": "1"}, "message": { "update": { "attribute": { "origin": "incomplete", "extended-community": [ 9225060886715039744 ] }, "announce": { "ipv4 flow": { "none": { "flow-11": { "fragment": [ "first-fragment|last-fragment" ], "string": "flow fragment first-fragment|last-fragment" } } } } } }} }

[thomas-mangin]

wireshark does not try to make sense of the data, it just shows what each bit stands for. You can throw some invalid data at him, as long as it can express the meaning of the bits, it will. So I do not know if what you generated is a valid flow spec or not.

I updated the decoder to handle it but it may be that it should not do as it seems that your message (1) does not have a comparaison operator - the ABSENT part of the patch (2) set more than one flag - the iteration over the bits.

The first one looks wrong to me, the second one may well be right and I may need to update my code to handle this case.

[pavel-odintsov]

Hello!

I just want to implement case when I could filter out packets with two or more flags enabled. I will do my custom generator of this packets and could discuss this case in details.

[thomas-mangin]

Make sense - could you please let me know if Juniper/Cisco is accepting your payload. If they do, I will update the code, otherwise I will most likely still update the code. As it would break backward compatibility in 3.4, I will only implement the feature in master.

[thomas-mangin]

The code in master handles the parsing of this packet better, the only things being supporting multiple bits setup. I will therefore fix/add the support for it.

[thomas-mangin]

Pavel, all should be right now on master. As previously explained, I can not easily backport this work in 3.4 so this will be master only.