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.

Sign up for GitHub

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

Jump to bottom

Add negation support for TLS and corresponding unit tests #5763

Closed
wants to merge 1 commit into from
Closed

Add negation support for TLS and corresponding unit tests #5763

wants to merge 1 commit into from

Conversation

Fredamabob
Copy link

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

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

Describe changes:

  • Added support for negation operator when specifying a TLS version. SSL versions (they use separate parsers for SSL and TLS) already supported it.
  • Added test cases to confirm that a negated rule behaves properly.

@Fredamabob Fredamabob requested review from norg and a team as code owners January 21, 2021 17:06
@codecov
Copy link

codecov bot commented Jan 21, 2021 •
edited

Codecov Report

Merging #5763 (105f2c4) into master (8536048) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5763      +/-   ##
==========================================
+ Coverage   76.87%   77.07%   +0.19%     
==========================================
  Files         613      613              
  Lines      186697   186792      +95     
==========================================
+ Hits       143529   143963     +434     
+ Misses      43168    42829     -339
Flag Coverage Δ
fuzzcorpus 52.94% <33.33%> (+0.01%) ⬆️
suricata-verify 52.09% <0.00%> (+0.40%) ⬆️
unittests 63.13% <100.00%> (+0.11%) ⬆️

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

@catenacyber
Copy link
Contributor

Thanks for this @Fredamabob
I would love to have Rust replace this PARSE_REGEX
Would you be interested in doing that ?

@Fredamabob
Copy link
Author

Thanks for this @Fredamabob
I would love to have Rust replace this PARSE_REGEX
Would you be interested in doing that ?

Hey, thanks for the feedback. I think rewriting it in Rust would be nice - but a larger task than expected. Do you think it'd be better to go with smaller changes to be safer, and open up another ticket for that?

@catenacyber
Copy link
Contributor

Do you think it'd be better to go with smaller changes to be safer, and open up another ticket for that?

This PR is ok as is.
The ticket to track rustification is https://redmine.openinfosecfoundation.org/issues/3195

catenacyber
catenacyber reviewed Feb 2, 2021
View reviewed changes
doc/userguide/rules/tls-keywords.rst

Supported values: "1.0", "1.1", "1.2", "1.3"
Supported values: "1.0", "1.1", "1.2", "1.3", "!1.0", "!1.1", "!1.2", "!1.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this support multiple values ?
Like either 1.1 or 1.2 ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. It will match on values 1.0->1.3, and !1.0->!1.3

Copy link
Contributor

Choose a reason for hiding this comment

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

That was not what I was asking.
Is it possible with this keyword to match on TLS flows which will either be TLS 1.1 or TLS 1.2 (but not TLS 1.3, not TLS 1.0 and not SSLv2 or 3) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also found out recently about SIGMATCH_HANDLE_NEGATION
Is this related ?

Copy link
Author

Choose a reason for hiding this comment

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

Ah alright, my bad. After a bit of testing with suricata-verify, it seems that the answer may be no...
SIGMATCH_HANDLE_NEGATION appears to be very similar to the approach implemented here, it may be related - I am only aware of this now that you have mentioned it.

Copy link
Author

Choose a reason for hiding this comment

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

@catenacyber Hi, sorry about the late reply. I can look into this if you'd like, but it might take awhile for me to get back to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, no rush, just wanted to know if this PR still alive :-)

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the long delay. I have updated it to use the flag SIGMATCH_HANDLE_NEGATION instead of using a new flag defined in detect-tls-version.h.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this improvement, I think CI will fail because of the PCRE1/PCRE2 thing cf other comment. Could you please take care of that ?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll take a look into that later this week!

catenacyber
catenacyber reviewed Oct 3, 2021
View reviewed changes
src/detect-tls-version.c Outdated
pcre2len = sizeof(ver_ptr);
res = pcre2_substring_copy_bynumber(
parse_regex.match, 1, (PCRE2_UCHAR8 *)ver_ptr, &pcre2len);
res = pcre_copy_substring((char *)str, ov, MAX_SUBSTRINGS, 1, ver_ptr, sizeof(ver_ptr));
Copy link
Contributor

Choose a reason for hiding this comment

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

You should now use PCRE2 which got merged in master, and no longer PCRE1...
Basically, that means you should leave untouched DetectParsePcreExec and such... and your PR should work

catenacyber
catenacyber approved these changes Oct 10, 2021
View reviewed changes
Copy link
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.

Looks good to me

jlucovsky
jlucovsky reviewed Oct 11, 2021
View reviewed changes
src/tests/detect-tls-version.c Outdated
@@ -86,7 +102,7 @@ static int DetectTlsVersionTestDetect01(void)
p->flags |= PKT_HAS_FLOW|PKT_STREAM_EST;
f.alproto = ALPROTO_TLS;

StreamTcpInitConfig(true);
StreamTcpInitConfig(TRUE);
Copy link
Contributor

Choose a reason for hiding this comment

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

StreamTcpInitConfig requires a bool so please remove this and similar changes.

Copy link
Author

Choose a reason for hiding this comment

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

Removed these changes.

victorjulien
victorjulien requested changes Jan 27, 2022
View reviewed changes
Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

Test change requested. Also generally please implement changes in a new branch and PR. This makes it easier to review. Thanks!

src/tests/detect-tls-version.c
/*
* Test that a TLS version 1.0 packet will match on a !1.1 version signature
*/
static int DetectTlsVersionTestDetect03(void)
Copy link
Member

Choose a reason for hiding this comment

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

we're actively trying to move away from these types of unittests in favor of suricata-verify tests. The unittests that mimic traffic and setting up all the layers (packet, stream, flow, etc) are generally taking too many shortcuts that hurt us later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlucovsky jlucovsky jlucovsky left review comments

@victorjulien victorjulien victorjulien requested changes

@catenacyber catenacyber catenacyber approved these changes

@norg norg Awaiting requested review from norg

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
4 participants
@Fredamabob @catenacyber @victorjulien @jlucovsky