syscheck decoder: off-by-one heap overflow in DB operations.

[cpu]

Edit: see my comment below this analysis was flawed. Apologies!

The ossec-analysisd's syscheck decoder (src/analysisd/decoders/syscheck.c) allocates two fixed size heap buffers via the global static struct sdb:

ossec-hids/src/analysisd/decoders/syscheck.c

Line 55 in abb36d4

static _sdb sdb;

The _sdb struct contains two fixed size arrays, one of char* called agent_ips and one of FILE* called agent_fps, both sized MAX_AGENTS (default build value is 2048). (edit: this was wrong)

ossec-hids/src/analysisd/decoders/syscheck.c

Lines 34 to 35 in abb36d4

	char *agent_ips[MAX_AGENTS + 1];
	FILE *agent_fps[MAX_AGENTS + 1];

In DB_SetCompleted and DB_File a while loop with a index counter i is used to try and find an index of sbd.agent_ips that matches the provided agent name.

To prevent i exceeding MAX_AGENTS the while loops are written:

    while (sdb.agent_ips[i] != NULL &&  i < MAX_AGENTS) {

Unfortunately due to the ordering of the clauses that means that when i == MAX_AGENTS the sdb.agent_ips[i] clause that is evaluated first will be accessing outside of the bounds of sdb.agent_ips by 1, likely reading into sdb.agent_fps[0].

This code was introduced in 91aa29a on Feb 7, 2012 while patching a more significant buffer overflow found by Paul Southerington. I believe it affects OSSEC v2.7+.

I suspect the fixed code is not exploitable, but is a buggy remediation.

This is triggerable via an authenticated client through the ossec-remoted. The client needs only write MAX_AGENT syscheck update messages with distinct message agent names.

While ossec-remoted always sets the agent name portion of messages passed on to ossec-analysisd with a prefix out of the attackers control based on the configured agent key and source IP (($NAME) $SRCPIP->), the portion after this prefix is attacker controlled and thus can be mutated to make MAX_AGENT unique names.

The while conditions in DB_File and DB_SetCompleted should be rewritten to short circuit if i >= MAX_AGENTS before dereferencing sdb.agent_ips[i].

E.g. instead of:

    while (sdb.agent_ips[i] != NULL &&  i < MAX_AGENTS) {

Use:

    while (i < MAX_AGENTS && sdb.agent_ips[i] != NULL) {

[cpu]

I took another read through this code while drafting a PR with a fix and I realized I made a mistake in my analysis. The agent_ips and agent_fps arrays are not sized MAX_AGENT for the syscheck decoder, they're sized MAX_AGENT + 1 and initialized accordingly:

ossec-hids/src/analysisd/decoders/syscheck.c

Lines 34 to 35 in abb36d4

	char *agent_ips[MAX_AGENTS + 1];
	FILE *agent_fps[MAX_AGENTS + 1];

ossec-hids/src/analysisd/decoders/syscheck.c

Lines 96 to 100 in abb36d4

	for (; i <= MAX_AGENTS; i++) {
	sdb.agent_ips[i] = NULL;
	sdb.agent_fps[i] = NULL;
	sdb.agent_cp[i][0] = '0';
	}

That means it's perfectly fine to be deref'ing sdb.agent_ips[i] at i == MAX_AGENTS. That value will always be NULL based on the initialization code.

Apologies for the false alarm with this bug! I'll close this issue and update #1821 accordingly.