Skip to content

COM interface privilege escalation #1670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
audriusbuika wants to merge 2 commits into
base: master
Choose a base branch
from
+200 −0
Open

COM interface privilege escalation #1670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9a7ae0
Windows: Add input validation whitelists to elevated COM methods
audriusbuika Apr 12, 2026
7f77501
Windows: Add drive number validation to FormatNtfs and FormatFs COM m…
audriusbuika Apr 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 196 additions & 0 deletions src/Common/BaseCom.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,155 @@

using namespace VeraCrypt;

// ========================================================================
// COM method input validation - whitelists and helper functions
// ========================================================================

// Helper: check if IOCTL code is in the allowed list
static BOOL IsIoctlInWhitelist (DWORD ioctl, const DWORD *whitelist, size_t count)
{
for (size_t i = 0; i < count; i++)
{
if (whitelist[i] == ioctl)
return TRUE;
}
return FALSE;
}

// Allowed IOCTL codes for CallDriver (VeraCrypt kernel driver IOCTLs)
static const DWORD g_CallDriverIoctlWhitelist[] = {
TC_IOCTL_GET_DRIVER_VERSION,
TC_IOCTL_GET_BOOT_LOADER_VERSION,
TC_IOCTL_MOUNT_VOLUME,
TC_IOCTL_UNMOUNT_VOLUME,
TC_IOCTL_UNMOUNT_ALL_VOLUMES,
TC_IOCTL_GET_MOUNTED_VOLUMES,
TC_IOCTL_GET_VOLUME_PROPERTIES,
TC_IOCTL_GET_DEVICE_REFCOUNT,
TC_IOCTL_IS_DRIVER_UNLOAD_DISABLED,
TC_IOCTL_IS_ANY_VOLUME_MOUNTED,
TC_IOCTL_GET_PASSWORD_CACHE_STATUS,
TC_IOCTL_WIPE_PASSWORD_CACHE,
TC_IOCTL_OPEN_TEST,
TC_IOCTL_GET_DRIVE_PARTITION_INFO,
TC_IOCTL_GET_DRIVE_GEOMETRY,
TC_IOCTL_PROBE_REAL_DRIVE_SIZE,
TC_IOCTL_GET_RESOLVED_SYMLINK,
TC_IOCTL_GET_BOOT_ENCRYPTION_STATUS,
TC_IOCTL_BOOT_ENCRYPTION_SETUP,
TC_IOCTL_ABORT_BOOT_ENCRYPTION_SETUP,
TC_IOCTL_GET_BOOT_ENCRYPTION_SETUP_RESULT,
TC_IOCTL_GET_BOOT_DRIVE_VOLUME_PROPERTIES,
TC_IOCTL_REOPEN_BOOT_VOLUME_HEADER,
TC_IOCTL_GET_BOOT_ENCRYPTION_ALGORITHM_NAME,
TC_IOCTL_GET_PORTABLE_MODE_STATUS,
TC_IOCTL_SET_PORTABLE_MODE_STATUS,
TC_IOCTL_IS_HIDDEN_SYSTEM_RUNNING,
TC_IOCTL_GET_SYSTEM_DRIVE_CONFIG,
TC_IOCTL_DISK_IS_WRITABLE,
TC_IOCTL_START_DECOY_SYSTEM_WIPE,
TC_IOCTL_ABORT_DECOY_SYSTEM_WIPE,
TC_IOCTL_GET_DECOY_SYSTEM_WIPE_STATUS,
TC_IOCTL_GET_DECOY_SYSTEM_WIPE_RESULT,
TC_IOCTL_WRITE_BOOT_DRIVE_SECTOR,
Copy link
Copy Markdown
Member

@idrassi idrassi Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whitelist currently includes every current driver IOCTL, including mutating or destructive operations such as TC_IOCTL_BOOT_ENCRYPTION_SETUP, TC_IOCTL_WRITE_BOOT_DRIVE_SECTOR, TC_IOCTL_START_DECOY_SYSTEM_WIPE and VC_IOCTL_EMERGENCY_CLEAR_ALL_KEYS.

That does not create a meaningful authorization boundary for the elevated COM server. It only blocks unknown future IOCTLs. Please narrow this to IOCTLs actually needed through elevated COM and add per-IOCTL validation for input/output sizes, structure fields, target device, offsets, and setup state where applicable before dispatching.

TC_IOCTL_GET_WARNING_FLAGS,
TC_IOCTL_SET_SYSTEM_FAVORITE_VOLUME_DIRTY,
TC_IOCTL_REREAD_DRIVER_CONFIG,
TC_IOCTL_GET_SYSTEM_DRIVE_DUMP_CONFIG,
VC_IOCTL_GET_BOOT_LOADER_FINGERPRINT,
VC_IOCTL_GET_DRIVE_GEOMETRY_EX,
VC_IOCTL_EMERGENCY_CLEAR_ALL_KEYS,
VC_IOCTL_IS_RAM_ENCRYPTION_ENABLED,
VC_IOCTL_ENCRYPTION_QUEUE_PARAMS,
};

