diff --git a/.github/workflows/build-linux.yml b/.github/workflows/build-linux.yml index 6379b82ce8..8de7e6ef15 100644 --- a/.github/workflows/build-linux.yml +++ b/.github/workflows/build-linux.yml @@ -28,6 +28,7 @@ on: - 'Tests/test_stdin_password.py' - 'Tests/test_mount_helper.py' - 'Tests/test_linux_mount_point_protection.py' + - 'Tests/test_linux_fuse_service_memory.py' - 'Tests/test.*.hc' - 'Tests/test_volume_read.py' - 'Tests/volume_read_test.cpp' @@ -64,6 +65,7 @@ on: - 'Tests/test_stdin_password.py' - 'Tests/test_mount_helper.py' - 'Tests/test_linux_mount_point_protection.py' + - 'Tests/test_linux_fuse_service_memory.py' - 'Tests/test.*.hc' - 'Tests/test_volume_read.py' - 'Tests/volume_read_test.cpp' @@ -221,6 +223,7 @@ jobs: sudo python3 Tests/test_stdin_password.py --require-locales sudo python3 Tests/test_mount_helper.py --helper /usr/sbin/mount.veracrypt sudo python3 Tests/test_linux_mount_point_protection.py + sudo python3 Tests/test_linux_fuse_service_memory.py sudo apt remove -y veracrypt - name: Install and test VeraCrypt Console .deb packages diff --git a/Tests/test_linux_fuse_service_memory.py b/Tests/test_linux_fuse_service_memory.py new file mode 100644 index 0000000000..9e7055ea9a --- /dev/null +++ b/Tests/test_linux_fuse_service_memory.py @@ -0,0 +1,174 @@ +#!/usr/bin/env python3 +"""Regression test for issue #1958, against the installed veracrypt. + +Usage (as root, after installing a package): + python3 Tests/test_linux_fuse_service_memory.py [--veracrypt PATH] + +Mounts a small volume through FUSE (nokernelcrypto) and checks that the +FUSE service, which holds the master keys, has locked memory and a core +file size limit of zero. +""" + +import argparse +import os +import shutil +import subprocess +import sys +import tempfile +from pathlib import Path + +PASSWORD_A = "regression-test-password-a" +# A low PIM keeps the tests fast; it needs passwords of 20 characters or more. +COMMON = ["--pim=1", "--keyfiles="] +MOUNT = COMMON + ["--protect-hidden=no", "--mount-options=nokernelcrypto"] + + +class Checks: + def __init__(self): + self.failures = 0 + + def check(self, desc, cond, detail=""): + print(("ok " if cond else "FAIL ") + desc, flush=True) + if not cond: + self.failures += 1 + if detail: + print(" " + detail.strip().replace("\n", "\n ")) + + +def text(result): + return (result.stdout or b"").decode("utf-8", "replace") + + +class VeraCrypt: + def __init__(self, path, work): + self.path = path + self.binary = os.path.realpath(path) + self.work = work + + def run(self, args, stdin=None, timeout=300): + return subprocess.run([self.path, "--text"] + args, input=stdin, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, cwd=self.work, + timeout=timeout) + + def create(self, path, size, password, filesystem, volume_type="normal"): + return self.run(["--non-interactive", "--create", str(path), + "--size=" + size, "--volume-type=" + volume_type, + "--encryption=AES", "--hash=SHA-512", + "--filesystem=" + filesystem, "-p", password, + "--random-source=" + str(self.work / "random")] + + COMMON) + + def mount(self, volume, password, mount_point=None, extra=()): + args = ["--non-interactive", "-p", password] + MOUNT + list(extra) + args.append(str(volume)) + if mount_point: + args.append(str(mount_point)) + return self.run(args) + + def dismount(self, target): + return self.run(["--non-interactive", "-d", str(target)]) + + def mounted_volumes(self): + """Paths of the mounted volumes; raises if they cannot be listed.""" + r = self.run(["--list"]) + if r.returncode != 0: + if "No volumes mounted" in text(r): + return set() + raise RuntimeError("veracrypt --list failed: " + text(r)) + return {line.split()[1] for line in text(r).splitlines() + if len(line.split()) >= 4 and line.split()[0].endswith(":")} + + def dismount_volumes(self, volumes): + """Dismounts only these volumes; returns whether none remains.""" + paths = {str(volume) for volume in volumes} + for path in paths & self.mounted_volumes(): + self.dismount(path) + return not paths & self.mounted_volumes() + + def service(self, volume): + """PID of the FUSE service that has the volume file open.""" + volume = os.path.realpath(volume) + for entry in os.listdir("/proc"): + if not entry.isdigit(): + continue + try: + if os.readlink("/proc/%s/exe" % entry) != self.binary: + continue + fds = ["/proc/%s/fd/%s" % (entry, fd) + for fd in os.listdir("/proc/%s/fd" % entry)] + targets = {os.readlink(fd) for fd in fds} + except OSError: + continue + if "/dev/fuse" in targets and volume in targets: + return int(entry) + return None + + +def process_status(pid, field): + with open("/proc/%d/status" % pid) as status: + for line in status: + if line.startswith(field + ":"): + return line.split(":", 1)[1].strip() + return "" + + +def core_limit(pid): + with open("/proc/%d/limits" % pid) as limits: + for line in limits: + if line.startswith("Max core file size"): + return line.split()[4:6] + return [] + + +def test_memory(vc, c, volume, mount_point): + r = vc.mount(volume, PASSWORD_A, mount_point) + c.check("#1958: mount", r.returncode == 0, text(r)) + pid = vc.service(volume) + c.check("#1958: find the FUSE service", pid is not None) + if pid: + locked = process_status(pid, "VmLck") + c.check("#1958: the FUSE service has locked memory", + locked.split()[:1] != ["0"], "VmLck: " + locked) + limit = core_limit(pid) + c.check("#1958: the FUSE service cannot write a core file", + limit == ["0", "0"], "core file size limit: %s" % limit) + c.check("#1958: dismount", vc.dismount_volumes([volume])) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--veracrypt", + default=shutil.which("veracrypt") or "veracrypt") + args = parser.parse_args() + if os.geteuid() != 0: + sys.exit("Run as root: the test mounts volumes") + + work = Path(tempfile.mkdtemp(prefix="vc-regression-")) + vc = VeraCrypt(args.veracrypt, work) + c = Checks() + mount_points = [work / name for name in ("a", "b")[:1]] + for mount_point in mount_points: + mount_point.mkdir() + (work / "random").write_bytes(os.urandom(4096)) + try: + volume = work / "a.hc" + r = vc.create(volume, "10M", PASSWORD_A, "ext4") + c.check("create %s" % volume.name, r.returncode == 0, text(r)) + if c.failures: + sys.exit("Cannot create the test volume") + test_memory(vc, c, volume, mount_points[0]) + finally: + # Only the volume of this test: unrelated ones may be mounted + if (not vc.dismount_volumes([work / "a.hc"]) + or any(os.path.ismount(m) for m in mount_points)): + sys.exit("A test volume is still mounted; not removing %s" % work) + shutil.rmtree(work, ignore_errors=True) + + if c.failures: + sys.exit("%d check(s) failed" % c.failures) + print("All checks passed") + + +if __name__ == "__main__": + main() diff --git a/src/Core/Unix/CoreService.cpp b/src/Core/Unix/CoreService.cpp index 66fbf20959..3c63b024c3 100644 --- a/src/Core/Unix/CoreService.cpp +++ b/src/Core/Unix/CoreService.cpp @@ -1136,6 +1136,8 @@ namespace VeraCrypt { try { + Process::DisableCoreDumps(); + if (forkProcess) RedirectStandardErrorToDevNull (); diff --git a/src/Driver/Fuse/FuseService.cpp b/src/Driver/Fuse/FuseService.cpp index 055eb1de0a..5b6a0527a9 100644 --- a/src/Driver/Fuse/FuseService.cpp +++ b/src/Driver/Fuse/FuseService.cpp @@ -664,6 +664,9 @@ namespace VeraCrypt { try { + // FUSE may have forked to run in the background + SecureBuffer::LockAllAfterFork(); + // Termination signals are handled by a separate process to allow clean dismount on shutdown struct sigaction action; Memory::Zero (&action, sizeof (action)); @@ -1749,6 +1752,9 @@ namespace VeraCrypt void FuseService::ExecFunctor::operator() (int argc, char *argv[]) { + // The service holds the master keys until the volume is dismounted + Process::DisableCoreDumps(); + #if defined(TC_MACOSX) && defined(VC_MACOSX_FUSET) close (StartupPeerFd); #endif diff --git a/src/Platform/Buffer.cpp b/src/Platform/Buffer.cpp index ac55c7bb93..db63bbaf90 100644 --- a/src/Platform/Buffer.cpp +++ b/src/Platform/Buffer.cpp @@ -12,9 +12,51 @@ #include "Buffer.h" #include "Exception.h" +#ifdef TC_UNIX +#include +#include +#include +#include "ForEach.h" +#include "Mutex.h" +#endif namespace VeraCrypt { +#ifdef TC_UNIX + // Secure buffers occupy whole pages of their own, which are locked in memory + // and excluded from core dumps. Unlocking or including the pages again when a + // buffer is freed cannot then affect another allocation. + struct SecureBufferList + { + Mutex ListMutex; + set Buffers; + }; + + static SecureBufferList &GetSecureBufferList () + { + // Never destroyed: buffers may be freed after static destructors have run + static SecureBufferList *list = new SecureBufferList; + return *list; + } + + static size_t GetPageSize () + { + long pageSize = sysconf (_SC_PAGESIZE); + return pageSize > 0 ? (size_t) pageSize : 4096; + } + + static void ExcludeFromCoreDump (void *memory, size_t size, bool exclude) + { +#if defined (MADV_DONTDUMP) + madvise (memory, size, exclude ? MADV_DONTDUMP : MADV_DODUMP); +#elif defined (MADV_NOCORE) + madvise (memory, size, exclude ? MADV_NOCORE : MADV_CORE); +#else + (void) memory; (void) size; (void) exclude; +#endif + } +#endif + Buffer::Buffer () : DataPtr (nullptr), DataSize (0), DataAlignment (0) { } @@ -108,7 +150,7 @@ namespace VeraCrypt Memory::Zero (DataPtr, DataSize); } - SecureBuffer::SecureBuffer (size_t size, size_t alignment) + SecureBuffer::SecureBuffer (size_t size, size_t alignment) : AllocatedSize (0), Locked (false) { Allocate (size, alignment); } @@ -121,7 +163,40 @@ namespace VeraCrypt void SecureBuffer::Allocate (size_t size, size_t alignment) { +#ifdef TC_UNIX + if (size < 1) + throw ParameterIncorrect (SRC_POS); + + if (DataPtr != nullptr) + { + if ((DataSize == size) && (DataAlignment == alignment)) + return; + Free(); + } + + const size_t pageSize = GetPageSize(); + if (size > ((size_t) -1) - pageSize) + throw ParameterTooLarge (SRC_POS); + + const size_t allocatedSize = (size + pageSize - 1) / pageSize * pageSize; + // Alignments are powers of two: the larger one satisfies both + uint8 *memory = static_cast (Memory::AllocateAligned (allocatedSize, alignment > pageSize ? alignment : pageSize)); + + // Locking fails if RLIMIT_MEMLOCK is exceeded; the buffer is still usable + Locked = mlock (memory, allocatedSize) == 0; + ExcludeFromCoreDump (memory, allocatedSize, true); + + DataPtr = memory; + DataSize = size; + DataAlignment = alignment; + AllocatedSize = allocatedSize; + + SecureBufferList &list = GetSecureBufferList(); + ScopeLock lock (list.ListMutex); + list.Buffers.insert (this); +#else Buffer::Allocate (size, alignment); +#endif } void SecureBuffer::Free () @@ -129,8 +204,39 @@ namespace VeraCrypt if (DataPtr == nullptr) throw NotInitialized (SRC_POS); +#ifdef TC_UNIX + { + SecureBufferList &list = GetSecureBufferList(); + ScopeLock lock (list.ListMutex); + list.Buffers.erase (this); + } + + burn (DataPtr, AllocatedSize); + ExcludeFromCoreDump (DataPtr, AllocatedSize, false); + if (Locked) + munlock (DataPtr, AllocatedSize); + + Memory::FreeAligned (DataPtr); + DataPtr = nullptr; + DataSize = 0; + DataAlignment = 0; + AllocatedSize = 0; + Locked = false; +#else Erase (); Buffer::Free (); +#endif + } + + void SecureBuffer::LockAllAfterFork () + { +#ifdef TC_UNIX + SecureBufferList &list = GetSecureBufferList(); + ScopeLock lock (list.ListMutex); + + foreach (SecureBuffer *buffer, list.Buffers) + buffer->Locked = mlock (buffer->DataPtr, buffer->AllocatedSize) == 0; +#endif } void BufferPtr::CopyFrom (const ConstBufferPtr &bufferPtr) const diff --git a/src/Platform/Buffer.h b/src/Platform/Buffer.h index 5d14d6a6b8..05e4cc4d91 100644 --- a/src/Platform/Buffer.h +++ b/src/Platform/Buffer.h @@ -107,15 +107,21 @@ namespace VeraCrypt class SecureBuffer : public Buffer { public: - SecureBuffer () { } + SecureBuffer () : AllocatedSize (0), Locked (false) { } SecureBuffer (size_t size, size_t alignment = 0); - SecureBuffer (const ConstBufferPtr &bufferPtr) { CopyFrom (bufferPtr); } + SecureBuffer (const ConstBufferPtr &bufferPtr) : AllocatedSize (0), Locked (false) { CopyFrom (bufferPtr); } virtual ~SecureBuffer (); virtual void Allocate (size_t size, size_t alignment = 0); virtual void Free (); + // Memory locks are not inherited by a forked process + static void LockAllAfterFork (); + private: + size_t AllocatedSize; + bool Locked; + SecureBuffer (const SecureBuffer &); SecureBuffer &operator= (const SecureBuffer &); }; diff --git a/src/Platform/Unix/Process.cpp b/src/Platform/Unix/Process.cpp index 51f0882308..6b7a0eee09 100644 --- a/src/Platform/Unix/Process.cpp +++ b/src/Platform/Unix/Process.cpp @@ -18,9 +18,11 @@ #include #include #include +#include #include #include #ifdef TC_LINUX +#include #include #endif #include "Process.h" @@ -209,6 +211,20 @@ namespace VeraCrypt } #endif + void Process::DisableCoreDumps () + { + // For processes that hold key material. A core_pattern pipe ignores + // RLIMIT_CORE on Linux, but not the dumpable flag, which also keeps + // other processes of the same user from attaching. + struct rlimit limit; + limit.rlim_cur = 0; + limit.rlim_max = 0; + setrlimit (RLIMIT_CORE, &limit); +#ifdef TC_LINUX + prctl (PR_SET_DUMPABLE, 0, 0, 0, 0); +#endif + } + bool Process::IsExecutable(const std::string& path) { struct stat sb; if (stat(path.c_str(), &sb) == 0) { diff --git a/src/Platform/Unix/Process.h b/src/Platform/Unix/Process.h index 3e3a61503a..cf4999e0c5 100644 --- a/src/Platform/Unix/Process.h +++ b/src/Platform/Unix/Process.h @@ -31,6 +31,7 @@ namespace VeraCrypt Process (); virtual ~Process (); + static void DisableCoreDumps (); static bool IsExecutable(const std::string& path); static std::string FindSystemBinary(const char* name, std::string& errorMsg); static string Execute (const string &processName, const list &arguments, int timeOut = -1, ProcessExecFunctor *execFunctor = nullptr, const Buffer *inputData = nullptr); diff --git a/src/Volume/Volume.cpp b/src/Volume/Volume.cpp index 0cee6344c6..9536a5b059 100644 --- a/src/Volume/Volume.cpp +++ b/src/Volume/Volume.cpp @@ -400,7 +400,10 @@ namespace VeraCrypt if (Protection == VolumeProtection::HiddenVolumeReadOnly) CheckProtectedRange (hostOffset, length); - SecureBuffer encBuf (buffer.Size()); + // Encrypted in place right after the copy. A SecureBuffer would lock the + // pages of every write in memory, which slows down writes considerably. + Buffer encBuf (buffer.Size()); + finally_do_arg (Buffer *, &encBuf, { finally_arg->Erase(); }); encBuf.CopyFrom (buffer); EA->EncryptSectors (encBuf, hostOffset / SectorSize, length / SectorSize, SectorSize);