Skip to content

Commit

Permalink
detect/pcre: fix false positive
Browse files Browse the repository at this point in the history 
Fix case where a HTTP modifier in PCRE statements would lead to
the rule alerting when it should not.

Bug OISF#2769
  • Loading branch information
@victorjulien
victorjulien committed Feb 20, 2019
1 parent 43698a9 commit 2bd23bc
Showing 1 changed file with 59 additions and 19 deletions.
78 changes: 59 additions & 19 deletions src/detect-pcre.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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': {
Expand All @@ -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': {
Expand All @@ -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;
}
Expand All @@ -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 */
Expand All @@ -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) {
Expand All @@ -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 */
Expand All @@ -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 */
Expand All @@ -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:
Expand Down Expand Up @@ -661,7 +675,6 @@ static DetectPcreData *DetectPcreParse (DetectEngineCtx *de_ctx, const char *reg
} else {
goto error;
}

return pd;

error:
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 2bd23bc

Please sign in to comment.