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

6.0.x: content: don't error out on incomplete hex - v1 #7885

Closed
wants to merge 1 commit into from
Closed

6.0.x: content: don't error out on incomplete hex - v1 #7885

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
109 changes: 82 additions & 27 deletions src/detect-content.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
* Simple content match part of the detection engine.
*/

#include "detect-engine-register.h"
#include "suricata-common.h"
#include "decode.h"
#include "detect.h"
Expand Down Expand Up @@ -79,8 +80,8 @@ void DetectContentRegister (void)
* \retval -1 error
* \retval 0 ok
*/
int DetectContentDataParse(const char *keyword, const char *contentstr,
uint8_t **pstr, uint16_t *plen)
int DetectContentDataParse(
const char *keyword, const char *contentstr, uint8_t **pstr, uint16_t *plen, bool *warning)
Copy link
Member Author

Choose a reason for hiding this comment

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

An output var just seemed more appropriate here than changing the return value.

{
char *str = NULL;
size_t slen = 0;
Expand Down Expand Up @@ -112,9 +113,18 @@ int DetectContentDataParse(const char *keyword, const char *contentstr,
bin_count++;
if (bin) {
if (binpos > 0) {
SCLogError(SC_ERR_INVALID_SIGNATURE,
"Incomplete hex code in content - %s. Invalidating signature.",
SCLogWarning(SC_ERR_INVALID_SIGNATURE,
Copy link
Member

Choose a reason for hiding this comment

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

I think this should only be a warning if we're not in strict mode. If we are in strict mode it should be an error like it was.

"Incomplete hex code in content - %s. Invalidating signature. "
"This will become an error in 7.0.",
contentstr);
/* Prior to 6.0.6, incomplete hex was silently
ignored. With 6.0.6 this turned into an
error with -T. For 6.0.7, make the error
non-fatal unless strict content parsing is
enabled. */
if (warning != NULL && !SigMatchStrictEnabled(DETECT_CONTENT)) {
*warning = true;
}
goto error;
}
bin = 0;
Expand Down Expand Up @@ -202,20 +212,21 @@ int DetectContentDataParse(const char *keyword, const char *contentstr,
error:
return -1;
}

/**
* \brief DetectContentParse
* \initonly
*/
DetectContentData *DetectContentParse(SpmGlobalThreadCtx *spm_global_thread_ctx,
const char *contentstr)
DetectContentData *DetectContentParse(
SpmGlobalThreadCtx *spm_global_thread_ctx, const char *contentstr, bool *warning)
{
DetectContentData *cd = NULL;
uint8_t *content = NULL;
uint16_t len = 0;
int ret;

ret = DetectContentDataParse("content", contentstr, &content, &len);
if (ret == -1) {
ret = DetectContentDataParse("content", contentstr, &content, &len, warning);
if (ret < 0) {
return NULL;
}

Expand Down Expand Up @@ -253,7 +264,7 @@ DetectContentData *DetectContentParse(SpmGlobalThreadCtx *spm_global_thread_ctx,
DetectContentData *DetectContentParseEncloseQuotes(SpmGlobalThreadCtx *spm_global_thread_ctx,
const char *contentstr)
{
return DetectContentParse(spm_global_thread_ctx, contentstr);
return DetectContentParse(spm_global_thread_ctx, contentstr, NULL);
}

/**
Expand Down Expand Up @@ -329,8 +340,9 @@ int DetectContentSetup(DetectEngineCtx *de_ctx, Signature *s, const char *conten
{
DetectContentData *cd = NULL;
SigMatch *sm = NULL;
bool warning = false;

cd = DetectContentParse(de_ctx->spm_global_thread_ctx, contentstr);
cd = DetectContentParse(de_ctx->spm_global_thread_ctx, contentstr, &warning);
if (cd == NULL)
goto error;
if (s->init_data->negated == true) {
Expand Down Expand Up @@ -369,6 +381,8 @@ int DetectContentSetup(DetectEngineCtx *de_ctx, Signature *s, const char *conten

error:
DetectContentFree(de_ctx, cd);
if (warning)
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

will this not load the rule, but minus the broken content condition?

so effectively turning
alert ... content:"|aa a aa|"; dsize:10;
into
alert ... dsize:10;
?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we should either return -2 or -3 here (not sure which though):

    int ret = SigParse(de_ctx, sig, sigstr, dir, &parser);                                               
    if (ret == -3) {                                                                                     
        de_ctx->sigerror_silent = true;                                                                  
        de_ctx->sigerror_ok = true;                                                                      
        goto error;                                                                                      
    }                                                                                                    
    else if (ret == -2) {                                                                                
        de_ctx->sigerror_silent = true;                                                                  
        goto error;                                                                                      
    } else if (ret < 0) {                                                                                
        goto error;                                                                                      
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, its a -4... New.. Log the error, but don't error out. An option we don't have it. Which handles the previous case as well.

ie)

        de_ctx->sigerror_ok = true;

return -1;
}

Expand Down Expand Up @@ -796,7 +810,7 @@ static int DetectContentParseTest01 (void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd != NULL) {
if (memcmp(cd->content, teststringparsed, strlen(teststringparsed)) != 0) {
SCLogDebug("expected %s got ", teststringparsed);
Expand Down Expand Up @@ -827,7 +841,7 @@ static int DetectContentParseTest02 (void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd != NULL) {
if (memcmp(cd->content, teststringparsed, strlen(teststringparsed)) != 0) {
SCLogDebug("expected %s got ", teststringparsed);
Expand Down Expand Up @@ -858,7 +872,7 @@ static int DetectContentParseTest03 (void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd != NULL) {
if (memcmp(cd->content, teststringparsed, strlen(teststringparsed)) != 0) {
SCLogDebug("expected %s got ", teststringparsed);
Expand Down Expand Up @@ -889,7 +903,7 @@ static int DetectContentParseTest04 (void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd != NULL) {
uint16_t len = (cd->content_len > strlen(teststringparsed));
if (memcmp(cd->content, teststringparsed, len) != 0) {
Expand Down Expand Up @@ -920,7 +934,7 @@ static int DetectContentParseTest05 (void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd != NULL) {
SCLogDebug("expected NULL got ");
PrintRawUriFp(stdout,cd->content,cd->content_len);
Expand All @@ -946,7 +960,7 @@ static int DetectContentParseTest06 (void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd != NULL) {
uint16_t len = (cd->content_len > strlen(teststringparsed));
if (memcmp(cd->content, teststringparsed, len) != 0) {
Expand Down Expand Up @@ -977,7 +991,7 @@ static int DetectContentParseTest07 (void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd != NULL) {
SCLogDebug("expected NULL got %p: ", cd);
result = 0;
Expand All @@ -1000,7 +1014,7 @@ static int DetectContentParseTest08 (void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd != NULL) {
SCLogDebug("expected NULL got %p: ", cd);
result = 0;
Expand Down Expand Up @@ -1288,7 +1302,7 @@ static int DetectContentParseTest09(void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
FAIL_IF_NULL(cd);
DetectContentFree(NULL, cd);
SpmDestroyGlobalThreadCtx(spm_global_thread_ctx);
Expand Down Expand Up @@ -2373,7 +2387,7 @@ static int DetectContentParseTest41(void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd == NULL) {
SCLogDebug("expected not NULL");
result = 0;
Expand Down Expand Up @@ -2406,7 +2420,7 @@ static int DetectContentParseTest42(void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd == NULL) {
SCLogDebug("expected not NULL");
result = 0;
Expand Down Expand Up @@ -2440,7 +2454,7 @@ static int DetectContentParseTest43(void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd == NULL) {
SCLogDebug("expected not NULL");
result = 0;
Expand Down Expand Up @@ -2477,7 +2491,7 @@ static int DetectContentParseTest44(void)
SpmGlobalThreadCtx *spm_global_thread_ctx = SpmInitGlobalThreadCtx(spm_matcher);
FAIL_IF(spm_global_thread_ctx == NULL);

cd = DetectContentParse(spm_global_thread_ctx, teststring);
cd = DetectContentParse(spm_global_thread_ctx, teststring, NULL);
if (cd == NULL) {
SCLogDebug("expected not NULL");
result = 0;
Expand Down Expand Up @@ -3030,8 +3044,26 @@ static int DetectLongContentTest3(void)
return !DetectLongContentTestCommon(sig, 1);
}

static int DetectBadBinContent(void)
static int DetectIncompleteHexNonStrict(void)
{
DetectEngineCtx *de_ctx = NULL;
de_ctx = DetectEngineCtxInit();
FAIL_IF_NULL(de_ctx);
de_ctx->flags |= DE_QUIET;
FAIL_IF_NULL(DetectEngineAppendSig(
de_ctx, "alert tcp any any -> any any (msg:\"test\"; content:\"|a|\"; sid:1;)"));
FAIL_IF_NULL(DetectEngineAppendSig(
de_ctx, "alert tcp any any -> any any (msg:\"test\"; content:\"|aa b|\"; sid:2;)"));
/* https://redmine.openinfosecfoundation.org/issues/5201 */
FAIL_IF_NULL(DetectEngineAppendSig(
de_ctx, "alert tcp any any -> any any (msg:\"test\"; content:\"|22 2 22|\"; sid:3;)"));
DetectEngineCtxFree(de_ctx);
PASS;
}

static int DetectIncompleteHexStrict(void)
{
SigTableApplyStrictCommandlineOption("content");
DetectEngineCtx *de_ctx = NULL;
de_ctx = DetectEngineCtxInit();
FAIL_IF_NULL(de_ctx);
Expand All @@ -3040,15 +3072,35 @@ static int DetectBadBinContent(void)
de_ctx, "alert tcp any any -> any any (msg:\"test\"; content:\"|a|\"; sid:1;)"));
FAIL_IF_NOT_NULL(DetectEngineAppendSig(
de_ctx, "alert tcp any any -> any any (msg:\"test\"; content:\"|aa b|\"; sid:1;)"));
FAIL_IF_NOT_NULL(DetectEngineAppendSig(
de_ctx, "alert tcp any any -> any any (msg:\"test\"; content:\"|aa bz|\"; sid:1;)"));
/* https://redmine.openinfosecfoundation.org/issues/5201 */
FAIL_IF_NOT_NULL(DetectEngineAppendSig(
de_ctx, "alert tcp any any -> any any (msg:\"test\"; content:\"|22 2 22|\"; sid:1;)"));
DetectEngineCtxFree(de_ctx);
PASS;
}

/**
* This is a bit of hack, to cleanup from DetectIncompleteHexStrict if
* it fails, to avoid causing other tests to fail.
*/
static int DetectIncompleteHexStrictCleanup(void)
{
SigTableClearStrictOption("content");
PASS;
}

static int DetectBadHexContent(void)
{
DetectEngineCtx *de_ctx = NULL;
de_ctx = DetectEngineCtxInit();
FAIL_IF_NULL(de_ctx);
de_ctx->flags |= DE_QUIET;
FAIL_IF_NOT_NULL(DetectEngineAppendSig(
de_ctx, "alert tcp any any -> any any (msg:\"test\"; content:\"|aa bz|\"; sid:1;)"));
DetectEngineCtxFree(de_ctx);
PASS;
}

/**
* \brief this function registers unit tests for DetectContent
*/
Expand Down Expand Up @@ -3168,6 +3220,9 @@ static void DetectContentRegisterTests(void)
UtRegisterTest("DetectLongContentTest2", DetectLongContentTest2);
UtRegisterTest("DetectLongContentTest3", DetectLongContentTest3);

UtRegisterTest("DetectBadBinContent", DetectBadBinContent);
UtRegisterTest("DetectBadHexContent", DetectBadHexContent);
UtRegisterTest("DetectIncompleteHexNonStrict", DetectIncompleteHexNonStrict);
UtRegisterTest("DetectIncompleteHexStrict", DetectIncompleteHexStrict);
UtRegisterTest("DetectIncompleteHexStrictCleanup", DetectIncompleteHexStrictCleanup);
}
#endif /* UNITTESTS */
8 changes: 4 additions & 4 deletions src/detect-content.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,10 @@ typedef struct DetectContentData_ {
/* prototypes */
void DetectContentRegister (void);
uint32_t DetectContentMaxId(DetectEngineCtx *);
DetectContentData *DetectContentParse(SpmGlobalThreadCtx *spm_global_thread_ctx,
const char *contentstr);
int DetectContentDataParse(const char *keyword, const char *contentstr,
uint8_t **pstr, uint16_t *plen);
DetectContentData *DetectContentParse(
SpmGlobalThreadCtx *spm_global_thread_ctx, const char *contentstr, bool *warning);
int DetectContentDataParse(
const char *keyword, const char *contentstr, uint8_t **pstr, uint16_t *plen, bool *warning);
DetectContentData *DetectContentParseEncloseQuotes(SpmGlobalThreadCtx *spm_global_thread_ctx,
const char *contentstr);

Expand Down
2 changes: 1 addition & 1 deletion src/detect-fileext.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ static DetectFileextData *DetectFileextParse (DetectEngineCtx *de_ctx, const cha

memset(fileext, 0x00, sizeof(DetectFileextData));

if (DetectContentDataParse("fileext", str, &fileext->ext, &fileext->len) == -1) {
if (DetectContentDataParse("fileext", str, &fileext->ext, &fileext->len, NULL) == -1) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess these other users of content parsing should respect the same behaviour.

goto error;
}
uint16_t u;
Expand Down
2 changes: 1 addition & 1 deletion src/detect-filemagic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ static DetectFilemagicData *DetectFilemagicParse (DetectEngineCtx *de_ctx, const

memset(filemagic, 0x00, sizeof(DetectFilemagicData));

if (DetectContentDataParse ("filemagic", str, &filemagic->name, &filemagic->len) == -1) {
if (DetectContentDataParse("filemagic", str, &filemagic->name, &filemagic->len, NULL) == -1) {
goto error;
}

Expand Down
2 changes: 1 addition & 1 deletion src/detect-filename.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ static DetectFilenameData *DetectFilenameParse (DetectEngineCtx *de_ctx, const c

memset(filename, 0x00, sizeof(DetectFilenameData));

if (DetectContentDataParse ("filename", str, &filename->name, &filename->len) == -1) {
if (DetectContentDataParse("filename", str, &filename->name, &filename->len, NULL) == -1) {
goto error;
}

Expand Down
3 changes: 2 additions & 1 deletion src/detect-flowvar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,8 @@ static int DetectFlowvarSetup (DetectEngineCtx *de_ctx, Signature *s, const char
}
SCLogDebug("varcontent %s", &varcontent[varcontent_index]);

res = DetectContentDataParse("flowvar", &varcontent[varcontent_index], &content, &contentlen);
res = DetectContentDataParse(
"flowvar", &varcontent[varcontent_index], &content, &contentlen, NULL);
if (res == -1)
goto error;

Expand Down
8 changes: 8 additions & 0 deletions src/detect-parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,14 @@ void SigTableApplyStrictCommandlineOption(const char *str)
SCFree(copy);
}

void SigTableClearStrictOption(const char *key)
{
SigTableElmt *st = SigTableGet((char *)key);
if (st != NULL) {
st->flags &= ~SIGMATCH_STRICT_PARSING;
}
}

/**
* \brief Append a SigMatch to the list type.
*
Expand Down
1 change: 1 addition & 0 deletions src/detect-parse.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ const char *DetectListToHumanString(int list);
const char *DetectListToString(int list);

void SigTableApplyStrictCommandlineOption(const char *str);
void SigTableClearStrictOption(const char *key);

SigMatch *DetectGetLastSM(const Signature *);
SigMatch *DetectGetLastSMFromMpmLists(const DetectEngineCtx *de_ctx, const Signature *s);
Expand Down
2 changes: 1 addition & 1 deletion src/detect-pktvar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ static int DetectPktvarSetup (DetectEngineCtx *de_ctx, Signature *s, const char
parse_content = varcontent;
}

ret = DetectContentDataParse("pktvar", parse_content, &content, &len);
ret = DetectContentDataParse("pktvar", parse_content, &content, &len, NULL);
if (ret == -1 || content == NULL) {
pcre_free(varname);
pcre_free(varcontent);
Expand Down
4 changes: 2 additions & 2 deletions src/detect-replace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ int DetectReplaceSetup(DetectEngineCtx *de_ctx, Signature *s, const char *replac
return 0;
}

int ret = DetectContentDataParse("replace", replacestr, &content, &len);
int ret = DetectContentDataParse("replace", replacestr, &content, &len, NULL);
if (ret == -1)
return -1;

Expand Down Expand Up @@ -862,4 +862,4 @@ void DetectReplaceRegisterTests(void)
UtRegisterTest("DetectReplaceParseTest06", DetectReplaceParseTest06);
UtRegisterTest("DetectReplaceParseTest07", DetectReplaceParseTest07);
}
#endif /* UNITTESTS */
#endif /* UNITTESTS */