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 generic uint 4112 v2 #7121

Closed

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/4112

Describe changes:

  • Makes use of generic DetectUint structure for dsize and dcerpc

Still TODO:

  • remove the C version to use only rust version
  • more keywords in C use specific versions

Replaces #7117 with commit fixing smb test case

as is done for u8 and u32
Ie inequality test for integer

Also adds prefilter functions for u16
In case of greater/lesser or equal
Despite what the comment said 1<>2 is not a valid range
as it is empty and cannot have any match.

Maybe we should even consider 1<>3 an invalid range as it
should rather be written as =2
from http2 to a generic file
so that it can be reused by dcerpc and others
ie <0 is impossible
@codecov
Copy link

codecov bot commented Mar 6, 2022

Codecov Report

Merging #7121 (088f74a) into master (935ea74) will increase coverage by 0.03%.
The diff coverage is 76.67%.

@@            Coverage Diff             @@
##           master    #7121      +/-   ##
==========================================
+ Coverage   78.01%   78.04%   +0.03%     
==========================================
  Files         628      628              
  Lines      185402   185373      -29     
==========================================
+ Hits       144637   144672      +35     
+ Misses      40765    40701      -64     
Flag Coverage Δ
fuzzcorpus 59.98% <74.63%> (+0.30%) ⬆️
suricata-verify 54.54% <41.44%> (-0.08%) ⬇️
unittests 63.10% <57.32%> (-0.05%) ⬇️

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

@inashivb inashivb self-assigned this Mar 7, 2022
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 6517

@@ -88,6 +88,16 @@ static int DetectU32Validate(DetectU32Data *du32)
return 1;
}
break;
case DETECT_UINT_LTE:
if (du32->arg1 == UINT32_MAX) {
Copy link
Member

Choose a reason for hiding this comment

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

Having a really hard time understanding these validations. I know it is not wrong since it's already been there for so long but how does checking for the first arg to be UINT32_MAX in case of LTE help us validate the argument? Also, why do we not check anything w.r.t. arg2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DETECT_UINT_LTE and UINT32_MAX comes from <=0xffffffff which is always true, so we do not need a rule

Arg2 is only used with ranges

return 1;
}
// we need at least one value that can match parg > du16->arg1 && parg < du16->arg2
if (du16->arg1 + 1 >= du16->arg2) {
Copy link
Member

Choose a reason for hiding this comment

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

So, this is for cases like dsize 1<>2?
Q: if I have

arg1 = 1
arg2 = 2

this matches. But, we mention in 06eef95 that a case like that should be invalid as it is an empty range. Should this be allowed then?
Given that we have exclusive end range checks, should we instead check for arg1 + 2 >= arg2. That way we ensure at least one element in the range.

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 is for cases like dsize 1<>2?

Indeed

a case like that should be invalid as it is an empty range

Indeed

Should this be allowed then?

It is not allowed. This check precisely does that.
In this cas arg1=1, arg1+1=2 so arg1>=arg2 is true.
DetectU16Validate returns 1 when the signature is invalid

Copy link
Member

Choose a reason for hiding this comment

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

Should this be allowed then?

It is not allowed. This check precisely does that. In this cas arg1=1, arg1+1=2 so arg1>=arg2 is true. DetectU16Validate returns 1 when the signature is invalid

ooooooh. Read the status code wrong. Sorry.

@catenacyber
Copy link
Contributor Author

Replaced by #7150

victorjulien added a commit to victorjulien/suricata that referenced this pull request Jun 25, 2024
Don't assume the ntlmssp version field is always present if the flag is
set. Instead keep track of the offsets of the data of the various blobs
and see if there is space for the version.

Inspired by how Wireshark does the parsing.

Bug: OISF#7121.
victorjulien added a commit to victorjulien/suricata that referenced this pull request Jun 25, 2024
Don't assume the ntlmssp version field is always present if the flag is
set. Instead keep track of the offsets of the data of the various blobs
and see if there is space for the version.

Inspired by how Wireshark does the parsing.

Bug: OISF#7121.
victorjulien added a commit to victorjulien/suricata that referenced this pull request Jul 3, 2024
Don't assume the ntlmssp version field is always present if the flag is
set. Instead keep track of the offsets of the data of the various blobs
and see if there is space for the version.

Inspired by how Wireshark does the parsing.

Bug: OISF#7121.
victorjulien added a commit to victorjulien/suricata that referenced this pull request Jul 9, 2024
Don't assume the ntlmssp version field is always present if the flag is
set. Instead keep track of the offsets of the data of the various blobs
and see if there is space for the version.

Inspired by how Wireshark does the parsing.

Bug: OISF#7121.
(cherry picked from commit f59c43b)
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