From 8d20b40cdd3c8e911b0c4b06fb4fdc999b2d5c7c Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 9 Jun 2022 22:25:44 +0200 Subject: [PATCH] detect/content: fix FNs due to bad depth calc When trying to propegate the depth/offset, within/distance chains a logic error would set too a restrictive depth on a pattern that followed more than one "unchained" patterns. Bug: #5162. --- src/detect-content.c | 121 ++++++++++++++++++++++++------------------- 1 file changed, 67 insertions(+), 54 deletions(-) diff --git a/src/detect-content.c b/src/detect-content.c index e491b47414..b457a83a99 100644 --- a/src/detect-content.c +++ b/src/detect-content.c @@ -456,14 +456,12 @@ void DetectContentPropagateLimits(Signature *s) uint16_t offset = 0; uint16_t offset_plus_pat = 0; uint16_t depth = 0; - bool last_reset = false; // TODO really last reset 'depth' + bool has_active_depth_chain = false; bool has_depth = false; bool has_ends_with = false; uint16_t ends_with_depth = 0; - bool have_anchor = false; - SigMatch *sm = s->init_data->smlists[list]; for ( ; sm != NULL; sm = sm->next) { switch (sm->type) { @@ -473,32 +471,30 @@ void DetectContentPropagateLimits(Signature *s) offset = depth = 0; offset_plus_pat = cd->content_len; SCLogDebug("reset"); - last_reset = true; - have_anchor = false; + has_active_depth_chain = false; continue; } if (cd->flags & DETECT_CONTENT_NEGATED) { offset = depth = 0; offset_plus_pat = 0; SCLogDebug("reset because of negation"); - last_reset = true; - have_anchor = false; + has_active_depth_chain = false; continue; } if (cd->depth) { has_depth = true; - have_anchor = true; + has_active_depth_chain = true; } SCLogDebug("sm %p depth %u offset %u distance %d within %d", sm, cd->depth, cd->offset, cd->distance, cd->within); - SCLogDebug("stored: offset %u depth %u offset_plus_pat %u", offset, depth, offset_plus_pat); - if ((cd->flags & DETECT_CONTENT_WITHIN) == 0) { + if ((cd->flags & (DETECT_DEPTH | DETECT_CONTENT_WITHIN)) == 0) { if (depth) SCLogDebug("no within, reset depth"); depth = 0; + has_active_depth_chain = false; } if ((cd->flags & DETECT_CONTENT_DISTANCE) == 0) { if (offset_plus_pat) @@ -506,51 +502,58 @@ void DetectContentPropagateLimits(Signature *s) offset_plus_pat = offset = 0; } - SCLogDebug("stored: offset %u depth %u offset_plus_pat %u", offset, depth, offset_plus_pat); - + SCLogDebug("stored: offset %u depth %u offset_plus_pat %u " + "has_active_depth_chain %s", + offset, depth, offset_plus_pat, + has_active_depth_chain ? "true" : "false"); if (cd->flags & DETECT_CONTENT_DISTANCE && cd->distance >= 0) { VALIDATE((uint32_t)offset_plus_pat + cd->distance <= UINT16_MAX); offset = cd->offset = (uint16_t)(offset_plus_pat + cd->distance); SCLogDebug("updated content to have offset %u", cd->offset); } - if (have_anchor && !last_reset && offset_plus_pat && cd->flags & DETECT_CONTENT_WITHIN && cd->within >= 0) { - if (depth && depth > offset_plus_pat) { - int32_t dist = 0; - if (cd->flags & DETECT_CONTENT_DISTANCE && cd->distance > 0) { - dist = cd->distance; - SCLogDebug("distance to add: %u. depth + dist %u", dist, depth + dist); - } - SCLogDebug("depth %u + cd->within %u", depth, cd->within); - VALIDATE(depth + cd->within + dist >= 0 && - depth + cd->within + dist <= UINT16_MAX); - depth = cd->depth = (uint16_t)(depth + cd->within + dist); - } else { - SCLogDebug("offset %u + cd->within %u", offset, cd->within); - VALIDATE(depth + cd->within >= 0 && depth + cd->within <= UINT16_MAX); - depth = cd->depth = (uint16_t)(offset + cd->within); - } - SCLogDebug("updated content to have depth %u", cd->depth); - } else { - if (cd->depth == 0 && depth != 0) { - if (cd->within > 0) { - SCLogDebug("within %d distance %d", cd->within, cd->distance); - if (cd->flags & DETECT_CONTENT_DISTANCE && cd->distance >= 0) { - VALIDATE(offset_plus_pat + cd->distance >= 0 && - offset_plus_pat + cd->distance <= UINT16_MAX); - cd->offset = (uint16_t)(offset_plus_pat + cd->distance); - SCLogDebug("updated content to have offset %u", cd->offset); + if (has_active_depth_chain) { + if (offset_plus_pat && cd->flags & DETECT_CONTENT_WITHIN && + cd->within >= 0) { + if (depth && depth > offset_plus_pat) { + int32_t dist = 0; + if (cd->flags & DETECT_CONTENT_DISTANCE && cd->distance > 0) { + dist = cd->distance; + SCLogDebug("distance to add: %u. depth + dist %u", dist, + depth + dist); } - + SCLogDebug("depth %u + cd->within %u", depth, cd->within); + VALIDATE(depth + cd->within + dist >= 0 && + depth + cd->within + dist <= UINT16_MAX); + depth = cd->depth = (uint16_t)(depth + cd->within + dist); + } else { + SCLogDebug("offset %u + cd->within %u", offset, cd->within); VALIDATE(depth + cd->within >= 0 && depth + cd->within <= UINT16_MAX); - depth = cd->depth = (uint16_t)(cd->within + depth); - SCLogDebug("updated content to have depth %u", cd->depth); - - if (cd->flags & DETECT_CONTENT_ENDS_WITH) { - has_ends_with = true; - if (ends_with_depth == 0) - ends_with_depth = depth; - ends_with_depth = MIN(ends_with_depth, depth); + depth = cd->depth = (uint16_t)(offset + cd->within); + } + SCLogDebug("updated content to have depth %u", cd->depth); + } else { + if (cd->depth == 0 && depth != 0) { + if (cd->within > 0) { + SCLogDebug("within %d distance %d", cd->within, cd->distance); + if (cd->flags & DETECT_CONTENT_DISTANCE && cd->distance >= 0) { + VALIDATE(offset_plus_pat + cd->distance >= 0 && + offset_plus_pat + cd->distance <= UINT16_MAX); + cd->offset = (uint16_t)(offset_plus_pat + cd->distance); + SCLogDebug("updated content to have offset %u", cd->offset); + } + + VALIDATE(depth + cd->within >= 0 && + depth + cd->within <= UINT16_MAX); + depth = cd->depth = (uint16_t)(cd->within + depth); + SCLogDebug("updated content to have depth %u", cd->depth); + + if (cd->flags & DETECT_CONTENT_ENDS_WITH) { + has_ends_with = true; + if (ends_with_depth == 0) + ends_with_depth = depth; + ends_with_depth = MIN(ends_with_depth, depth); + } } } } @@ -589,30 +592,27 @@ void DetectContentPropagateLimits(Signature *s) } } if ((cd->flags & (DETECT_CONTENT_WITHIN|DETECT_CONTENT_DEPTH)) == 0) { - last_reset = true; + has_active_depth_chain = false; depth = 0; - } else { - last_reset = false; } break; } case DETECT_PCRE: { // relative could leave offset_plus_pat set. - DetectPcreData *pd = (DetectPcreData *)sm->ctx; + const DetectPcreData *pd = (const DetectPcreData *)sm->ctx; if (pd->flags & DETECT_PCRE_RELATIVE) { depth = 0; - last_reset = true; } else { SCLogDebug("non-anchored PCRE not supported, reset offset_plus_pat & offset"); offset_plus_pat = offset = depth = 0; - last_reset = true; } + has_active_depth_chain = false; break; } default: { SCLogDebug("keyword not supported, reset offset_plus_pat & offset"); offset_plus_pat = offset = depth = 0; - last_reset = true; + has_active_depth_chain = false; break; } } @@ -780,6 +780,19 @@ static int DetectContentDepthTest01(void) // distance value is too high so we bail and not set anything on this content TEST_RUN("content:\"0123456789\"; content:\"abcdef\"; distance:2147483647;", 0, 0); + // Bug #5162. + TEST_RUN("content:\"SMB\"; depth:8; content:\"|09 00|\"; distance:8; within:2;", 11, 18); + TEST_RUN("content:\"SMB\"; depth:8; content:\"|09 00|\"; distance:8; within:2; content:\"|05 " + "00 00|\"; distance:0;", + 13, 0); + TEST_RUN("content:\"SMB\"; depth:8; content:\"|09 00|\"; distance:8; within:2; content:\"|05 " + "00 00|\"; distance:0; content:\"|0c 00|\"; distance:19; within:2;", + 35, 0); + TEST_RUN("content:\"SMB\"; depth:8; content:\"|09 00|\"; distance:8; within:2; content:\"|05 " + "00 00|\"; distance:0; content:\"|0c 00|\"; distance:19; within:2; content:\"|15 00 " + "00 00|\"; distance:20; within:4;", + 57, 0); + TEST_DONE; }