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

smb: New keyword smb.cmd v7 #7372

Closed
wants to merge 43 commits into from
Closed

Conversation

zer1t0
Copy link
Contributor

@zer1t0 zer1t0 commented May 4, 2022

Make sure these boxes are signed before submitting your Pull Request -- thank you.

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

Describe changes:

  • New keyword smb.cmd added, which allows to match the command value in smb message header. It works for smb1 and smb2.
  • Documentation for the keyword added (created SMB keyword documentation)

Example of rule alert smb any any -> any any (msg: "Smb command rule"; smb.cmd: 10; sid: 1;) . More examples in documentation.

suricata-verify-pr: 733

@zer1t0 zer1t0 requested review from victorjulien, jasonish, norg and a team as code owners May 4, 2022 08:26
@zer1t0 zer1t0 mentioned this pull request May 4, 2022
3 tasks
cppcheck:

src/util-memcmp.h:281:18: warning: Identical condition 'len-offset<16', second condition is always false [identicalConditionAfterEarlyExit]
        if (diff < 16) {
                 ^
src/util-memcmp.h:280:24: note: 'diff' is assigned value 'len-offset' here.
        int diff = len - offset;
                       ^
src/util-memcmp.h:269:33: note: If condition 'len-offset<16' is true, the function will return/exit
        if (likely(len - offset < 16)) {
                                ^
src/util-memcmp.h:281:18: note: Testing identical condition 'len-offset<16'
        if (diff < 16) {
                 ^
src/util-memcmp.h:344:18: warning: Identical condition 'len-offset<16', second condition is always false [identicalConditionAfterEarlyExit]
        if (diff < 16) {
                 ^
src/util-memcmp.h:343:24: note: 'diff' is assigned value 'len-offset' here.
        int diff = len - offset;
                       ^
src/util-memcmp.h:318:33: note: If condition 'len-offset<16' is true, the function will return/exit
        if (likely(len - offset < 16)) {
                                ^
src/util-memcmp.h:344:18: note: Testing identical condition 'len-offset<16'
        if (diff < 16) {
                 ^
src/util-memcmp.h:171:18: warning: Identical condition 'len-offset<16', second condition is always false [identicalConditionAfterEarlyExit]
        if (diff < 16) {
                 ^
src/util-memcmp.h:170:24: note: 'diff' is assigned value 'len-offset' here.
        int diff = len - offset;
                       ^
src/util-memcmp.h:159:33: note: If condition 'len-offset<16' is true, the function will return/exit
        if (likely(len - offset < 16)) {
                                ^
src/util-memcmp.h:171:18: note: Testing identical condition 'len-offset<16'
        if (diff < 16) {
                 ^
src/util-memcmp.h:233:18: warning: Identical condition 'len-offset<16', second condition is always false [identicalConditionAfterEarlyExit]
        if (diff < 16) {
                 ^
src/util-memcmp.h:232:24: note: 'diff' is assigned value 'len-offset' here.
        int diff = len - offset;
                       ^
src/util-memcmp.h:208:33: note: If condition 'len-offset<16' is true, the function will return/exit
        if (likely(len - offset < 16)) {
                                ^
src/util-memcmp.h:233:18: note: Testing identical condition 'len-offset<16'
        if (diff < 16) {
                 ^
Since GCC 12 the memcmp code using `_mm_blendv_epi8` failed to work.
Inspection of the disassembled objects suggests that it simply omits
the instruction on systems that are not AVX512 capable. On AVX512
it does replace it with VPCMPB logic that appears to work.

Luckily our use of blend is actually uncessary. A simple AND is sufficient.

Bug: OISF#5312.
Reduce scope where possible. Suggested by cppcheck.
src/detect-engine-state.c:127:91: style: Suspicious calculation. Please use parentheses to clarify the code. The code ''a&b?c:d'' should be written as either ''(a&b)?c:d'' or ''a&(b?c:d)''. [clarifyCalculation]
    DetectEngineStateDirection *dir_state = &state->dir_state[direction & STREAM_TOSERVER ? 0 : 1];
                                                                                          ^
src/detect-engine-state.c:194:53: style: Suspicious calculation. Please use parentheses to clarify the code. The code ''a&b?c:d'' should be written as either ''(a&b)?c:d'' or ''a&(b?c:d)''. [clarifyCalculation]
    de_state->dir_state[direction & STREAM_TOSERVER ? 0 : 1].filestore_cnt += file_no_match;
                                                    ^
src/detect-engine-state.c:201:57: style: Suspicious calculation. Please use parentheses to clarify the code. The code ''a&b?c:d'' should be written as either ''(a&b)?c:d'' or ''a&(b?c:d)''. [clarifyCalculation]
    if (de_state->dir_state[direction & STREAM_TOSERVER ? 0 : 1].filestore_cnt == sgh->filestore_cnt)
                                                        ^
src/app-layer-ftp.c:945:49: style: Parameter 'ftp_state' can be declared with const [constParameter]
static FTPTransaction *FTPGetOldestTx(FtpState *ftp_state, FTPTransaction *starttx)
                                                ^
@catenacyber
Copy link
Contributor

Looks like you need to rebase on latest master so that CI goes green again

@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #7372 (a587411) into master (dfe76bb) will increase coverage by 0.05%.
The diff coverage is 84.96%.

@@            Coverage Diff             @@
##           master    #7372      +/-   ##
==========================================
+ Coverage   75.87%   75.92%   +0.05%     
==========================================
  Files         656      657       +1     
  Lines      190105   189960     -145     
==========================================
- Hits       144236   144228       -8     
+ Misses      45869    45732     -137     
Flag Coverage Δ
fuzzcorpus 60.58% <50.94%> (+0.05%) ⬆️
suricata-verify 51.91% <81.96%> (+0.16%) ⬆️
unittests 61.07% <54.70%> (+0.05%) ⬆️

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

@zer1t0
Copy link
Contributor Author

zer1t0 commented May 12, 2022

Moved to #7404

@zer1t0 zer1t0 closed this May 12, 2022
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