From 2bd23bc1d511801469d4736ef85efeb373992411 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 20 Feb 2019 16:58:34 +0100 Subject: [PATCH] detect/pcre: fix false positive Fix case where a HTTP modifier in PCRE statements would lead to the rule alerting when it should not. Bug #2769 --- src/detect-pcre.c | 78 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 59 insertions(+), 19 deletions(-) diff --git a/src/detect-pcre.c b/src/detect-pcre.c index ddd4b0711819..64589e71cd16 100644 --- a/src/detect-pcre.c +++ b/src/detect-pcre.c @@ -324,8 +324,9 @@ static int DetectPcreHasUpperCase(const char *re) return 0; } -static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *regexstr, int *sm_list, - char *capture_names, size_t capture_names_size, bool negate) +static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, + const char *regexstr, int *sm_list, char *capture_names, + size_t capture_names_size, bool negate, AppProto *alproto) { int ec; const char *eb; @@ -466,6 +467,7 @@ static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *reg } int list = DetectBufferTypeGetByName("http_uri"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'V': { @@ -475,6 +477,7 @@ static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *reg } int list = DetectBufferTypeGetByName("http_user_agent"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'W': { @@ -484,6 +487,7 @@ static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *reg } int list = DetectBufferTypeGetByName("http_host"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; check_host_header = 1; break; } @@ -494,6 +498,7 @@ static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *reg } int list = DetectBufferTypeGetByName("http_raw_host"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'H': { /* snort's option */ @@ -503,6 +508,7 @@ static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *reg } int list = DetectBufferTypeGetByName("http_header"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'I': { /* snort's option */ if (pd->flags & DETECT_PCRE_RAWBYTES) { @@ -511,11 +517,13 @@ static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *reg } int list = DetectBufferTypeGetByName("http_raw_uri"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'D': { /* snort's option */ int list = DetectBufferTypeGetByName("http_raw_header"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'M': { /* snort's option */ @@ -525,6 +533,7 @@ static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *reg } int list = DetectBufferTypeGetByName("http_method"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'C': { /* snort's option */ @@ -534,30 +543,35 @@ static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *reg } int list = DetectBufferTypeGetByName("http_cookie"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'P': { /* snort's option (http request body inspection) */ int list = DetectBufferTypeGetByName("http_client_body"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'Q': { int list = DetectBufferTypeGetByName("file_data"); /* suricata extension (http response body inspection) */ *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'Y': { /* snort's option */ int list = DetectBufferTypeGetByName("http_stat_msg"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } case 'S': { /* snort's option */ int list = DetectBufferTypeGetByName("http_stat_code"); *sm_list = DetectPcreSetList(*sm_list, list); + *alproto = ALPROTO_HTTP; break; } default: @@ -661,7 +675,6 @@ static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *reg } else { goto error; } - return pd; error: @@ -817,9 +830,11 @@ static int DetectPcreSetup (DetectEngineCtx *de_ctx, Signature *s, const char *r int ret = -1; int parsed_sm_list = DETECT_SM_LIST_NOTSET; char capture_names[1024] = ""; + AppProto alproto = ALPROTO_UNKNOWN; pd = DetectPcreParse(de_ctx, regexstr, &parsed_sm_list, - capture_names, sizeof(capture_names), s->init_data->negated); + capture_names, sizeof(capture_names), s->init_data->negated, + &alproto); if (pd == NULL) goto error; if (DetectPcreParseCapture(regexstr, de_ctx, pd, capture_names) < 0) @@ -837,9 +852,19 @@ static int DetectPcreSetup (DetectEngineCtx *de_ctx, Signature *s, const char *r case DETECT_SM_LIST_NOTSET: sm_list = DETECT_SM_LIST_PMATCH; break; - default: + default: { + if (alproto != ALPROTO_UNKNOWN) { + /* see if the proto doesn't conflict + * with what we already have. */ + if (s->alproto != ALPROTO_UNKNOWN && + alproto != s->alproto) { + goto error; + } + s->alproto = alproto; + } sm_list = parsed_sm_list; break; + } } } if (sm_list == -1) @@ -921,8 +946,9 @@ static int DetectPcreParseTest01 (void) int list = DETECT_SM_LIST_NOTSET; DetectEngineCtx *de_ctx = DetectEngineCtxInit(); FAIL_IF_NULL(de_ctx); + AppProto alproto = ALPROTO_UNKNOWN; - pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false, &alproto); FAIL_IF_NOT_NULL(pd); DetectEngineCtxFree(de_ctx); @@ -940,9 +966,11 @@ static int DetectPcreParseTest02 (void) int list = DETECT_SM_LIST_NOTSET; DetectEngineCtx *de_ctx = DetectEngineCtxInit(); FAIL_IF_NULL(de_ctx); + AppProto alproto = ALPROTO_UNKNOWN; - pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false, &alproto); FAIL_IF_NOT_NULL(pd); + FAIL_IF_NOT(alproto == ALPROTO_HTTP); DetectEngineCtxFree(de_ctx); return result; @@ -959,8 +987,9 @@ static int DetectPcreParseTest03 (void) int list = DETECT_SM_LIST_NOTSET; DetectEngineCtx *de_ctx = DetectEngineCtxInit(); FAIL_IF_NULL(de_ctx); + AppProto alproto = ALPROTO_UNKNOWN; - pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false, &alproto); FAIL_IF_NOT_NULL(pd); DetectEngineCtxFree(de_ctx); @@ -978,9 +1007,11 @@ static int DetectPcreParseTest04 (void) int list = DETECT_SM_LIST_NOTSET; DetectEngineCtx *de_ctx = DetectEngineCtxInit(); FAIL_IF_NULL(de_ctx); + AppProto alproto = ALPROTO_UNKNOWN; - pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false, &alproto); FAIL_IF_NULL(pd); + FAIL_IF_NOT(alproto == ALPROTO_UNKNOWN); DetectPcreFree(pd); DetectEngineCtxFree(de_ctx); @@ -998,9 +1029,11 @@ static int DetectPcreParseTest05 (void) int list = DETECT_SM_LIST_NOTSET; DetectEngineCtx *de_ctx = DetectEngineCtxInit(); FAIL_IF_NULL(de_ctx); + AppProto alproto = ALPROTO_UNKNOWN; - pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false, &alproto); FAIL_IF_NULL(pd); + FAIL_IF_NOT(alproto == ALPROTO_UNKNOWN); DetectPcreFree(pd); DetectEngineCtxFree(de_ctx); @@ -1018,9 +1051,11 @@ static int DetectPcreParseTest06 (void) int list = DETECT_SM_LIST_NOTSET; DetectEngineCtx *de_ctx = DetectEngineCtxInit(); FAIL_IF_NULL(de_ctx); + AppProto alproto = ALPROTO_UNKNOWN; - pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false, &alproto); FAIL_IF_NULL(pd); + FAIL_IF_NOT(alproto == ALPROTO_UNKNOWN); DetectPcreFree(pd); DetectEngineCtxFree(de_ctx); @@ -1038,9 +1073,11 @@ static int DetectPcreParseTest07 (void) int list = DETECT_SM_LIST_NOTSET; DetectEngineCtx *de_ctx = DetectEngineCtxInit(); FAIL_IF_NULL(de_ctx); + AppProto alproto = ALPROTO_UNKNOWN; - pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false, &alproto); FAIL_IF_NULL(pd); + FAIL_IF_NOT(alproto == ALPROTO_HTTP); DetectPcreFree(pd); DetectEngineCtxFree(de_ctx); @@ -1058,9 +1095,11 @@ static int DetectPcreParseTest08 (void) int list = DETECT_SM_LIST_NOTSET; DetectEngineCtx *de_ctx = DetectEngineCtxInit(); FAIL_IF_NULL(de_ctx); + AppProto alproto = ALPROTO_UNKNOWN; - pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false, &alproto); FAIL_IF_NULL(pd); + FAIL_IF_NOT(alproto == ALPROTO_UNKNOWN); DetectPcreFree(pd); DetectEngineCtxFree(de_ctx); @@ -1078,8 +1117,9 @@ static int DetectPcreParseTest09 (void) int list = DETECT_SM_LIST_NOTSET; DetectEngineCtx *de_ctx = DetectEngineCtxInit(); FAIL_IF_NULL(de_ctx); + AppProto alproto = ALPROTO_UNKNOWN; - pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false, &alproto); FAIL_IF_NULL(pd); DetectPcreFree(pd); @@ -3422,30 +3462,30 @@ static int DetectPcreFlowvarCapture03(void) */ static int DetectPcreParseHttpHost(void) { - DetectPcreData *pd = NULL; + AppProto alproto = ALPROTO_UNKNOWN; int list = DETECT_SM_LIST_NOTSET; DetectEngineCtx *de_ctx = DetectEngineCtxInit(); FAIL_IF(de_ctx == NULL); - pd = DetectPcreParse(de_ctx, "/domain\\.com/W", &list, NULL, 0, false); + DetectPcreData *pd = DetectPcreParse(de_ctx, "/domain\\.com/W", &list, NULL, 0, false, &alproto); FAIL_IF(pd == NULL); DetectPcreFree(pd); list = DETECT_SM_LIST_NOTSET; - pd = DetectPcreParse(de_ctx, "/dOmain\\.com/W", &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, "/dOmain\\.com/W", &list, NULL, 0, false, &alproto); FAIL_IF(pd != NULL); /* Uppercase meta characters are valid. */ list = DETECT_SM_LIST_NOTSET; - pd = DetectPcreParse(de_ctx, "/domain\\D+\\.com/W", &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, "/domain\\D+\\.com/W", &list, NULL, 0, false, &alproto); FAIL_IF(pd == NULL); DetectPcreFree(pd); /* This should not parse as the first \ escapes the second \, then * we have a D. */ list = DETECT_SM_LIST_NOTSET; - pd = DetectPcreParse(de_ctx, "/\\\\Ddomain\\.com/W", &list, NULL, 0, false); + pd = DetectPcreParse(de_ctx, "/\\\\Ddomain\\.com/W", &list, NULL, 0, false, &alproto); FAIL_IF(pd != NULL); DetectEngineCtxFree(de_ctx);