telekom / 5g-trace-visualizer

This set of Python scripts allow you to convert pcap, pcapnp or pdml 5G protocol traces (Wireshark, tcpdump, ...) into SVG sequence diagrams.
Apache License 2.0
265 stars 77 forks source link

Multiple TAC in NAS "5GS tracking area identity list" #28

Closed rtommy closed 1 year ago

rtommy commented 1 year ago

It is a valid case when the NAS "5GS tracking area identity list" contains a list of TAC values and not just a single TAC value.

The xml2json function is not prepared for that, it will overwrite the TAC items and keeps the last one.

Wireshark screenshot: tac

PDML lines:

<field name="nas_5gs.tac" showname="TAC: 134" size="3" pos="53" show="134" value="000086"/>
<field name="nas_5gs.tac" showname="TAC: 118" size="3" pos="56" show="118" value="000076"/>

Source code:

child_name = child.attrib["name"]
...
out[child_name].append(data_to_append)

PUML "note":

        5GS tracking area identity list:
            nas_5gs.mm.elem_id: 'Element ID: 0x54'
            gsm_a.len: 'Length: 10'
            Partial tracking area list  1:
                nas_5gs.mm.tal_t_li: '.00. .... = Type of list: list of TACs belonging to one PLMN or SNPN, with non-consecutive TAC values (0)'
                nas_5gs.mm.tal_num_e: '...0 0001 = Number of elements: 2 elements (1)'
                e212.5gstai.mcc: 'Mobile Country Code (MCC): Sweden (240)'
                e212.5gstai.mnc: 'Mobile Network Code (MNC): Unknown (80)'
                nas_5gs.tac: 'TAC: 118'

Please consider updating the xml2json function for this.

jkolom commented 1 year ago

Hi, thanks for spotting this one :) Could you please chekc if the fix I just pushed works for you? I tested it with the Free5GC traces in the examples and it seems to work. The bug was actually affecting other IEs, so it was an ugly one, actually.

rtommy commented 1 year ago

First of all, I think you accidentally left a print() there. I guess it should be logging.debug() instead. print('Found repeated child element {0}. Renamed to {1}'.format(original_child_name, child_name))

Secondly, I am not sure simply renaming the child is entirely ok since the new name won't be a valid Wireshark tag. I assume adding an additional comment in the function and mentioning this could be enough?

Or perhaps the child_name (e.g. nas_5gs.tac) should be a list with all the values instead of having a modified child_name for each occurrence. But this is not valid for every item such as nas_5gs.spare_half_octet, nas_5gs.spare_half_octet2...

jkolom commented 1 year ago

Hi, The flow is that the wireshark trace is converted to PDML (i.e. XML) and the file is then parsed. The name it is given in the returned data structure is just for visualization purposes. Since the data structure passed on to the visualization code is a dicitonary, keys need to be unique. Redoing the code to use not a dict but something else would requrie quite some reworking and well... it would be challenging to find time for that as this is entirely a hobby project born out of frustration during an early 5GC PoC that I decided to pulbicly upload 😉 I manually edited a PDML file from the examples with some random changes (i.e. not necessarily semantically correct) and it looks as shown below, which does not look that bad: image

Since the text (SVG) is searchable, searching for pfcp.flags still yields all of the child entries. Maybe a bit clearer it could be to use pfcp.flags (2)? The same would apply to the example with TAC below image

Something like the screenshots below: image image

As for the print... well, I am a bit sloppy 😄

rtommy commented 1 year ago

valid points. Just do the minimum for now to make it work. The new example where the sequence number is in brackets is much easier to read. Thanks.