Project

General

Profile

Actions

Bug #2224

open

Negated http_* match returns false if buffer not populated

Added by David Wharton about 7 years ago. Updated 7 months ago.

Status:
In Review
Priority:
Normal
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

If a rule has a negated content match for 'http_user_agent' buffer but the http_user_agent buffer isn't populated (i.e. the HTTP traffic doesn't have a "User-Agent" header), the negated content match will return false when it should be true. Example:

HTTP traffic:

GET /nouser-agent.html HTTP/1.1
Accept: */*

Rule:

alert http any any -> any any (msg:"User-Agent"; flow:established,to_server; content:"user-agent"; http_uri; content:!"doesnotexist"; http_user_agent; priority:2; sid:8675309;)

The above traffic does not have a User-Agent buffer so any negated content match in the http_user_agent buffer should return true. However, the above rule does not alert (unless the http_user_agent content match is removed).

Tested on Suricata 4.0.0, 3.2.3, 2.9.0, etc. This behavior applies to other http_* buffers too. e.g. http_host:

alert http any any -> any any (msg:"User-Agent"; flow:established,to_server; content:"user-agent"; http_uri; content:!"doesnotexist"; http_host; priority:2; sid:8675308;)

Maybe this behavior is "as designed" ... if so, can this bug report be turned in to a feature request?


Files

no_user-agent.pcap (422 Bytes) no_user-agent.pcap pcap David Wharton, 10/09/2017 10:48 AM
no_accept.pcap (619 Bytes) no_accept.pcap Brandon Murphy, 05/03/2023 02:36 PM
no_user_agent.pcap (633 Bytes) no_user_agent.pcap Brandon Murphy, 05/03/2023 02:36 PM
no_accept_language.pcap (626 Bytes) no_accept_language.pcap Brandon Murphy, 05/03/2023 02:36 PM
no_accept_encoding.pcap (618 Bytes) no_accept_encoding.pcap Brandon Murphy, 05/03/2023 02:36 PM
no_referer.pcap (617 Bytes) no_referer.pcap Brandon Murphy, 05/03/2023 02:36 PM
no_connection.pcap (626 Bytes) no_connection.pcap Brandon Murphy, 05/03/2023 02:36 PM
no_content_type.pcap (618 Bytes) no_content_type.pcap Brandon Murphy, 05/03/2023 02:36 PM
all_headers.pcap (650 Bytes) all_headers.pcap Brandon Murphy, 05/03/2023 02:36 PM

Related issues 5 (2 open3 closed)

Related to Suricata - Bug #2479: http_cookie negation fails if no cookie in trafficClosedOISF DevActions
Related to Suricata - Task #4097: Suricon 2020 brainstormAssignedVictor JulienActions
Blocked by Suricata - Bug #6025: detect: allow bsize 0 for existing empty buffersClosedPhilippe AntoineActions
Blocked by Suricata - Optimization #6575: detect/multi-buffer: use single definition of struct PrefilterMpmKrb5NameClosedPhilippe AntoineActions
Blocks Suricata - Story #7124: rules: improve rule languageNewVictor JulienActions
Actions #1

Updated by Andreas Herz almost 7 years ago

  • Assignee set to OISF Dev
  • Target version set to TBD
Actions #2

Updated by Philippe Antoine over 5 years ago

  • Related to Bug #2479: http_cookie negation fails if no cookie in traffic added
Actions #3

Updated by Andreas Herz over 5 years ago

Philippe are you looking into those two related issues so I can assign them to you :)?

Actions #4

Updated by Victor Julien over 5 years ago

This is not really a bug. It's more a design decisions about to how to deal with negation on buffers we don't see. Right now won't match as the rule is only evaluated if the buffer is present.

Actions #5

Updated by Philippe Antoine about 5 years ago

So, the feature request would be to have a new negate keyword that includes absence ?

Actions #6

Updated by Jason Ish almost 4 years ago

  • Related to Task #4097: Suricon 2020 brainstorm added
Actions #7

Updated by Brandon Murphy over 3 years ago

Just as an FYI - this issue/design/whatever continues to cause False Negatives on a pretty regular basis. If there is some sort of unofficial "vote" for tickets which need attention, consider this my vote.

Actions #8

Updated by Victor Julien about 2 years ago

@Brandon Murphy could you share some test cases to show how this causes FN's?

Actions #9

Updated by Philippe Antoine over 1 year ago

Digging a bit into this.

If we want this behavior, we need to patch DetectEngineInspectBufferGeneric (and others) so that it does not bail out early with if (unlikely(buffer NULL)) but checks if engine->smd->type DETECT_CONTENT and cd->flags & DETECT_CONTENT_NEGATED to return DETECT_ENGINE_INSPECT_SIG_MATCH

But it only works if we have another buffer to pass the pre filter step...

Actions #10

Updated by Philippe Antoine over 1 year ago

  • Blocked by Bug #6025: detect: allow bsize 0 for existing empty buffers added
Actions #11

Updated by Philippe Antoine over 1 year ago

https://redmine.openinfosecfoundation.org/issues/6025 simple patch is a prerequisite for this.

This will help distinguish absent buffers (matching negated content but not size 0) from empty buffers (matching both bsize 0 and negated content)

Actions #12

Updated by Philippe Antoine over 1 year ago

Posting my patch here waiting for #6025 merge

diff --git a/src/detect-engine.c b/src/detect-engine.c
index ce2d319d5..0c64b1e4b 100644
--- a/src/detect-engine.c
+++ b/src/detect-engine.c
@@ -2050,6 +2050,12 @@ uint8_t DetectEngineInspectBufferGeneric(DetectEngineCtx *de_ctx, DetectEngineTh
     const InspectionBuffer *buffer = engine->v2.GetData(det_ctx, transforms,
             f, flags, txv, list_id);
     if (unlikely(buffer == NULL)) {
+        if (engine->smd->type == DETECT_CONTENT) {
+            DetectContentData *cd = (DetectContentData *)engine->smd->ctx;
+            if (cd->flags & DETECT_CONTENT_NEGATED) {
+                return DETECT_ENGINE_INSPECT_SIG_MATCH;
+            }
+        }
         return eof ? DETECT_ENGINE_INSPECT_SIG_CANT_MATCH :
                      DETECT_ENGINE_INSPECT_SIG_NO_MATCH;
     }

Actions #13

Updated by Brandon Murphy over 1 year ago

Victor Julien wrote in #note-8:

@Brandon Murphy could you share some test cases to show how this causes FN's?

I somehow missed this, @Philippe Antoine do you still need test cases?

Actions #14

Updated by Philippe Antoine over 1 year ago

Brandon, yes please, your tests cases are welcome

Updated by Brandon Murphy over 1 year ago

Basic rules for multiple HTTP request rules and associated pcaps for testing.

# Test to ensure it works without a negated content
# This signature should alert with _any_  pcap
alert http $HOME_NET any -> $EXTERNAL_NET any (msg:"TP test for URI"; flow:established,to_server; http.uri; bsize:1; content:"/"; sid:1;)

# Test to prove an FN when UA is not included
# This signature will not alert (FN) on pcaps where the UA is not included in the HTTP Request
# test pcap = no_user_agent.pcap
alert http $HOME_NET any -> $EXTERNAL_NET any (msg:"FN test for User-Agent"; flow:established,to_server; http.uri; bsize:1; content:"/"; http.user_agent; content:!"example"; sid:2;)

# Test to prove FN when ACCEPT is not included
# This signature will not alert (FN) on pcaps where the Accept is not included in the HTTP Request
# test pcap = no_accept.pcap
alert http $HOME_NET any -> $EXTERNAL_NET any (msg:"FN test for Accept"; flow:established,to_server; http.uri; bsize:1; content:"/"; http.accept; content:!"example"; sid:3;)

# Test to prove FN when Accept-Language is not included
# This signature will not alert (FN) on pcaps where the Accept-Language is not included in the HTTP Request
# test pcap = no_accept_language.pcap
alert http $HOME_NET any -> $EXTERNAL_NET any (msg:"FN test for Accept-Language"; flow:established,to_server; http.uri; bsize:1; content:"/"; http.accept_lang; content:!"example"; sid:4;)

# Test to prove FN when Accept-Encoding is not included
# This signature will not alert (FN) on pcaps where the Accept-Encoding is not included in the HTTP Request
# test pcap = no_accept_encoding.pcap
alert http $HOME_NET any -> $EXTERNAL_NET any (msg:"FN test for Accept-Encoding"; flow:established,to_server; http.uri; bsize:1; content:"/"; http.accept_enc; content:!"example"; sid:5;)

# Test to prove FN when Referer is not included
alert http $HOME_NET any -> $EXTERNAL_NET any (msg:"FN test for Referer"; flow:established,to_server; http.uri; bsize:1; content:"/"; http.referer; content:!"example"; sid:6;)

# Test to prove FN when Connection is not included
alert http $HOME_NET any -> $EXTERNAL_NET any (msg:"FN test for Connection"; flow:established,to_server; http.uri; bsize:1; content:"/"; http.connection; content:!"example"; sid:7;)

# Test to prove FN when Content-Type is not included
# This signature will not alert (FN) on pcaps where the Content-Type is not included in the HTTP Request 
# test pcap = no_content_type.pcap
alert http $HOME_NET any -> $EXTERNAL_NET any (msg:"FN test for Content-Type"; flow:established,to_server; http.uri; bsize:1; content:"/"; http.content_type; content:!"example"; sid:8;)

Output of "no_referer.pcap" - notice that sid:6 does not fire

05/03/2023-14:18:45.426064  [**] [1:1:0] TP test for URI [**] [Classification: (null)] [Priority: 3] {TCP} 192.168.244.96:54222 -> 12.16.121.171:80
05/03/2023-14:18:45.426064  [**] [1:2:0] FN test for User-Agent [**] [Classification: (null)] [Priority: 3] {TCP} 192.168.244.96:54222 -> 12.16.121.171:80
05/03/2023-14:18:45.426064  [**] [1:3:0] FN test for Accept [**] [Classification: (null)] [Priority: 3] {TCP} 192.168.244.96:54222 -> 12.16.121.171:80
05/03/2023-14:18:45.426064  [**] [1:4:0] FN test for Accept-Language [**] [Classification: (null)] [Priority: 3] {TCP} 192.168.244.96:54222 -> 12.16.121.171:80
05/03/2023-14:18:45.426064  [**] [1:5:0] FN test for Accept-Encoding [**] [Classification: (null)] [Priority: 3] {TCP} 192.168.244.96:54222 -> 12.16.121.171:80
05/03/2023-14:18:45.426064  [**] [1:7:0] FN test for Connection [**] [Classification: (null)] [Priority: 3] {TCP} 192.168.244.96:54222 -> 12.16.121.171:80
05/03/2023-14:18:45.426064  [**] [1:8:0] FN test for Content-Type [**] [Classification: (null)] [Priority: 3] {TCP} 192.168.244.96:54222 -> 12.16.121.171:80

Actions #16

Updated by Philippe Antoine 11 months ago

  • Assignee changed from OISF Dev to Philippe Antoine
  • Target version changed from TBD to 8.0.0-beta1
Actions #17

Updated by Philippe Antoine 10 months ago

  • Status changed from New to In Review
Actions #18

Updated by Philippe Antoine 8 months ago

  • Status changed from In Review to In Progress

Today's status : https://github.com/OISF/suricata/pull/10140
support all "sticky buffers" not just DetectEngineInspectBufferGeneric

Actions #19

Updated by Philippe Antoine 8 months ago

  • Blocked by Optimization #6575: detect/multi-buffer: use single definition of struct PrefilterMpmKrb5Name added
Actions #20

Updated by Philippe Antoine 7 months ago

  • Status changed from In Progress to In Review

https://github.com/OISF/suricata/pull/10334 supports all, but the commits for #6575 should be merged first in their own PR

Actions #21

Updated by Victor Julien 4 months ago

  • Blocks Story #7124: rules: improve rule language added
Actions

Also available in: Atom PDF