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

Fixed the typo of the buffer name for sip.stat_code #11846

Closed
wants to merge 1 commit into from

Conversation

starrysec
Copy link

@starrysec starrysec commented Sep 30, 2024

Fixed the typo of the buffer name for sip.stat_code, The correct name is sip.stat_code, not sip.method.

SV_BRANCH=OISF/suricata-verify#2072

@starrysec starrysec changed the title Fixed typo : sip stat code buffer name. Fixed the typo of the buffer name for sip.stat_code Sep 30, 2024
Copy link

NOTE: This PR may contain new authors.

@@ -56,7 +56,7 @@

#define KEYWORD_NAME "sip.stat_code"
#define KEYWORD_DOC "sip-keywords.html#sip-stat-code"
#define BUFFER_NAME "sip.method"
#define BUFFER_NAME "sip.stat_code"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this.

Question for the team: can this lead to wrong detection when using sip.stat_codeand sip.method keywords ?

Note: this was fixed in master by moving the code to rust

Copy link
Member

@inashivb inashivb Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for the team: can this lead to wrong detection when using sip.stat_codeand sip.method keywords ?

Don't know anything about sip but we do have s-v tests for this which pass in master, main-7.0.x and this PR:

  1. sip-stat-code which passes. I can confirm that it passes correctly. Wireshark filter sip.Status-Code==100 shows exactly the number of matches expected.
  2. sip-method which passes. I can confirm that this passes correctly too. Wireshark filter sip.Method==REGISTER shows exactly the number of matches expected.

I haven't tried to look how the BUFFER_NAME impacts the parsing and storage of the respective fields..

@catenacyber
Copy link
Contributor

Don't know anything about sip but we do have s-v tests for this which pass in master, main-7.0.x and this PR:

What we would want to test here I guess is a test with at the same time both sip.stat_code and sip.method rules

Maybe we are only safe because these are not in the same direction

@catenacyber
Copy link
Contributor

I think that rule alert sip any any -> any any (sip.stat_code; content:"REGISTER"; sid:2;) will trigger an alert in sip-method SV test

and alert sip any any -> any any (sip.method; content:"100"; sid:2;) in sip-stat-code test

@catenacyber
Copy link
Contributor

@catenacyber
Copy link
Contributor

And I created OISF/suricata-verify#2072 as a test

Copy link
Member

@inashivb inashivb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your first contribution to Suricata! :)
Could you please:

  1. make commit message as follows:
sip/stat_code: fix buffer name

Bug #7295
  1. change the commit author to the format FirstName LastName <[email protected]>. This name should match the name you sign contribution agreement with.
  2. rebase on latest main-7.0.x.
  3. submit a new PR w the changes asked above. Please do not remove the PR body and tick all the boxes that apply.

@catenacyber
Copy link
Contributor

I was doing it in #11857

@catenacyber catenacyber closed this Oct 2, 2024
@glongo
Copy link
Contributor

glongo commented Oct 2, 2024

I apologize :'-)

@catenacyber
Copy link
Contributor

I apologize :'-)

I thank you for having fixed this in master already Giuseppe ;-)

@victorjulien
Copy link
Member

@starrysec can you please rework this in a next PR:

  1. make sure to review the entries from the PR template
  2. review & sign the CLA
  3. use a real name commit author setting.
    Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants