Heap buffer overflows in RT-Thread dfs_v2 dfs_file

[0xdea]

Hi,

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

Potential heap buffer overflows in RT-Thread dfs_v2 dfs_file

Summary

I spotted some potential heap buffer overflow vulnerabilities at the following locations in the RT-Thread dfs_v2 dfs_file source code:
https://github.com/RT-Thread/rt-thread/blob/master/components/dfs/dfs_v2/src/dfs_file.c#L234
https://github.com/RT-Thread/rt-thread/blob/master/components/dfs/dfs_v2/src/dfs_file.c#L262
https://github.com/RT-Thread/rt-thread/blob/master/components/dfs/dfs_v2/src/dfs_file.c#L284
https://github.com/RT-Thread/rt-thread/blob/master/components/dfs/dfs_v2/src/dfs_file.c#L314

Details

Lack of length check in the the dfs_nolink_path() function could lead to heap buffer overflows at the marked lines:

static char *dfs_nolink_path(struct dfs_mnt **mnt, char *fullpath, int mode)
{
    int index = 0;
    char *path = RT_NULL;
    char link_fn[DFS_PATH_MAX] = {0};
    struct dfs_dentry *dentry = RT_NULL;

    path = (char *)rt_malloc((DFS_PATH_MAX * 2) + 1); // path + syslink + \0
    if (!path)
    {
        return path;
    }

    if (*mnt && fullpath)
    {
        int i = 0;
        char *fp = fullpath;

        while (*fp != '\0')
        {
            fp++;
            i++;
            if (*fp == '/')
            {
                rt_memcpy(path + index, fp - i, i); /* VULN: if fullpath has components large enough so that i+index becomes larger than DFS_PATH_MAX*2+1, we could overflow past the path buffer */
                path[index + i] = '\0';

                dentry = dfs_dentry_lookup(*mnt, path, 0);
                if (dentry && dentry->vnode->type == FT_SYMLINK)
                {
                    int ret = -1;

                    if ((*mnt)->fs_ops->readlink)
                    {
                        if (dfs_is_mounted((*mnt)) == 0)
                        {
                            ret = (*mnt)->fs_ops->readlink(dentry, link_fn, DFS_PATH_MAX);
                        }
                    }

                    if (ret > 0)
                    {
                        int len = rt_strlen(link_fn);
                        if (link_fn[0] == '/')
                        {
                            rt_memcpy(path, link_fn, len);
                            index = len;
                        }
                        else
                        {
                            path[index] = '/';
                            index++;
                            rt_memcpy(path + index, link_fn, len); /* VULN: len can be DFS_PATH_MAX; if index can become larger than DFS_PATH_MAX+1, we can overflow past the path buffer */
                            index += len;
                        }
                        path[index] = '\0';
                        *mnt = dfs_mnt_lookup(path);
                    }
                    else
                    {
                        rt_kprintf("link error: %s\n", path);
                    }
                }
                else
                {
                    index += i;
                }
                dfs_dentry_unref(dentry);
                i = 0;
            }
        }

        if (i)
        {
            rt_memcpy(path + index, fp - i, i); /* VULN: if fullpath has components large enough so that i+index becomes larger than DFS_PATH_MAX*2+1, we could overflow past the path buffer */
            path[index + i] = '\0';

            if (mode)
            {
                dentry = dfs_dentry_lookup(*mnt, path, 0);
                if (dentry && dentry->vnode->type == FT_SYMLINK)
                {
                    int ret = -1;

                    if ((*mnt)->fs_ops->readlink)
                    {
                        if (dfs_is_mounted((*mnt)) == 0)
                        {
                            ret = (*mnt)->fs_ops->readlink(dentry, link_fn, DFS_PATH_MAX);
                        }
                    }

                    if (ret > 0)
                    {
                        int len = rt_strlen(link_fn);
                        if (link_fn[0] == '/')
                        {
                            rt_memcpy(path, link_fn, len);
                            index = len;
                        }
                        else
                        {
                            path[index] = '/';
                            index++;
                            rt_memcpy(path + index, link_fn, len); /* VULN: len can be DFS_PATH_MAX; if index can become larger than DFS_PATH_MAX+1, we can overflow past the path buffer */
                            index += len;
                        }
                        path[index] = '\0';
                        *mnt = dfs_mnt_lookup(path);
                    }
                    else
                    {
                        rt_kprintf("link error: %s\n", path);
                    }

                    char *_fullpath = dfs_normalize_path(RT_NULL, path);
                    if (_fullpath)
                    {
                        strncpy(path, _fullpath, DFS_PATH_MAX);
                        rt_free(_fullpath);
                    }
                }
                dfs_dentry_unref(dentry);
            }
        }
    }
    else
    {
        rt_free(path);
        path = RT_NULL;
    }

    //rt_kprintf("%s: %s => %s\n", __FUNCTION__, fullpath, path);

    return path;
}

