Project

General

Profile

Actions

Task #8690

open
DU CT

detect-engine-mpm: adjust transforms->cnt type to unsigned integer

Task #8690: detect-engine-mpm: adjust transforms->cnt type to unsigned integer

Added by Dmitry Uryvchikov 5 days ago. Updated 2 days ago.

Status:
Triaged
Priority:
Normal
Target version:
Effort:
low
Difficulty:
low
Label:

Description

typedef struct DetectEngineTransforms { TransformData transforms[DETECT_TRANSFORMS_MAX]; int cnt; } DetectEngineTransforms;

DetectEngineTransforms::cnt is an int, but only cnt 0 was checked:

if (transforms NULL || transforms->cnt == 0)
return;

Negative values are therefore treated as valid. When transforms->cnt is negative, the loop is skipped, leaving xforms empty. The subsequent statement

xforms[strlen(xforms) - 1] = '\0';

evaluates to xforms[-1], causing an out-of-bounds write and resulting in undefined behavior. Fix this by checking cnt <= 0 instead of cnt == 0.


Subtasks 1 (1 open0 closed)

Task #8703: detect-engine-mpm: adjust transforms->cnt type to unsigned integer (8.0.x backport)AssignedCommunity TicketActions

DU Updated by Dmitry Uryvchikov 5 days ago Actions #1

typedef struct DetectEngineTransforms {
    TransformData transforms[DETECT_TRANSFORMS_MAX];
    int cnt;
} DetectEngineTransforms;

DetectEngineTransforms::cnt is an int, but only cnt 0 was checked:

if (transforms  NULL || transforms->cnt == 0)
    return;

Negative values are therefore treated as valid. When transforms->cnt is negative, the loop is skipped, leaving xforms empty. The subsequent statement

char xforms[1024] = "";
    for (int i = 0; i < transforms->cnt; i++) {
        char ttstr[64];
        (void)snprintf(ttstr, sizeof(ttstr), "%s,",
                sigmatch_table[transforms->transforms[i].transform].name);
        strlcat(xforms, ttstr, sizeof(xforms));
    }
    xforms[strlen(xforms) - 1] = '\0';

evaluates to xforms[-1], causing an out-of-bounds write and resulting in undefined behavior. Fix this by checking cnt <= 0 instead of cnt == 0.

VJ Updated by Victor Julien 5 days ago Actions #2

How would cnt be < 0?

DU Updated by Dmitry Uryvchikov 5 days ago Actions #3

transforms->cnt is an integer, so checking only for cnt == 0 does not account for negative values. Negative values for cnt are possible due to errors or an incorrect state—both now and in the future.

VJ Updated by Victor Julien 5 days ago Actions #4

But how do we get into this state now? What ruleset would get us there?

DU Updated by Dmitry Uryvchikov 4 days ago Actions #5

I analyzed the entire call chain:

DetectBsizeRegister->DetectBsizeSetup->DetectBufferGetActiveList->DetectEngineBufferTypeGetByIdTransforms-> DetectAppLayerMpmRegisterByParentId->AppendTransformsToPname.

In the current code, there is no path where `cnt` could become negative: initialization via SCCalloc, increment only with a check against DETECT_TRANSFORMS_MAX , reset to 0.
However, the invariant cnt >= 0 is enforced only by convention, not by the type. It could easily be violated by any future code changes.
I propose the following options:
Add a check for transforms->cnt <= 0 if the data type is not changed.
Change the type of `cnt` from `int` to `uint8_t` —the invariant is enforced at the type level; `DETECT_TRANSFORMS_MAX` is small, so `uint8_t` is sufficient.
Add `DEBUG_VALIDATE_BUG_ON(transforms->cnt < 0)` —if the invariant is violated, the test will fail with a clear reason, rather than resulting in a silent UB.

LS Updated by Lukas Sismis 2 days ago Actions #6

  • Tracker changed from Bug to Task
  • Subject changed from detect-engine-mpm: Out-of-bounds write when transforms->cnt is negative to detect-engine-mpm: adjust transforms->cnt type to unsigned integer
  • Status changed from New to Triaged
  • Assignee set to Community Ticket
  • Effort set to low
  • Difficulty set to low
  • Affected Versions deleted (9.0.0-beta1)

LS Updated by Lukas Sismis 2 days ago Actions #7

  • Target version changed from TBD to 9.0.0-beta1

LS Updated by Lukas Sismis 2 days ago Actions #8

  • Label Needs backport to 8.0 added

OT Updated by OISF Ticketbot 2 days ago Actions #9

  • Subtask #8703 added

OT Updated by OISF Ticketbot 2 days ago Actions #10

  • Label deleted (Needs backport to 8.0)
Actions

Also available in: PDF Atom