Kernel: CoreDump follows symlinks for core dump directory when writing core dump files

[bcoles]

The /tmp/coredump/ directory used for storing coredumps is created by CrashDaemon upon system startup. The directory is owned by the user who launched the service (default anon).

If the directory is removed, next time a crash is encountered the directory will be recreated with 777 permissions and root ownership :

serenity/Kernel/CoreDump.cpp

Lines 62 to 85 in 5eacf62

	RefPtr<FileDescription> CoreDump::create_target_file(const Process& process, const String& output_path)
	{
	LexicalPath lexical_path(output_path);
	auto output_directory = lexical_path.dirname();
	if (VFS::the().open_directory(output_directory, VFS::the().root_custody()).is_error()) {
	auto res = VFS::the().mkdir(output_directory, 0777, VFS::the().root_custody());
	if (res.is_error())
	return nullptr;
	}
	auto tmp_dir = VFS::the().open_directory(output_directory, VFS::the().root_custody());
	if (tmp_dir.is_error())
	return nullptr;
	auto fd_or_error = VFS::the().open(
	lexical_path.basename(),
	O_CREAT | O_WRONLY | O_EXCL,
	0, // We will enable reading from userspace when we finish generating the coredump file
	*tmp_dir.value(),
	UidAndGid { process.uid(), process.gid() });
	if (fd_or_error.is_error())
	return nullptr;
	return fd_or_error.value();
	}

This permits replacing the directory with a symlink (as anon in the default scenario or as any user if the directory has been deleted/replaced with 777 permissions).

As CrashDaemon follows symlinks, this allows low-privileged users to spray core dump files onto any writable mounted filesystems.

Note: Writing core dumps does not appear to follow symlinks for the core dump files themselves, only the core dump directory. As such, despite the predictable core dump file names, it is not possible to abuse this to control the core dump file name or clobber existing files (unless I'm missing something). Here's the code I was using to mess around with the predictable core dump file names:

/*
 * Copyright (c) 2020, the SerenityOS developers.
 * All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions are met:
 *
 * 1. Redistributions of source code must retain the above copyright notice, this
 *    list of conditions and the following disclaimer.
 *
 * 2. Redistributions in binary form must reproduce the above copyright notice,
 *    this list of conditions and the following disclaimer in the documentation
 *    and/or other materials provided with the distribution.
 *
 * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
 * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
 * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
 * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
 * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
 * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
 * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
 * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
 * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */

#include <LibCore/ArgsParser.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <AK/Time.h>
#include <LibCore/DateTime.h>

int main(int argc, char** argv)
{
    const char* new_file = nullptr;

    Core::ArgsParser args_parser;
    args_parser.add_option(new_file, "File to create", nullptr, 'f', "file");
    args_parser.parse(argc, argv);

    //rmdir("/tmp/coredump/");
    long rtc = 1608142500; // adjust this. use `crash -d` first to find the current time. im lazy.
    long pid = getpid() + 2; // +1 for calling `crash -d` and +1 for CrashDeamon restart
    dbg() << "PID: " << pid;

    for (auto i = 0; i < 120; ++i) {
        // Kernel/Process.cpp:601:        auto coredump_path = String::formatted("/tmp/coredump/{}_{}_{}", name(), m_pid.value(), RTC::now());
        auto path = String::format("/tmp/coredump/crash_%d_%d", pid, rtc + i);
        dbg() << "Path: " << path;
        int rc = symlink(new_file, path.characters());
        if (rc < 0) {
            perror("symlink");
            return 1;
        }
    }

    // wait for CrashDaemon to restart
    sleep(5);
    outln("Now run: `crash -d`");
    return 0;
}

[awesomekling]

cc @itamar8910 @bugaevc

Generally, I think we should move away from the kernel writing coredumps to a hard-coded fs path and instead making them available through either a CoredumpFS or some kind /dev device. Hm, or perhaps a /proc/PID/coredump..

[bugaevc]

My plan was a special device in /dev (/dev/dump or something) that SystemServer would listen on like it does for sockets, then once it becomes readable SystemServer spawns a special service (DumpService) that reads the dump, saves it to some location on disk, then displays a GUI notification with a button to open the dump for post-mortem debugging in HackStudio.

[bugaevc]

And yes, the kernel assuming anything about the filesystem layout feels like a violation of layering. Ideally the only path hardcoded in the kernel is /bin/SystemServer, and that is too overridable via kernel command line (init=/totally/different/path).

That being said, it's not as important for Serenity as it is for Linux, because we don't expect people to run the kernel with userspaces different than ours. Whereas you can run various userspaces on Linux, with wildly different FS layouts and such.

[bcoles]

I'm fond of per-user temporary directories for a whole bunch of reasons. This may be a viable option for storing the core dumps on disk (so long as symlinks are forbidden). This would also help prevent leaking information via readable core dumps of another user's processes or privileged processes (su, etc).

Edit: +1 for CoredumpFS

[supercomputer7]

I like the option of CoredumpFS as suggested by @awesomekling. One advantage of this option (as said by AK) is to put coredumps on the disk, which can assist us in case of OOM.
Last commit (for now) 4232874 disabled coredump generation in such case, and CoredumpFS could revert this change.
Also, it will be nice to preserve coredumps across reboots, in case of "I forgot to backup my coredump and I crashed the system", or "my program crashed and then the system crashed too".

[bugaevc]

CoredumpFS is not about saving files to disk, on the contrary it's about keeping core dumps in RAM and exposing them as a pseudo-filesystem like ProcFS.