Skip to content

smb: fix SMB_COM_WRITE_ANDX record parser - v4#8905

Closed
b1tg wants to merge 2 commits into
OISF:masterfrom
b1tg:issue/6008-smb-v4
Closed

smb: fix SMB_COM_WRITE_ANDX record parser - v4#8905
b1tg wants to merge 2 commits into
OISF:masterfrom
b1tg:issue/6008-smb-v4

Conversation

@b1tg
Copy link
Copy Markdown
Contributor

@b1tg b1tg commented May 22, 2023

Bug: #6008

ticket:
https://redmine.openinfosecfoundation.org/issues/6008

Previous PR: #8889

Changes from last PR:

  • split into two commit and add tests
  • update SV

SV_BRANCH=pr/1211

b1tg added 2 commits May 22, 2023 09:16
Wrong data offset when wct = 12

Bug: OISF#6008
Fix data padding logic and handle possible evasion

Bug: OISF#6008
@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2023

Codecov Report

Merging #8905 (1445f07) into master (ebe0a7b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8905      +/-   ##
==========================================
+ Coverage   82.30%   82.33%   +0.03%     
==========================================
  Files         969      969              
  Lines      273335   273376      +41     
==========================================
+ Hits       224961   225084     +123     
+ Misses      48374    48292      -82     
Flag Coverage Δ
fuzzcorpus 64.73% <100.00%> (+0.07%) ⬆️
suricata-verify 60.47% <88.88%> (+0.01%) ⬆️
unittests 62.96% <100.00%> (+0.01%) ⬆️

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

@victorjulien
Copy link
Copy Markdown
Member

Can you look into why some of the SV tests fail?

Also, could you give the git commits unique subjects?

@b1tg
Copy link
Copy Markdown
Contributor Author

b1tg commented May 23, 2023

Can you look into why some of the SV tests fail?

Why Fedora 37 show error about filestore-filecontainer-smb1-padding which is not in SV master?

@victorjulien
Copy link
Copy Markdown
Member

Can you look into why some of the SV tests fail?

Why Fedora 37 show error about filestore-filecontainer-smb1-padding which is not in SV master?

It uses the tests from SV PR 1211, since that is what the PR body specifies.

@b1tg
Copy link
Copy Markdown
Contributor Author

b1tg commented May 23, 2023

Seems the num before .json is not fixed between runs, i am not familiar with SV, any suggestions on how to troubleshoot this? Thanks.

checks:
  - filter:
      filename: "filestore/04/04f93fbae50680991af90eb8a5a447d7b353d9c09097b3a905745d285d7ba634.1684287875.1.json"
      count: 1
      match:
        fileinfo.sha256: 04f93fbae50680991af90eb8a5a447d7b353d9c09097b3a905745d285d7ba634
  - filter:
      filename: "filestore/81/81ef17f513f4959ba2a8243fa1412fa11b7d8f2c064da1f7ae98429188b6229c.1684287889.2.json"
      count: 1
      match:
        fileinfo.sha256: 81ef17f513f4959ba2a8243fa1412fa11b7d8f2c064da1f7ae98429188b6229c

@b1tg
Copy link
Copy Markdown
Contributor Author

b1tg commented May 23, 2023

I just push a new commit in SV, could you re-run the failed tests?
OISF/suricata-verify@f8537ba

@victorjulien
Copy link
Copy Markdown
Member

I just push a new commit in SV, could you re-run the failed tests? OISF/suricata-verify@f8537ba

I've restarted all "build" runs.

@b1tg
Copy link
Copy Markdown
Contributor Author

b1tg commented May 23, 2023

All tests passed now.

Copy link
Copy Markdown
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

Thanks for these fixes

CI : 🟢
Code : 🟢
Commits segmentation : 🟢
Commit messages : could you give the git commits unique subjects?
I also think you can just write Bug: #6008 simpler than Bug: OISF#6008
Git ID set : 🟢
CLA : 🟢 already some merged commits
Doc update : 🟢 none needed
Redmine ticket : 🟢 I updated
Rustfmt : rust is not formatted before this PR
Tests : Are the unit tests duplicating the S-V ones ? I think we prefer S-V
Dependencies added: 🟢 none

@catenacyber
Copy link
Copy Markdown
Contributor

So, almost all green, just the commit messages to reword a bit, and a questions about the testing...

@b1tg
Copy link
Copy Markdown
Contributor Author

b1tg commented May 29, 2023

use #8938

@b1tg b1tg closed this May 29, 2023
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