Stack buffer overflow in RT-Thread IPC

[0xdea]

Hi,

I would like to report another potential vulnerability in the current version of RT-Thread. Please let me know if you plan to ask for a CVE ID in case the vulnerability is confirmed. I'm available if you need further clarifications.

Potential stack buffer overflow in RT-Thread IPC

Summary

I spotted a potential stack buffer overflow vulnerability at the following location in the RT-Thread IPC source code:
https://github.com/RT-Thread/rt-thread/blob/master/components/libc/posix/ipc/mqueue.c#L278

Details

Unbounded rt_sprintf() in the mq_unlink() function could lead to a stack buffer overflow at the marked line:

int mq_unlink(const char *name)
{
    if(*name == '/')
    {
        name++;
    }
    const char *mq_path = "/dev/mqueue/";
    char mq_name[RT_NAME_MAX + 12] = {0};
    rt_sprintf(mq_name, "%s%s", mq_path, name); /* VULN: stack buffer overflow */
    return unlink(mq_name);
}

Please note that the mq_open() function at https://github.com/RT-Thread/rt-thread/blob/master/components/libc/posix/ipc/mqueue.c#L65-L70 implements bound checking:

    int len = rt_strlen(name);
    if (len > RT_NAME_MAX)
    {
        rt_set_errno(ENAMETOOLONG);
        return (mqd_t)(-1);
    }

Impact

If the unchecked input above is confirmed to be attacker-controlled and crossing a security boundary, the impact of the reported buffer overflow vulnerability could range from denial of service to arbitrary code execution.

[0xdea]

Hi, it's been one month since I reported this vulnerability, and I wanted to ask if you have any update. As standard practice, I plan to request a CVE ID for every confirmed vulnerability. I also intend to publish an advisory by February at the latest, unless there's a specific reason to postpone. Thanks!

[0xdea]

Hi there, CVE-2024-25391 was assigned to this vulnerability. I'm planning to publish my security advisory and writeup on March 5th. Thanks.

[ianr34]

I got here from the article on HaD - but looking at the code above it is unclear if it is or isn't a really big problem...
In this case "char mq_name[RT_NAME_MAX + 12]" sets the memory being copied to - where the 12 is the path length of the line before (by the way, using a '12' here is bad coding practice and could lead to problems later..) so as long as the "name" being passed in has been checked against RT_NAME_MAX things might still be ok - though badly written.

Looking around RT_NAME_MAX is 8, and the routine calling this routine does no checking. Indeed the example code []

rt-thread/examples/libc/mq.c

Line 123 in b85c4ca

mq_unlink(MQ_NAME_2);

calls it with

#define MQ_NAME_1       "testmsg1"
#define MQ_NAME_2       "testmsg2"

Which are 8 bytes long (plus a EOS). Plus the path which is 12. Plus the EOS which is 1. = 21bytes. Ooops, the buffer has overrun even with the example code.

So yes, it is a problem. I simply can't understand why people write code like the above (problem) code - there has been no point in the development of C that such code would have been either acceptable or pass any type of quality control, let alone actual testing..