[Pascal Quantin <pascal.quantin@xxxxxxxxx>]

2015-09-15 23:20 GMT+02:00 João Valverde <joao.valverde@xxxxxxxxxxxxxxxxxx>:
>
>
>
> On 09/15/2015 09:43 PM, Pascal Quantin wrote:
>>
>>
>>
>> 2015-09-15 22:39 GMT+02:00 João Valverde
>> <joao.valverde@xxxxxxxxxxxxxxxxxx
>> <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx>>:
>>
>>
>>
>>     On 09/15/2015 09:05 PM, Pascal Quantin wrote:
>>
>>
>>
>>         2015-09-15 21:15 GMT+02:00 João Valverde
>>         <joao.valverde@xxxxxxxxxxxxxxxxxx
>>         <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx>
>>         <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx
>>
>>         <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx>>>:
>>
>>
>>
>>              On 09/15/2015 07:38 PM, Pascal Quantin wrote:
>>
>>                  Hi João,
>>
>>                  Le 15 sept. 2015 4:41 PM, "João Valverde"
>>                  <joao.valverde@xxxxxxxxxxxxxxxxxx
>>         <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx>
>>                  <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx
>>         <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx>>
>>                  <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx
>>         <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx>
>>
>>                  <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx
>>         <mailto:joao.valverde@xxxxxxxxxxxxxxxxxx>>>> a écrit :
>>                    >
>>                    > Hi,
>>                    >
>>                    > I'm trying to understand and troubleshoot some
>>         "Decode As"
>>                  issues. To
>>                  give an example consider the packet:
>>                    >
>>                    > IPv6 | IPv6 | UDP
>>                    >
>>                    > Wouldn't the second IPv6 layer overwrite the Decode
>>         As protocol
>>                  number for the first layer, giving:
>>                    >
>>                    > IPv6 (Decode IP protocol 17 As:) | IPv6 (Decode IP
>>         protocol
>>                  17 As:)
>>                  | UDP
>>                    >
>>                    > Instead of the correct:
>>                    >
>>                    > IPv6 (Decode IP protocol 41 As:) | IPv6 (Decode IP
>>         protocol
>>                  17 As:) | UDP
>>
>>                  Did you try it? In that case what result did you get?
>>         Can you
>>                  share a pcap?
>>
>>                    >
>>                    > Is this a bug, a known limitation of the current
>>         code, or
>>                  just my
>>                  misunderstanding?
>>
>>                  My understanding of the 'Decode As' functionality is
>>         that it
>>                  will allow
>>                  you to override the default protocol id / next header
>>         value <-> sub
>>                  dissector mapping with another sub dissector call.
>>                  It means that in your example (if I understood it
>>         correctly) if you
>>                  change the sub dissector called for next header 17 it
>>         should not
>>                  impact
>>                  another next header value (value 41 in first IPv6 header).
>>                  Note that I do not have access to the code right now to
>>         verify the
>>                  behavior, but that's what I would expect (it works like
>>         this for
>>                  Ethertype for example).
>>
>>                  Regards,
>>                  Pascal.
>>
>>
>>              Hi Pascal,
>>
>>              Looking at the code to handle this:
>>
>>              static void ipv6_prompt(packet_info *pinfo, gchar* result)
>>              {
>>                   g_snprintf(result, MAX_DECODE_AS_PROMPT_LEN, "IP
>>         protocol %u as",
>>                       GPOINTER_TO_UINT(p_get_proto_data(pinfo->pool, pinfo,
>>              proto_ipv6, IPV6_PROTO_VALUE)));
>>              }
>>
>>              (...)
>>                   p_add_proto_data(wmem_packet_scope(), pinfo, proto_ipv6,
>>              IPV6_PROTO_VALUE, GUINT_TO_POINTER((guint)nxt));
>>              (...)
>>
>>              If you call p_add_proto_data() twice, first for the outer
>>         header
>>              with nxt == 41 (IPv6 Encapsulation) and after that for the
>>         inner
>>              header with nxt == 17 (UDP), only the UDP value will survive.
>>
>>              Both network layers will show a "Decode IP protocol 17 As"
>>         (UDP),
>>              that seems incorrect. This first (outer) network layer
>>         should show
>>              "Decode IP protocol 41 As" (IPv6 Encapsulation)...
>>
>>              It's not just a display issue. Selecting "Decode As" for
>>         the outer
>>              IPv6 header, decode as TCP for example, I'd expect the
>>         inner IPv6
>>              header to be decoded as TCP (and throw some errors for a
>>         malformed
>>              TCP header). That's my understanding of how the feature
>>         should work,
>>              "decode next layer as...". But instead the UDP (transport)
>>         layer is
>>              decoded as TCP, ignoring the inner (network) header. So it
>>         seems to
>>              me that the code doesn't handle a different layering other
>>         than:
>>
>>              <Network>
>>              <Transport>
>>
>>              When you have:
>>
>>              <Network>
>>              <Network>
>>              <Transport>
>>
>>              as in this example, it breaks. The solution doesn't seem
>>         trivial...
>>              my first impression is that p_add_proto_data() needs to store
>>              multiple "nxt" values somehow for 'proto_ipv6', then using some
>>              field in *pinfo to return the correct network layer value in
>>              ipv6_prompt(), and maybe improving the layer decoding logic to
>>              handle the second case correctly?
>>
>>              Hope that makes sense.
>>
>>
>>         Then it sounds like an issue specific to IPv6 (and not an issue
>>         with the
>>         'Decode As' functionality by itself).
>>
>>
>>     Good point. The problem (as such) might be related to the presence
>>     of the pseudo-layer "IPv6 Next Header" (or it might not, I don't see
>>     why it would be at a glance). I need to test and compare the same
>>     behaviour with IPv4.
>>
>>         The key argument of the p_(add|get)_proto_data is already used
>>         by some
>>         other dissectors as a way to identify multiple encapsulated messages
>>         (like in SIP messages).
>>         So instead of using
>>               p_get_proto_data(pinfo->pool, pinfo, proto_ipv6,
>>         IPV6_PROTO_VALUE);
>>         and
>>               p_add_proto_data(wmem_packet_scope(), pinfo, proto_ipv6,
>>         IPV6_PROTO_VALUE, GUINT_TO_POINTER((guint)nxt));
>>         you could give a try to:
>>               p_get_proto_data(pinfo->pool, pinfo, proto_ipv6,
>>         (IPV6_PROTO_VALUE<<24) | pinfo->curr_layer_num);
>>         and
>>               p_add_proto_data(wmem_packet_scope(), pinfo, proto_ipv6,
>>         (IPV6_PROTO_VALUE<<24) | pinfo->curr_layer_num,
>>         GUINT_TO_POINTER((guint)nxt));
>>
>>         And see if it helps (did not verify this, I'm just thinking loudly).
>>
>>
>>     Thanks for the pointer. It helps a lot anyway to know how other
>>     protocol handle this use case.
>>
>>
>> BTW I just gave a try to
>> https://bugs.wireshark.org/bugzilla/attachment.cgi?id=5842 that seems to
>> match your example and I was able to change inner protocol dissection
>> from UDP to TCP.
>> Presumably this is because the IPv6 dissector is doing a subtle
>> difference between what it calls IP Protocol and IPv6 Next Header...
>> What issue did you face exactly?
>
>
> If I understand correctly my issue is with the outer protocol dissection (not inner). Same exact issue with that capture. Frame 4, first IPv6 header (222::2 -> 333::3), select "Decode As", two issues (for first "Network" counting from the left layer in the decode as dialog):
>
> - "IP Protocol 17" should be "IP Protocol 41"
> - "Decode As" anything (TCP, ICMPv6, ...) takes effect in the transport layer (UDP) and not the inner IPv6 header (::172.16.199.2 -> ::224.0.0.2)

I started working on a mockup but it is not as straightforward as I initially thought.
When calling the dialog, curr_layer_num is (obviously) set to the value of the latest protocol in the tree.
It was quite easy to fix this when populating the GTK UI, but it does not work yet when applying the config. And Qt GUI is not using the same code path as GTK so it does not work either yet (same root cause as for GTK: curr_num_layer does not have the right value).
It deserves a bit more thought but it's too late for tonight :)

Pascal.