Impact

If an attacker is able to control fullpath above and craft it as required to trigger the reported heap buffer overflow vulnerabilities, their impact could range from denial of service to arbitrary code execution.

[geniusgogo]

If an attacker is able to control fullpath above and craft it as required to trigger the reported heap buffer overflow vulnerabilities, their impact could range from denial of service to arbitrary code execution.

the fullpath from dfs_normalize_path，shouldn't be the case with Fullpath's provenance？

[0xdea]

Based on my analysis, fullpath comes indeed from dfs_normalize_path(), which in turn processes user input. I couldn't see any limits imposed on path length in dfs_normalize_path(), i.e.:

    if (filename[0] != '/') /* it's a absolute path, use it directly */
    {
        fullpath = (char *)rt_malloc(strlen(directory) + strlen(filename) + 2);

        if (fullpath == NULL)
            return NULL;

        /* join path and file name */
        rt_snprintf(fullpath, strlen(directory) + strlen(filename) + 2,
                    "%s/%s", directory, filename);
    }
    else
    {
        fullpath = rt_strdup(filename); /* copy string */

        if (fullpath == NULL)
            return NULL;
    }

Therefore, unless there are other filesystem limitations in place or I'm missing something else, I think an attacker might be able to control fullpath and trigger the buffer overflows. Just to err on the side of caution, I'd recommend adding some explicit length checks.

[BernardXiong]

Thank you for your feedback. @geniusgogo have fixed with #8305. If you could check it again, we would greatly appreciate! 🙏

[0xdea]

It looks good now. I'm glad my bug report has sparked some improvements also in other areas of the codebase!

However, I'm not entirely sure about this fix that is included in the same commit but is related to #8286 (heap buffer overflows in finsh):

        if (rt_strcmp(".", dirent->d_name) != 0 &&
                rt_strcmp("..", dirent->d_name) != 0)
        {
            if (strlen(pathname) + 1 + strlen(dirent->d_name) > DFS_PATH_MAX)
            {
                rt_kprintf("cannot remove '%s/%s', path too long.\n", pathname, dirent->d_name);
                continue;
            }
            rt_sprintf(full_path, "%s/%s", pathname, dirent->d_name);
            if (dirent->d_type != DT_DIR)
            {
...
        if (rt_strcmp(".", dirent->d_name) != 0 &&
            rt_strcmp("..", dirent->d_name) != 0)
        {
            if (strlen(pathname) + 1 + strlen(dirent->d_name) > DFS_PATH_MAX)
            {
                rt_kprintf("'%s/%s' setattr failed, path too long.\n", pathname, dirent->d_name);
                continue;
            }
            rt_sprintf(full_path, "%s/%s", pathname, dirent->d_name);
            if (dirent->d_type == DT_REG)
            {

The added length check doesn't account for the terminating NUL byte: if strlen(pathname) + 1 + strlen(dirent->d_name) == DFS_PATH_MAX, we would pass the check but have an off-by-one bug in the rt_sprintf() calls. It would probably be better to replace rt_sprintf() with snprintf(), slprintf(), or a similar safer alternative.

[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, I just wanted to let you know that I've requested a CVE ID from MITRE for this vulnerability and for #8271 as well. I'm planning to do the same for the other issues that I've opened in November 2023.

As anticipated, I intend to publish an advisory that documents these issues and possibly a short writeup on my blog. The tentative date for publication is February 28th, 2024, more than 90 days since the initial report, which is standard practice for security researchers. Especially considering that these issues are already public on GitHub, I don't see any reasons to postpone. However, it would be great to have these issues triaged and patched before publication.

I remain available in case you need any clarifications. Thanks!

[0xdea]

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