// Allowed IOCTL codes for DeviceIoControl (standard disk/storage/filesystem IOCTLs)
static const DWORD g_DeviceIoControlWhitelist[] = {
IOCTL_DISK_GET_DRIVE_GEOMETRY,
IOCTL_DISK_GET_PARTITION_INFO,
IOCTL_DISK_GET_PARTITION_INFO_EX,
IOCTL_DISK_GET_DRIVE_GEOMETRY_EX,
IOCTL_DISK_GET_LENGTH_INFO,
IOCTL_DISK_PERFORMANCE,
IOCTL_STORAGE_GET_DEVICE_NUMBER,
IOCTL_STORAGE_READ_CAPACITY,
IOCTL_STORAGE_MANAGE_DATA_SET_ATTRIBUTES,
FSCTL_LOCK_VOLUME,
FSCTL_DISMOUNT_VOLUME,
FSCTL_IS_VOLUME_MOUNTED,
FSCTL_GET_NTFS_VOLUME_DATA,
FSCTL_GET_VOLUME_BITMAP,
FSCTL_GET_RETRIEVAL_POINTERS,
FSCTL_MOVE_FILE,
FSCTL_ALLOW_EXTENDED_DASD_IO,
FSCTL_SET_SPARSE,
FSCTL_EXTEND_VOLUME,
Comment on lines +103 to +106
Copy link
Copy Markdown
Member

@idrassi idrassi Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mutating FSCTLs FSCTL_MOVE_FILE / FSCTL_EXTEND_VOLUME should not be exposed through a generic elevated DeviceIoControl broker without operation-specific validation. With this list, the caller still controls both the target path/device and the input buffer for operations such as FSCTL_MOVE_FILE, FSCTL_EXTEND_VOLUME, FSCTL_SHRINK_VOLUME and
IOCTL_STORAGE_MANAGE_DATA_SET_ATTRIBUTES.

Please remove IOCTLs that are not required by current elevated COM call sites or replace them with dedicated broker methods that validate the exact target, buffer size, fields, and operation context.

FSCTL_SHRINK_VOLUME,
};

// Allowed HKLM registry key path prefixes (case-insensitive)
static const wchar_t* g_RegistryKeyPrefixWhitelist[] = {
L"SYSTEM\\CurrentControlSet\\Services\\veracrypt",
L"SYSTEM\\CurrentControlSet\\Services\\VeraCryptSystemFavorites",
L"SYSTEM\\CurrentControlSet\\Control\\Power",
L"SYSTEM\\CurrentControlSet\\Control\\Session Manager\\Power",
};

// Helper: check if registry key path matches an allowed prefix (exact or subkey)
static BOOL IsRegistryKeyPathAllowed (const wchar_t *keyPath)
{
if (!keyPath)
return FALSE;
for (size_t i = 0; i < ARRAYSIZE (g_RegistryKeyPrefixWhitelist); i++)
{
size_t prefixLen = wcslen (g_RegistryKeyPrefixWhitelist[i]);
if (_wcsnicmp (keyPath, g_RegistryKeyPrefixWhitelist[i], prefixLen) == 0)
{
wchar_t nextChar = keyPath[prefixLen];
if (nextChar == L'\0' || nextChar == L'\\')
return TRUE;
}
}
return FALSE;
}

// Helper: reject paths containing ".." traversal components
static BOOL ValidatePathNoTraversal (const wchar_t *path)
{
if (!path)
return FALSE;
if (wcsstr (path, L"..") != NULL)
Copy link
Copy Markdown
Member

@idrassi idrassi Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a safe path validation policy. It rejects legitimate file names containing "..." but still accepts arbitrary absolute paths such as "C:\Windows..." and it does not handle canonicalization, junctions, symlinks, mount points, or other reparse points.

Please replace this with canonical path validation tied to each operation's expected target. User selected container paths should be treated as a separate workflow with explicit validation.

return FALSE;
return TRUE;
}

