Wazuh: Memory leak in ossec-dbd

[DaveVG1]

Bug report

OS
_Any_

Wazuh version
_3.8.0_

Install type
_manager_

Install method
_Any_

Valgrind report

==4130== LEAK SUMMARY:
==4130==    definitely lost: 908 bytes in 11 blocks
==4130==    indirectly lost: 4,173 bytes in 98 blocks
==4130==      possibly lost: 675,676 bytes in 13,590 blocks
==4130==    still reachable: 155,671 bytes in 4,252 blocks

Description

There are memory leaks in ossec-dbd. The valgrind report of this is above.

[crolopez]

Hi,

I have found this.

==13562== Invalid read of size 1
==13562==    at 0x11088F: osdb_escapestr (db_op.c:93)
==13562==    by 0x1113E9: _Rules_ReadInsertDB (rules.c:178)
==13562==    by 0x1865CE: OS_ReadXMLRules (rules_op.c:861)
==13562==    by 0x1115AF: OS_InsertRulesDB (rules.c:226)
==13562==    by 0x111E8A: main (main.c:228)
==13562==  Address 0x72e23cf is 1 bytes before a block of size 1 alloc'd
==13562==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13562==    by 0x61239B9: strdup (strdup.c:42)
==13562==    by 0x172ABA: os_LoadString (mem_op.c:93)
==13562==    by 0x1832ED: OS_ReadXMLRules (rules_op.c:304)
==13562==    by 0x1115AF: OS_InsertRulesDB (rules.c:226)
==13562==    by 0x111E8A: main (main.c:228)

[DarkMatterCore]

After analizing the Valgrind output provided by @crolopez, it becomes clear that the issue is related to the comment member of the config_ruleinfo struct (of type RuleInfo). This variable represents a character string allocated in memory using strdup().

---

Valgrind log (part 1): function doing the incorrect accessing

==13562== Invalid read of size 1
==13562==    at 0x11088F: osdb_escapestr (db_op.c:93)
==13562==    by 0x1113E9: _Rules_ReadInsertDB (rules.c:178)
==13562==    by 0x1865CE: OS_ReadXMLRules (rules_op.c:861)
==13562==    by 0x1115AF: OS_InsertRulesDB (rules.c:226)
==13562==    by 0x111E8A: main (main.c:228)
==13562==  Address 0x72e23cf is 1 bytes before a block of size 1 alloc'd

According to this block from the Valgrind output, this code sequence would be the following:

https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/os_dbd/main.c#L229
https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/os_dbd/rules.c#L227
https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/shared/rules_op.c#L865
ruleact_function represents a function pointer that references _Rules_ReadInsertDB() in src/os_dbd/rules.c.
https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/os_dbd/rules.c#L179
https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/os_dbd/db_op.c#L94

An "invalid read of size 1" occurs when trying to perform a comparison on the value stored in the previous pointer position, using a simple substraction + pointer dereference. This indicates that str - 1 yields an invalid memory address.

The stored string is a duplication created through strdup() in os_LoadString():

https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/shared/mem_op.c#L94

This function can also reallocate the memory buffer to concatenate additional data to it if it is already valid. However, this is not the case.

The fact that no allocation errors are reported in the logfile and that Valgrind clearly states that the access is being performed "1 byte before a block of size 1 alloc'd", makes me think that str is, somehow, a 1-byte allocated buffer that holds a single null (\0) byte.

This would explain two things: (a) why the str - 1 substraction yields an invalid memory access after the pointer position is supposedly increased inside the loop, and (b) why an invalid memory access is not reported by Valgrind inside the while() loop in osdb_escapestr().

Truth is, even though the str pointer holds a valid memory address, the while() loop is being skipped because its condition isn't met initially - the pointer dereference *str is actually \0. A possible solution would be changing this:

https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/os_dbd/db_op.c#L78-L80

Into this:

    if (!str || !*str) {
        return;
    }

However, this would only fix the memory access issue in this function. We need to attack the real source of the problem.

---

Valgrind log (part 2): function that allocated the "block of size 1"

==13562==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13562==    by 0x61239B9: strdup (strdup.c:42)
==13562==    by 0x172ABA: os_LoadString (mem_op.c:93)
==13562==    by 0x1832ED: OS_ReadXMLRules (rules_op.c:304)
==13562==    by 0x1115AF: OS_InsertRulesDB (rules.c:226)
==13562==    by 0x111E8A: main (main.c:228)

According to this block from the Valgrind output, this code sequence would be the following:

https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/os_dbd/main.c#L229
https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/os_dbd/rules.c#L227
https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/shared/rules_op.c#L304-L306
https://github.com/wazuh/wazuh/blob/688157e869f371271395da30cc490d45cd6d5753/src/shared/mem_op.c#L94

The comment member of the config_ruleinfo struct is initially set to NULL, thus making the os_LoadString() function use strdup() on it instead of reallocating the variable, just like it was mentioned before.

Even though expanding the condition in osdb_escapestr() can help avoid problems in that function, the real cause of the memory leak is here.

We need to make os_LoadString() check if the data to duplicate/concatenate is actually valid before proceeding. A slightly modified version of the code block for osdb_escapestr() should to the trick:

char *os_LoadString(char *at, const char *str)
{
    if (at == NULL) {
        if (!str || !*str) {
            return (NULL);
        }

        at = strdup(str);
        if (!at) {
            merror(MEM_ERROR, errno, strerror(errno));
        }
        return (at);
    } else { /* at is not null. Need to reallocate its memory and copy str to it */
        if (!str || !*str) {
            return (at);
        }

        char *newat;
        size_t strsize = strlen(str);
        size_t finalsize = strsize + strlen(at) + 1;

        newat = (char *) realloc(at, finalsize * sizeof(char));
        if (newat == NULL) {
            free(at);
            merror(MEM_ERROR, errno, strerror(errno));
            return (NULL);
        }
        at = newat;

        strncat(at, str, strsize);
        at[finalsize - 1] = '\0';

        return (at);
    }

    return (NULL);
}

[DarkMatterCore]

A PR has been created to address this issue.

@DaveVG1 Thanks for reporting this problem!

@crolopez Thanks for the additional input on this matter!