Unix: lock key buffers in memory and exclude them from core dumps

.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

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()

src/Core/Unix/CoreService.cpp

@@ -1136,6 +1136,8 @@ namespace VeraCrypt
 		{
 			try
 			{
+				Process::DisableCoreDumps();
+
 				if (forkProcess)
 					RedirectStandardErrorToDevNull ();
 

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

src/Platform/Buffer.cpp

@@ -12,9 +12,51 @@
 
 #include "Buffer.h"
 #include "Exception.h"
+#ifdef TC_UNIX
+#include <set>
+#include <sys/mman.h>
+#include <unistd.h>
+#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 <SecureBuffer *> 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,16 +163,80 @@ 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 <uint8 *> (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 ()
 	{
 		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

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 &);
 	};