// Helper: case-insensitive check if path contains "veracrypt" substring
static BOOL PathContainsVeraCrypt (const wchar_t *path)
{
if (!path)
return FALSE;
size_t pathLen = wcslen (path);
if (pathLen < 9)
return FALSE;
for (size_t i = 0; i <= pathLen - 9; i++)
{
if (_wcsnicmp (&path[i], L"veracrypt", 9) == 0)
Copy link
Copy Markdown
Member

@idrassi idrassi Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for a veracrypt substring is not a safe location policy. A caller can choose arbitrary paths containing that substring and this does not protect against reparse-point redirection.

Please restrict CopyFile/DeleteFile to the exact expected VeraCrypt-managed files after resolving/canonicalizing the path or replace these generic methods with dedicated operations for the known copy/delete workflows.

return TRUE;
}
return FALSE;
}

// Helper: validate device path format (must start with \\.\ or \\?\)
static BOOL IsValidDevicePath (const wchar_t *path)
{
if (!path || wcslen (path) < 4)
return FALSE;
return (path[0] == L'\\' && path[1] == L'\\' && (path[2] == L'.' || path[2] == L'?') && path[3] == L'\\');
Copy link
Copy Markdown
Member

@idrassi idrassi Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks existing elevated device fallback paths. Device::Device opens PhysicalDriveN locally by prepending \.\ but stores the original Path. System-drive configuration uses values like PhysicalDrive. When non-elevated access fails with ERROR_ACCESS_DENIED, Elevator::ReadWriteFile / Elevator::DeviceIoControl passes that original PhysicalDrive0 string to COM and this validator rejects it.

Accepting \.\PhysicalDrive0 here is not sufficient by itself. Device::Device only preserves paths that already start with \?. Other inputs are prefixed with "\." so passing \.\PhysicalDrive0 into Device::Device would produce an invalid doubled prefix. Please normalize device paths consistently in one place while preserving the existing PhysicalDriveN elevated fallback behavior.

}

// ========================================================================


