Fixed whitespace bug in port parser

[drycode]

Notes

Problem Statement

An off by one error in the detect-engine-port.c was causing negation port strings (everything after the port range) of length 16 to be copied to an array in such a way that the last character was dropped. Example

"              45"

was interpretted as port 4. Therefore, instead of negating port 45, or failing to parse the rule, Suricata was applying the negation rule to port 4.

Solution:

During parsing, send a parsing exception for ports of length 16. Ports with length greater than 16 already send the same exception.

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

• [x] I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
• [x] I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
• [x] I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable) N/A

Link to redmine ticket: N/A

Describe changes:

• When parsing ports, the off-by-one exception is being handled by making an assertion about the string length, and defaulting to error state if the port string is >= 16 https://github.com/OISF/suricata/compare/master...drypycode:whitespace-bug#diff-d60d6f0bbed7504f674240e85929fecc6fc5ff161d25155414506e928fb3e3f8R1289

suricata-verify-pr: 829 #suricata-verify-repo: #suricata-verify-branch: #suricata-update-pr: #suricata-update-repo: #suricata-update-branch: #libhtp-pr: #libhtp-repo: #libhtp-branch:

[catenacyber]

Thanks for this contribution. I uncommented the reference to the Suricata-Verify PR so that it gets run with CI

@victorjulien is this part of the stuff you are working on ?

[victorjulien]

@victorjulien is this part of the stuff you are working on ?

No, working on the address side of rule parsing only.

[victorjulien]

Thanks for this contribution. I uncommented the reference to the Suricata-Verify PR so that it gets run with CI

I fixed your edit :-P

[drycode]

A few questions:

1. Is there a way to check the output of the output of the formatting script?
2. It appeared that the builds failed for MacOS because of an unused variable I had declared. I removed it, but It seems I'll need re-approval to run workflows. I can't seem to view the results of the previous build anymore. Is there a way to approve this PR to run workflows so I can continue to debug?

Thanks for the quick responses! Happy to help.

[jufajardini]

A few questions:

1. Is there a way to check the output of the output of the formatting script? Is this related to clang? If so, you can run from the suricata dir on your local repo

./scripts/clang-format.sh check-branch

This will check if your code needs formatting. Running

./scripts/clang-format.sh branch

Will format all of your changes in your branch as an additional commit - then you can check that commit to see the changes

2. It appeared that the builds failed for MacOS because of an unused variable I had declared. I removed it, but It seems I'll need re-approval to run workflows. I can't seem to view the results of the previous build anymore. Is there a way to approve this PR to run workflows so I can continue to debug?

Thanks for the quick responses! Happy to help.

Hello! I just approved the CI checks again, but I believe that for those changes you should also be able to check the CI tests in your forked repo, right?

[drycode]

Hello! I just approved the CI checks again, but I believe that for those changes you should also be able to check the CI tests in your forked repo, right?

Ahh I didn't know this. Haven't used Github workflows before, but I'll play around with it.

[jufajardini]

Hello! I just approved the CI checks again, but I believe that for those changes you should also be able to check the CI tests in your forked repo, right?

Ahh I didn't know this. Haven't used Github workflows before, but I'll play around with it.

I usually let the CI checks run on my forked repo before submitting my PRs, as that allows me to filter out most of the errors that could arise from that. Saves me some time and unnecessary re-doing of PRs :P

[drycode]

@jufajardini Definitely. Thanks for the advice! Any idea why the build scripts aren't running on my machine?

➜  suricata git:(whitespace-bug) ./scripts/clang-format.sh whitespace-bug
./scripts/clang-format.sh: command substitution: line 395: syntax error near unexpected token `)'
./scripts/clang-format.sh: command substitution: line 395: `)'
./scripts/clang-format.sh: line 366: syntax error near unexpected token `;;'
./scripts/clang-format.sh: line 366: `            ;;'

perhaps a bash version issue?

[jufajardini]

@jufajardini Definitely. Thanks for the advice! Any idea why the build scripts aren't running on my machine?

➜  suricata git:(whitespace-bug) ./scripts/clang-format.sh whitespace-bug
./scripts/clang-format.sh: command substitution: line 395: syntax error near unexpected token `)'
./scripts/clang-format.sh: command substitution: line 395: `)'
./scripts/clang-format.sh: line 366: syntax error near unexpected token `;;'
./scripts/clang-format.sh: line 366: `            ;;'

perhaps a bash version issue?

I think I led to some communication issue. the command is really supposed to be

./scripts/clang-format.sh branch

No need to replace branch with your branch-name there. Would that help?

[drycode]

No need to replace branch with your branch-name there. Would that help? Same issue actually 🤔

[jufajardini]

No need to replace branch with your branch-name there. Would that help? Same issue actually thinking

Talkin about versions... what's your clang version? I see that our ci checks use clang-format 9...

[drycode]

No need to replace branch with your branch-name there. Would that help? Same issue actually thinking

Talkin about versions... what's your clang version? I see that our ci checks use clang-format 9...

➜  suricata git:(whitespace-bug) clang --version
Apple clang version 13.0.0 (clang-1300.0.29.30)
Target: x86_64-apple-darwin20.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

