From: Victor Julien Date: Tue, 4 Mar 2014 16:49:36 +0000 (+0100) Subject: Fix False Positive of rules with ports on portless protocols X-Git-Tag: suricata-2.0rc2~11 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F872%2Fhead;p=thirdparty%2Fsuricata.git Fix False Positive of rules with ports on portless protocols In case of 'alert ip' rules that have ports, the port checks would be bypassed for non-port protocols, such as ICMP. This would lead to a rule matching: a false positive. This patch adds a check. If the rule has a port setting other than 'any' and the protocol is not TCP, UDP or SCTP, then we rule won't match. Rules with 'alert ip' and ports are rare, so the impact should be minimal. Bug #611. --- diff --git a/src/detect.c b/src/detect.c index 323188d267..d5bcab1a46 100644 --- a/src/detect.c +++ b/src/detect.c @@ -1367,6 +1367,9 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh goto next; } } + } else if ((s->flags & (SIG_FLAG_DP_ANY|SIG_FLAG_SP_ANY)) != (SIG_FLAG_DP_ANY|SIG_FLAG_SP_ANY)) { + SCLogDebug("port-less protocol and sig needs ports"); + goto next; } /* check the destination address */ @@ -11296,6 +11299,58 @@ end: return result; } +/** \test ICMP packet shouldn't be matching port based sig + * Bug #611 */ +static int SigTestPorts01(void) +{ + int result = 0; + Packet *p1 = NULL; + Signature *s = NULL; + ThreadVars tv; + DetectEngineThreadCtx *det_ctx = NULL; + uint8_t payload[] = "AAAAAAAAAAAAAAAAAA"; + + memset(&tv, 0, sizeof(ThreadVars)); + + p1 = UTHBuildPacket(payload, sizeof(payload), IPPROTO_ICMP); + + DetectEngineCtx *de_ctx = DetectEngineCtxInit(); + if (de_ctx == NULL) { + goto end; + } + de_ctx->mpm_matcher = MPM_B2G; + de_ctx->flags |= DE_QUIET; + + s = de_ctx->sig_list = SigInit(de_ctx, "alert ip any any -> any 80 " + "(content:\"AAA\"; sid:1;)"); + if (s == NULL) { + goto end; + } + + SigGroupBuild(de_ctx); + DetectEngineThreadCtxInit(&tv, (void *)de_ctx, (void *)&det_ctx); + + /* do detect */ + SigMatchSignatures(&tv, de_ctx, det_ctx, p1); + + if (PacketAlertCheck(p1, 1)) { + printf("sig 1 alerted on p1, but it should not: "); + goto end; + } + + result = 1; +end: + if (det_ctx != NULL) + DetectEngineThreadCtxDeinit(&tv, det_ctx); + if (de_ctx != NULL) + SigGroupCleanup(de_ctx); + if (de_ctx != NULL) + DetectEngineCtxFree(de_ctx); + + UTHFreePackets(&p1, 1); + return result; +} + static const char *dummy_conf_string2 = "%YAML 1.1\n" "---\n" @@ -11694,6 +11749,8 @@ void SigRegisterTests(void) { UtRegisterTest("DetectAddressYamlParsing03", DetectAddressYamlParsing03, 1); UtRegisterTest("DetectAddressYamlParsing04", DetectAddressYamlParsing04, 1); + UtRegisterTest("SigTestPorts01", SigTestPorts01, 1); + DetectSimdRegisterTests(); #endif /* UNITTESTS */ }