Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding tests for Diameter and SIP Port decoding preferences #8

Merged
merged 5 commits into from
Jun 26, 2024

Conversation

dipeshwalia
Copy link
Contributor

@dipeshwalia dipeshwalia commented Jun 20, 2024

Hello folks,
Thank you for reading the following PR.

We are using set preferences and notice some protocols are not correctly decoded.

I have tried to add a test PR to show the failing issue. Please advise me if I am missing something.

CI results can be viewed here

cc @dehydr8 for your attention.

Thank you so much again for your time.

Note: I modified the make file to make c-ares-1.15.0 worked for ci to run tests.

@dehydr8
Copy link
Member

dehydr8 commented Jun 21, 2024

Hi @dipeshwalia

I looked at the prefs_set_pref code that we use to set preferences. It has cases to handle deprecated preferences and ignore obsolete ones. This doesn't mean that the pref you're trying to set is invalid.

https://github.com/wireshark/wireshark/blob/c552f74cdc235956749afb80f6536ca8c9c145d7/epan/prefs.c#L5560C16-L5560C36

https://github.com/wireshark/wireshark/blob/c552f74cdc235956749afb80f6536ca8c9c145d7/epan/prefs.c#L5339

The prefs you mentioned appear to be on the list:

    /* These are obsolete preferences from the dissectors' view,
       (typically because of a switch from a single value to a
       range value) but the name of the preference conflicts
       with the generated preference name from the dissector table.
       Don't allow the obsolete preference through to be handled */
    struct obsolete_pref_name obsolete_prefs[] = {
        {"diameter.tcp.port"},
        {"kafka.tcp.port"},
        {"mrcpv2.tcp.port"},
        {"rtsp.tcp.port"},
        {"sip.tcp.port"},
        {"t38.tcp.port"},
    };

The Wireshark QT UI doesn't use prefs_set_pref (they use specific methods for different pref types) but tshark and sharkd do.

Is there a way you can reproduce this with tshark or sharkd?

@dipeshwalia
Copy link
Contributor Author

Hi @dehydr8,
Thank you for your super quick response.

I am attaching a sample pcap that I used to test tshark decode preference.

tshark -r sample.pcap

1 0.000000 16.0.0.1 → 20.0.0.2 TCP 354 64568 → 3871 [PSH, ACK] Seq=3992574984 Ack=2271477523 Win=32768 Len=284 TSval=204338 TSecr=144170106

with decode

tshark -r sample.pcap -d tcp.port==3871,diameter

1 0.000000 16.0.0.1 → 20.0.0.2 DIAMETER 354 cmd=Credit-Control Request(272) flags=RP-- appl=3GPP Sd(16777303) h2h=38523fe9 e2e=3879a419 |

sample.pcap.zip

Let me know if I missed something.

Thank you

@dehydr8
Copy link
Member

dehydr8 commented Jun 21, 2024

Hi @dipeshwalia

It appears that tshark doesn't use prefs_set_pref but relies on dissect_opts_handle_opt in wireshark/ui/dissect_opts.c

We do not touch any ui code from Wireshark and only rely on epan for Wiregasm.

I have added a special case for handling PREF_DECODE_AS_RANGE in wg_set_pref that follows the same steps when changes are made in the Wireshark UI Preference Editor.

Let me know if it works for you.

@dehydr8 dehydr8 self-assigned this Jun 21, 2024
@dehydr8 dehydr8 added the bug Something isn't working label Jun 21, 2024
@dipeshwalia
Copy link
Contributor Author

Hi @dehydr8

Thank you for this patch; I can confirm my early results worked.

I'll play deeper and keep you posted.

@dipeshwalia
Copy link
Contributor Author

@dehydr8 Thank you. We are good with this change. I tested it on a bunch of cases, and there have been no issues so far. Thank you so much for your support.

Do you have Discord, Slack, or some other community account? We would love to connect

@dehydr8
Copy link
Member

dehydr8 commented Jun 26, 2024

Awesome, I'll merge it then.

I currently don't, but can create a Discord server for good.tools, with a #wiregasm channel.

Edit: Here's the link to the invite: https://discord.gg/wfdVbNaM

@dehydr8 dehydr8 merged commit ed415d3 into good-tools:master Jun 26, 2024
1 check passed
Copy link

🎉 This PR is included in version 1.6.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants