Task #8690
opendetect-engine-mpm: adjust transforms->cnt type to unsigned integer
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.
DU Updated by Dmitry Uryvchikov 5 days ago
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
How would cnt be < 0?
DU Updated by Dmitry Uryvchikov 5 days ago
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
But how do we get into this state now? What ruleset would get us there?
DU Updated by Dmitry Uryvchikov 4 days ago
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
- 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
- Target version changed from TBD to 9.0.0-beta1
LS Updated by Lukas Sismis 2 days ago
- Label Needs backport to 8.0 added
OT Updated by OISF Ticketbot 2 days ago
- Subtask #8703 added
OT Updated by OISF Ticketbot 2 days ago
- Label deleted (
Needs backport to 8.0)