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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions doc/userguide/rules/tls-keywords.rst
View file
Open in desktop
Expand Up @@ -153,11 +153,12 @@ Example::
tls.version
-----------

Match on negotiated TLS/SSL version.
Match on negotiated TLS/SSL version. Supports negation of the version match.

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!


It is also possible to match versions using a hex string.
These can also be inverted.

Examples::

Expand Down
15 changes: 13 additions & 2 deletions src/detect-tls-version.c
View file
Open in desktop
Expand Up @@ -54,7 +54,7 @@
/**
* \brief Regex for parsing "id" option, matching number or "number"
*/
#define PARSE_REGEX "^\\s*([A-z0-9\\.]+|\"[A-z0-9\\.]+\")\\s*$"
#define PARSE_REGEX "^\\s*(!?[A-z0-9\\.]+|\"[A-z0-9\\.]+\")\\s*$"

static DetectParseRegex parse_regex;

Expand Down Expand Up @@ -135,6 +135,11 @@ static int DetectTlsVersionMatch (DetectEngineThreadCtx *det_ctx,
ret = 1;
}

if (tls_data->flags & SIGMATCH_HANDLE_NEGATION) {
/* Inverse match result for negated sig */
ret = (ret == 0) ? 1 : 0;
}

SCReturnInt(ret);
}

Expand Down Expand Up @@ -185,6 +190,11 @@ static DetectTlsVersionData *DetectTlsVersionParse (DetectEngineCtx *de_ctx, con
tmp_str += 1;
}

if (tmp_str[0] == '!') {
tls->flags |= SIGMATCH_HANDLE_NEGATION;
tmp_str++;
}

if (strncmp("1.0", tmp_str, 3) == 0) {
temp = TLS_VERSION_10;
} else if (strncmp("1.1", tmp_str, 3) == 0) {
Expand All @@ -203,7 +213,8 @@ static DetectTlsVersionData *DetectTlsVersionParse (DetectEngineCtx *de_ctx, con

tls->ver = temp;

SCLogDebug("will look for tls %"PRIu16"", tls->ver);
SCLogDebug("will look for tls %s%" PRIu16 "",
(tls->flags & SIGMATCH_HANDLE_NEGATION) ? "!" : "", tls->ver);
}

return tls;
Expand Down
107 changes: 107 additions & 0 deletions src/tests/detect-tls-version.c
View file
Open in desktop
Expand Up @@ -32,6 +32,7 @@ static int DetectTlsVersionTestParse01 (void)
tls = DetectTlsVersionParse(NULL, "1.0");
FAIL_IF_NULL(tls);
FAIL_IF_NOT(tls->ver == TLS_VERSION_10);
FAIL_IF(tls->flags & SIGMATCH_HANDLE_NEGATION);
DetectTlsVersionFree(NULL, tls);
PASS;
}
Expand All @@ -50,6 +51,21 @@ static int DetectTlsVersionTestParse02 (void)
PASS;
}

/**
* \test DetectTlsVersionTestParse03 is a test to make sure that we parse the "id"
* option correctly when given valid (negated) id option
*/
static int DetectTlsVersionTestParse03(void)
{
DetectTlsVersionData *tls = NULL;
tls = DetectTlsVersionParse(NULL, "!1.0");
FAIL_IF_NULL(tls);
FAIL_IF_NOT(tls->ver == TLS_VERSION_10);
FAIL_IF_NOT(tls->flags & SIGMATCH_HANDLE_NEGATION);
DetectTlsVersionFree(NULL, tls);
PASS;
}

#include "stream-tcp-reassemble.h"

/** \test Send a get request in three chunks + more data. */
Expand Down Expand Up @@ -236,15 +252,106 @@ static int DetectTlsVersionTestDetect02(void)
PASS;
}

