Project

General

Profile

Actions

Documentation #8031

closed
SC VJ

isdataat: document different semantics between absolute and relative modes

Documentation #8031: isdataat: document different semantics between absolute and relative modes

Added by Sven Cuyt 5 months ago. Updated about 1 month ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

There seems to be an off by one error in isdataat's implementation. Below is the code that handles isdataat in detect-engine-content-inspection.c. Consider the following two scenarios:

1. isdataat is used absolutely with a value equal to the packet length. In that case, dataat < buffer_len would be false, thus it would not be a match.
2. isdataat is used relatively with a value equal to the packet length and the current offset is 0. In that case, det_ctx->buffer_offset + dataat > buffer_len is false, which results in a match.

However, I'd expect a relative isdataat from offset 0 to behave the same as an absolute isdataat.

        if (id->flags & ISDATAAT_RELATIVE) {
            if (det_ctx->buffer_offset + dataat > buffer_len) {
                SCLogDebug("det_ctx->buffer_offset + dataat %"PRIu32" > %"PRIu32, det_ctx->buffer_offset + dataat, buffer_len);
                if (id->flags & ISDATAAT_NEGATED)
                    goto match;
                if ((id->flags & ISDATAAT_OFFSET_VAR) == 0) {
                    goto no_match_discontinue;
                }
                goto no_match;
            } else {
                SCLogDebug("relative isdataat match");
                if (id->flags & ISDATAAT_NEGATED) {
                    goto no_match;
                }
                goto match;
            }
        } else {
            if (dataat < buffer_len) {
                SCLogDebug("absolute isdataat match");
                if (id->flags & ISDATAAT_NEGATED) {
                    if ((id->flags & ISDATAAT_OFFSET_VAR) == 0) {
                        goto no_match_discontinue;
                    }
                    goto no_match;
                }
                goto match;
            } else {
                SCLogDebug("absolute isdataat mismatch, id->isdataat %"PRIu32", buffer_len %"PRIu32"", dataat, buffer_len);
                if (id->flags & ISDATAAT_NEGATED)
                    goto match;
                if ((id->flags & ISDATAAT_OFFSET_VAR) == 0) {
                    goto no_match_discontinue;
                }
                goto no_match;
            }
        }
References:

Subtasks 2 (0 open2 closed)

Documentation #8240: isdataat: document different semantics between absolute and relative modes (8.0.x backport)ClosedVictor JulienActions
Documentation #8241: isdataat: document different semantics between absolute and relative modes (7.0.x backport)ClosedVictor JulienActions

VJ Updated by Victor Julien 3 months ago Actions #1

  • Subject changed from Different semantics between absolute and relative isdataat to isdataat: different semantics between absolute and relative modes
  • Status changed from New to Assigned
  • Assignee set to Victor Julien

I'll investigate.

VJ Updated by Victor Julien 3 months ago · Edited Actions #2

  • Tracker changed from Bug to Documentation
  • Subject changed from isdataat: different semantics between absolute and relative modes to isdataat: document different semantics between absolute and relative modes
  • Target version changed from TBD to 9.0.0-beta1

So this turns out to be a documented thing, but it's only documented here https://docs.suricata.io/en/suricata-8.0.2/rules/differences-from-snort.html#isdataat-keyword

It should be documented with the actual keyword doc too. It is not behavior we can change as it would risk of breaking many existing rules.

VJ Updated by Victor Julien 3 months ago Actions #4

  • Label Needs backport to 7.0, Needs backport to 8.0 added

OT Updated by OISF Ticketbot 3 months ago Actions #5

  • Subtask #8240 added

OT Updated by OISF Ticketbot 3 months ago Actions #6

  • Label deleted (Needs backport to 8.0)

OT Updated by OISF Ticketbot 3 months ago Actions #7

  • Subtask #8241 added

OT Updated by OISF Ticketbot 3 months ago Actions #8

  • Label deleted (Needs backport to 7.0)

SC Updated by Sven Cuyt 3 months ago Actions #9

Victor Julien wrote in #note-2:

So this turns out to be a documented thing, but it's only documented here https://docs.suricata.io/en/suricata-8.0.2/rules/differences-from-snort.html#isdataat-keyword

It should be documented with the actual keyword doc too. It is not behavior we can change as it would risk of breaking many existing rules.

Being unable to change the existing behavior is unfortunate but understandable. As a result I'd love to see clear documentation explaining the difference between absolute and relative isdataat, e.g. by providing some examples.

VJ Updated by Victor Julien 3 months ago · Edited Actions #10

@Surikitty any feedback on https://github.com/OISF/suricata/pull/14664/changes#diff-7ffcd036dc5853dac864215faa64a0401e43ba8d38eeb35730aeaf42295d22aeR274 ?

(edit: spotted an error in the table, should be good now)

SC Updated by Sven Cuyt 3 months ago Actions #11

Can you also add the link to https://docs.suricata.io/en/suricata-8.0.2/rules/differences-from-snort.html#isdataat-keyword

I also left some comments on the merge request you made.

VJ Updated by Victor Julien about 2 months ago Actions #12

  • Status changed from In Review to Resolved

VJ Updated by Victor Julien about 1 month ago Actions #13

  • Status changed from Resolved to Closed
Actions

Also available in: PDF Atom