[jufajardini]

No need to replace branch with your branch-name there. Would that help? Same issue actually thinking

Talkin about versions... what's your clang version? I see that our ci checks use clang-format 9...

➜  suricata git:(whitespace-bug) clang --version
Apple clang version 13.0.0 (clang-1300.0.29.30)
Target: x86_64-apple-darwin20.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

Mine is 10... but Ubuntu. I don't think that's the issue, then.

If you try ./scripts/clang-format.sh --help do you also get an error, or do any other commands work?

[drycode]

Looking at my output from the error, it really looks like an issue with the do...case statement in the bash script. I'll keep debugging. I was able to get the workflows running on my branch though!

[drycode]

@victorjulien @jufajardini Looks like the 2 "fuzzer" checks are failing, but everything else is passing. It's giving me the following error:

+ cd ..
+ export CARGO_BUILD_TARGET=x86_64-unknown-linux-gnu
+ CARGO_BUILD_TARGET=x86_64-unknown-linux-gnu
+ export MSAN_OPTIONS=strict_memcmp=false
+ MSAN_OPTIONS=strict_memcmp=false
+ mv libhtp suricata/
mv: cannot move 'libhtp' to 'suricata/libhtp': Directory not empty
2022-05-25 20:47:33,048 - root - ERROR - Building fuzzers failed.
2022-05-25 20:47:33,048 - root - ERROR - Error building fuzzers for (commit: 963d58b6f85aaec9e64dfec8b50ceab357272d35, pr_ref: refs/pull/1/merge).

This looks to me like a problem with the build script. Is this a known issue?

[jufajardini]

@victorjulien @jufajardini Looks like the 2 "fuzzer" checks are failing, but everything else is passing. It's giving me the following error:

+ cd ..
+ export CARGO_BUILD_TARGET=x86_64-unknown-linux-gnu
+ CARGO_BUILD_TARGET=x86_64-unknown-linux-gnu
+ export MSAN_OPTIONS=strict_memcmp=false
+ MSAN_OPTIONS=strict_memcmp=false
+ mv libhtp suricata/
mv: cannot move 'libhtp' to 'suricata/libhtp': Directory not empty
2022-05-25 20:47:33,048 - root - ERROR - Building fuzzers failed.
2022-05-25 20:47:33,048 - root - ERROR - Error building fuzzers for (commit: 963d58b6f85aaec9e64dfec8b50ceab357272d35, pr_ref: refs/pull/1/merge).

This looks to me like a problem with the build script. Is this a known issue?

Hello! Looking at other PRs and personal repos, it doesn't seem that this is happening for everyone... I've re-approved the workflow runs, let's see what do they say.

[codecov[bot]]

Codecov Report

Merging #7424 (f2be2a9) into master (b6407c4) will decrease coverage by 0.13%. The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master    #7424      +/-   ##
==========================================
- Coverage   75.94%   75.81%   -0.14%     
==========================================
  Files         656      657       +1     
  Lines      189916   189569     -347     
==========================================
- Hits       144233   143715     -518     
- Misses      45683    45854     +171     

Flag	Coverage Δ
fuzzcorpus	60.32% <100.00%> (-0.32%)	:arrow_down:
suricata-verify	52.04% <100.00%> (+0.14%)	:arrow_up:
unittests	60.92% <92.30%> (-0.16%)	:arrow_down:

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

[drycode]

With regards to the Fedora 35 build that's failing, can you please rerun it? It appears it might not be deterministic as it passed in my branch https://github.com/drypycode/suricata/runs/6599842582?check_suite_focus=true

Also note it appears to be failing on the rust linter, and my PR doesn't contain any rust code that isn't currently in the master branch

[drycode]

@jufajardini Could use some clarification on what you want me to do here. As far as I can tell, these builds are not deterministic between my fork's PR and this PR. It also appears that the code that is failing the build is not code that I wrote or modified in any way.

If there's anything else I can do to get this merged, let me know. Otherwise, I'll leave it to you all to determine what to do with

[jufajardini]

@jufajardini Could use some clarification on what you want me to do here. As far as I can tell, these builds are not deterministic between my fork's PR and this PR. It also appears that the code that is failing the build is not code that I wrote or modified in any way.

If there's anything else I can do to get this merged, let me know. Otherwise, I'll leave it to you all to determine what to do with

Hello @drypycode, I think @victorjulien is better suited to approve or say if we're missing anything here. The CI Check failure was not related to your code; you removed the test that didn't have to be in the commit.

From my end, there's a nit thing regarding the commit message. We use the format:

module: change (imperative form)

so... something like:

detect/parse-port: fix whitespaces bug

Would work better with our commit history :)

[drycode]

@victorjulien anything I can do to help get this into master?

[catenacyber]

Thanks @drypycode for pinging.

I did not look into the code... But

anything I can do to help get this into master?

I think this will need a redmine ticket to be created, to be referenced in the commit message. (We will use that for backports)

[victorjulien]

Merged in #8137, thanks!

I did change the logic to be more permissive. It strips the leading spaces.