/*
* 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.

{
Flow f;
uint8_t tlsbuf1[] = { 0x16 };
uint32_t tlslen1 = sizeof(tlsbuf1);
uint8_t tlsbuf2[] = { 0x03 };
uint32_t tlslen2 = sizeof(tlsbuf2);
uint8_t tlsbuf3[] = { 0x01 };
uint32_t tlslen3 = sizeof(tlsbuf3);
uint8_t tlsbuf4[] = { 0x01, 0x00, 0x00, 0xad, 0x03, 0x02 };
uint32_t tlslen4 = sizeof(tlsbuf4);
TcpSession ssn;
Packet *p = NULL;
Signature *s = NULL;
ThreadVars th_v;
DetectEngineThreadCtx *det_ctx = NULL;
AppLayerParserThreadCtx *alp_tctx = AppLayerParserThreadCtxAlloc();

memset(&th_v, 0, sizeof(th_v));
memset(&f, 0, sizeof(f));
memset(&ssn, 0, sizeof(ssn));

p = UTHBuildPacket(NULL, 0, IPPROTO_TCP);

FLOW_INITIALIZE(&f);
f.protoctx = (void *)&ssn;
f.proto = IPPROTO_TCP;
p->flow = &f;
p->flowflags |= FLOW_PKT_TOSERVER;
p->flowflags |= FLOW_PKT_ESTABLISHED;
p->flags |= PKT_HAS_FLOW | PKT_STREAM_EST;
f.alproto = ALPROTO_TLS;

StreamTcpInitConfig(true);

DetectEngineCtx *de_ctx = DetectEngineCtxInit();
FAIL_IF_NULL(de_ctx);

de_ctx->flags |= DE_QUIET;

s = de_ctx->sig_list =
SigInit(de_ctx, "alert tls any any -> any any (msg:\"TLS\"; tls.version:!1.1; sid:1;)");
FAIL_IF_NULL(s);

SigGroupBuild(de_ctx);
DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx);

int r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_TLS, STREAM_TOSERVER, tlsbuf1, tlslen1);
FAIL_IF(r != 0);

r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_TLS, STREAM_TOSERVER, tlsbuf2, tlslen2);
FAIL_IF(r != 0);

r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_TLS, STREAM_TOSERVER, tlsbuf3, tlslen3);
FAIL_IF(r != 0);

r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_TLS, STREAM_TOSERVER, tlsbuf4, tlslen4);
FAIL_IF(r != 0);

SSLState *ssl_state = f.alstate;
FAIL_IF_NULL(ssl_state);

FAIL_IF(ssl_state->client_connp.content_type != 0x16);

FAIL_IF(ssl_state->client_connp.version != TLS_VERSION_10);

/* do detect */
SigMatchSignatures(&th_v, de_ctx, det_ctx, p);

FAIL_IF_NOT(PacketAlertCheck(p, 1));

AppLayerParserThreadCtxFree(alp_tctx);
SigGroupCleanup(de_ctx);
SigCleanSignatures(de_ctx);

DetectEngineThreadCtxDeinit(&th_v, (void *)det_ctx);
DetectEngineCtxFree(de_ctx);

StreamTcpFreeConfig(true);
FLOW_DESTROY(&f);

UTHFreePackets(&p, 1);

PASS;
}

/**
* \brief this function registers unit tests for DetectTlsVersion
*/
static void DetectTlsVersionRegisterTests(void)
{
UtRegisterTest("DetectTlsVersionTestParse01", DetectTlsVersionTestParse01);
UtRegisterTest("DetectTlsVersionTestParse02", DetectTlsVersionTestParse02);
UtRegisterTest("DetectTlsVersionTestParse03", DetectTlsVersionTestParse03);
UtRegisterTest("DetectTlsVersionTestDetect01",
DetectTlsVersionTestDetect01);
UtRegisterTest("DetectTlsVersionTestDetect02",
DetectTlsVersionTestDetect02);
UtRegisterTest("DetectTlsVersionTestDetect03", DetectTlsVersionTestDetect03);
}