Windows: Fix elevated COM format drive validation and device path normalization#1670
Conversation
|
Thank you for looking into this area. The validation policy also needs to be narrower. The driver IOCTL whitelist currently includes all current VeraCrypt driver IOCTLs, including mutating operations, so it does not materially constrain the elevated broker beyond blocking unknown codes. The DeviceIoControl list also includes mutating FSCTLs that do not appear to be required by current elevated COM call sites. For path-based methods, substring checks like "contains veracrypt" and .. rejection are not a reliable location policy. I think this should be reworked incrementally: first preserve existing elevated boot/system drive behavior, fix the format return value bug, and derive whitelists from actual elevated COM call sites. Then add per-operation validation where practical, with special treatment for workflows that genuinely require caller-provided paths, such as fast creation of user selected containers. |
| FSCTL_MOVE_FILE, | ||
| FSCTL_ALLOW_EXTENDED_DASD_IO, | ||
| FSCTL_SET_SPARSE, | ||
| FSCTL_EXTEND_VOLUME, |
There was a problem hiding this comment.
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.
| return FALSE; | ||
| for (size_t i = 0; i <= pathLen - 9; i++) | ||
| { | ||
| if (_wcsnicmp (&path[i], L"veracrypt", 9) == 0) |
There was a problem hiding this comment.
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.
|
Thank you for reworking this. The current final diff is much safer than the previous version. The remaining changes look reasonable to me as a small correctness fix:
I would only ask that the PR be re-scoped to match what it now does. In its current state, it no longer fixes the broader elevated COM privilege-escalation surface: it fixes two correctness issues discovered while reviewing that area. A title such as "Windows: Fix elevated COM format drive validation and device path normalization" would be more accurate, with the broader COM hardening deferred to a follow-up work. Can you please do this? |
This PR addresses two correctness issues discovered while reviewing the
elevated COM surface on Windows. It is intentionally narrow in scope: it
fixes the specific bugs below and does not attempt the broader COM
privilege-escalation hardening, which will be handled in a separate
follow-up.
Changes
1.
Device::Device— avoid double-prefixing device pathsPreviously,
Device::Devicecould prepend\\.\to a path that alreadybegan with
\\.\, producing a malformed device path. The constructor nowchecks whether the input already starts with
\\.\and only adds theprefix when it is missing. The existing
PhysicalDriveNhandling ispreserved unchanged.
2.
FormatNtfs/FormatFs— return a non-zero error for invalid drive numbersPreviously, when given an invalid drive number,
FormatNtfsandFormatFscould return a success status without performing anyformatting. Because
UacFormatNtfs/UacFormatFsforward this statusacross the COM boundary, the elevated callers would interpret invalid
input as a successful format. Both functions now return a non-zero error
code for invalid drive numbers, so the UAC wrappers correctly propagate
the failure to the caller.
Scope
This PR is limited to the two correctness fixes above. The broader
hardening of the elevated COM privilege-escalation surface that
originally motivated this review is deferred to a follow-up PR.