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

Detect integers 6644 v11 #10222

Closed
wants to merge 5 commits into from

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/6644 and all subtickets
https://redmine.openinfosecfoundation.org/issues/6645
https://redmine.openinfosecfoundation.org/issues/6646
https://redmine.openinfosecfoundation.org/issues/6647
https://redmine.openinfosecfoundation.org/issues/6648
https://redmine.openinfosecfoundation.org/issues/6628

Describe changes:

  • detect/integers: support hexadecimal notation for parsing
  • detect/integers: add mode for negated range
  • detect/integers: rust derive for enumerations
  • detect/integers: keywords now accept bitmasks
  • doc: detect/integers

#10197 with code review taken into account (better doc)

catenacyber and others added 4 commits January 22, 2024 20:28
So that we can write enip.revision: 0x203

Ticket: 6645
Ticket: 6647

Allows keywords using integers to use strings in signature
parsing based on a rust enumeration with a derive.
Ticket: 6648

Like &0x40=0x40 to test for a specific bit set
Ticket: 6628

Document the generic detection capabilities for integer keywords.
and make every integer keyword pointing to this section.
@catenacyber catenacyber force-pushed the detect-integers-6644-v11 branch from cd32edc to 27093cc Compare January 22, 2024 20:17
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 17685

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (3cb7112) 82.18% compared to head (27093cc) 82.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10222      +/-   ##
==========================================
+ Coverage   82.18%   82.23%   +0.04%     
==========================================
  Files         977      978       +1     
  Lines      271894   272012     +118     
==========================================
+ Hits       223465   223698     +233     
+ Misses      48429    48314     -115     
Flag Coverage Δ
fuzzcorpus 63.29% <42.85%> (+0.30%) ⬆️
suricata-verify 61.46% <42.85%> (-0.03%) ⬇️
unittests 62.83% <97.61%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

Picked up two things for in the docs, pointed them out inline.

(Really want to see this merged!)

The integer value can be written as base-10 like ``100`` or as
an hexadecimal value like ``0x64``.

The most direct exemple is to match for equality, but there are
Copy link
Contributor

Choose a reason for hiding this comment

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

nit typo: example

(sorry for not picking that up before)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing

Comment on lines +63 to +64
websocket.opcode:text;
websocket.opcode:1; # behaves the same
Copy link
Contributor

Choose a reason for hiding this comment

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

For a future work: I think that this deserves a ticket for us to ensure that all such cases have proper documentation indicating that both ways work, and what are the accepted text values for each.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should rather be part of the dev/review process then.

Otherwise, this ticket can never be closed as there will always be new future keywords

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, not for this ticket, indeed!
Do you mean that as part of the review process for new keywords, we should do this?
I was thinking of adding a ticket for when this work is merged, someone review the documentation we have and update it as needed...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this is a new feature brought by a commit in this PR so no keywords use it now.
One in the websocket PR and 2 in the Enip PR do use this

Copy link
Contributor

Choose a reason for hiding this comment

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

AH! Sorry for the noise, then 🙇🏽

@@ -280,6 +280,8 @@ bsize
With the ``bsize`` keyword, you can match on the length of the buffer. This adds
precision to the content match, previously this could have been done with ``isdataat``.

bsize uses an :ref:`unsigned 64-bits integer <rules-integer-keywords>`.

An optional operator can be specified; if no operator is present, the operator will
default to '='. When a relational operator is used, e.g., '<', '>' or '<>' (range),
the bsize value will be compared using the relational operator. Ranges are inclusive.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should update this line where it says that ranges are inclusive, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be its own ticket, and back ported.
Do you create it or should I ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can do it, then :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@catenacyber
Copy link
Contributor Author

Continued in #10234

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.

3 participants