6.0.x: content: don't error out on incomplete hex - v1 #7885
Conversation
Before 6.0.6 if hex content was incomplete, Suricata didn't error out. With 6.0.6 incomplete hex was detected and errored on which is a breaking change in a release branch. Instead, only emit a warning unless strict content checking has been requested. To enable strict behaviour on incomplete content hex in a rule, "--strict-rule-keywords=content" can be used on the command line. Issue: OISF#5546
| 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) |
There was a problem hiding this comment.
An output var just seemed more appropriate here than changing the return value.
| @@ -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) { | |||
There was a problem hiding this comment.
I guess these other users of content parsing should respect the same behaviour.
|
Another issue is that when not strict, you have no context what rule is causing the error. The similar error for classtype has access to |
| @@ -369,6 +381,8 @@ int DetectContentSetup(DetectEngineCtx *de_ctx, Signature *s, const char *conten | |||
|
|
|||
| error: | |||
| DetectContentFree(de_ctx, cd); | |||
| if (warning) | |||
| return 0; | |||
There was a problem hiding this comment.
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;
?
There was a problem hiding this comment.
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;
} There was a problem hiding this comment.
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;
| @@ -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, | |||
There was a problem hiding this comment.
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.
|
Updated at #7887. |
Before 6.0.6 if hex content was incomplete, Suricata didn't error out.
With 6.0.6 incomplete hex was detected and errored on which is a
breaking change in a release branch. Instead, only emit a warning
unless strict content checking has been requested.
To enable strict behaviour on incomplete content hex in a rule,
"--strict-rule-keywords=content" can be used on the command line.
Issue: https://redmine.openinfosecfoundation.org/issues/5546
For now there is no accompanying master commit, as the change in
behaviour for a major version is OK.