HRESULT CreateElevatedComObject (HWND hwnd, REFGUID guid, REFIID iid, void **ppv)
{
WCHAR monikerName[1024];
Expand Down Expand Up @@ -81,6 +230,9 @@ BOOL ComGetInstanceBase (HWND hWnd, REFCLSID clsid, REFIID iid, void **tcServer)

DWORD BaseCom::CallDriver (DWORD ioctl, BSTR input, BSTR *output)
{
if (!IsIoctlInWhitelist (ioctl, g_CallDriverIoctlWhitelist, ARRAYSIZE (g_CallDriverIoctlWhitelist)))
return ERROR_ACCESS_DENIED;

try
{
BootEncryption bootEnc (NULL);
Expand Down Expand Up @@ -108,6 +260,11 @@ DWORD BaseCom::CallDriver (DWORD ioctl, BSTR input, BSTR *output)

DWORD BaseCom::CopyFile (BSTR sourceFile, BSTR destinationFile)
{
if (!ValidatePathNoTraversal (sourceFile) || !ValidatePathNoTraversal (destinationFile))
return ERROR_ACCESS_DENIED;

if (!PathContainsVeraCrypt (sourceFile) || !PathContainsVeraCrypt (destinationFile))
return ERROR_ACCESS_DENIED;

if (!::CopyFileW (sourceFile, destinationFile, FALSE))
return GetLastError();
Expand All @@ -118,6 +275,11 @@ DWORD BaseCom::CopyFile (BSTR sourceFile, BSTR destinationFile)

DWORD BaseCom::DeleteFile (BSTR file)
{
if (!ValidatePathNoTraversal (file))
return ERROR_ACCESS_DENIED;

if (!PathContainsVeraCrypt (file))
return ERROR_ACCESS_DENIED;

if (!::DeleteFileW (file))
return GetLastError();
Expand All @@ -134,6 +296,17 @@ BOOL BaseCom::IsPagingFileActive (BOOL checkNonWindowsPartitionsOnly)

DWORD BaseCom::ReadWriteFile (BOOL write, BOOL device, BSTR filePath, BSTR *bufferBstr, unsigned __int64 offset, unsigned __int32 size, DWORD *sizeDone)
{
if (device)
{
if (!IsValidDevicePath (filePath))
Copy link
Copy Markdown
Member

@idrassi idrassi Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For device == TRUE, this currently performs only syntactic prefix validation. Even after the path normalization issue is fixed, this remains a generic elevated raw device read/write primitive.

Please constrain elevated device reads/writes to the boot/system-drive workflows that require them and validate the target device plus permitted offsets and sizes for each operation.

return ERROR_ACCESS_DENIED;
}
else
{
if (!ValidatePathNoTraversal (filePath))
return ERROR_ACCESS_DENIED;
}

try
{
unique_ptr <File> file (device ? new Device (filePath, !write) : new File (filePath, !write));
Expand Down Expand Up @@ -172,6 +345,9 @@ DWORD BaseCom::GetFileSize (BSTR filePath, unsigned __int64 *pSize)
if (!pSize)
return ERROR_INVALID_PARAMETER;

if (!ValidatePathNoTraversal (filePath))
return ERROR_ACCESS_DENIED;

try
{
std::wstring path (filePath);
Expand All @@ -198,6 +374,20 @@ DWORD BaseCom::GetFileSize (BSTR filePath, unsigned __int64 *pSize)

DWORD BaseCom::DeviceIoControl (BOOL readOnly, BOOL device, BSTR filePath, DWORD dwIoControlCode, BSTR input, BSTR *output)
{
if (!IsIoctlInWhitelist (dwIoControlCode, g_DeviceIoControlWhitelist, ARRAYSIZE (g_DeviceIoControlWhitelist)))
return ERROR_ACCESS_DENIED;

if (device)
{
if (!IsValidDevicePath (filePath))
return ERROR_ACCESS_DENIED;
}
else
{
if (!ValidatePathNoTraversal (filePath))
return ERROR_ACCESS_DENIED;
}

try
{
unique_ptr <File> file (device ? new Device (filePath, readOnly == TRUE) : new File (filePath, readOnly == TRUE));
Expand Down Expand Up @@ -302,6 +492,9 @@ DWORD BaseCom::SetDriverServiceStartType (DWORD startType)

DWORD BaseCom::WriteLocalMachineRegistryDwordValue (BSTR keyPath, BSTR valueName, DWORD value)
{
if (!IsRegistryKeyPathAllowed (keyPath))
Copy link
Copy Markdown
Member

@idrassi idrassi Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prefix whitelist is still too broad for a generic elevated DWORD write method. Current callers only need exact writes such as veracrypt\VeraCryptConfig with known config bits, VeraCryptEnableMemoryProtection/VeraCryptEnableScreenProtection as 0 or 1, VeraCryptEncryptionFreeCpuCount within the CPU-count limit, VeraCryptSystemFavorites\VeraCryptSystemFavoritesConfig with known service-config bits and HibernateEnabled/HiberbootEnabled set to 0.

Please validate exact (keyPath, valueName, value) combinations. For example, this method should not allow unrelated service values such as SYSTEM\CurrentControlSet\Services\veracrypt\Start: service start changes already have the dedicated SetDriverServiceStartType path.

return ERROR_ACCESS_DENIED;

if (!::WriteLocalMachineRegistryDword (keyPath, valueName, value))
return GetLastError();

Expand Down Expand Up @@ -500,5 +693,8 @@ DWORD BaseCom::NotifyService(DWORD dwNotifyCode)

DWORD BaseCom::FastFileResize (BSTR filePath, __int64 fileSize)
{
if (!ValidatePathNoTraversal (filePath))
Copy link
Copy Markdown
Member

@idrassi idrassi Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FastFileResize is especially sensitive because it enables SE_MANAGE_VOLUME_NAME and calls SetFileValidData. A ".." substring check is not enough for a privileged operation on a caller-provided path.

Please validate that the target is the container file created by the formatting workflow, reject reparse-point/symlink redirection and avoid accepting arbitrary absolute paths through this elevated method.

return ERROR_ACCESS_DENIED;

return ::FastResizeFile (filePath, fileSize);
}
4 changes: 4 additions & 0 deletions src/Format/FormatCom.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ class TrueCryptFormatCom : public ITrueCryptFormatCom

virtual BOOL STDMETHODCALLTYPE FormatNtfs (int driveNo, int clusterSize)
{
if (driveNo < 0 || driveNo > 25)
return FALSE;
Copy link
Copy Markdown
Member

@idrassi idrassi Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns FALSE but UacFormatNtfs consumes the COM return value as an int status where 0 means success and non-zero means failure. Invalid input would therefore be reported as a successful format.

Please return a non-zero failure code such as ERROR_INVALID_DRIVE or ERROR_INVALID_PARAMETER or update the COM method contract consistently so invalid input
cannot be interpreted as success.

return ::FormatNtfs (driveNo, clusterSize, TRUE);
}

Expand Down Expand Up @@ -134,6 +136,8 @@ class TrueCryptFormatCom : public ITrueCryptFormatCom

virtual BOOL STDMETHODCALLTYPE FormatFs (int driveNo, int clusterSize, int fsType)
{
if (driveNo < 0 || driveNo > 25)
return FALSE;
Copy link
Copy Markdown
Member

@idrassi idrassi Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as FormatNtfs: FALSE is 0 and the caller treats 0 as success. Please return a non-zero error code for invalid drive numbers.

return ::FormatFs (driveNo, clusterSize, fsType, TRUE);
}

Expand Down