Skip to content

Commit 8851b7b

Browse files
planetchiliplanetchili
committed
simplify and improve secure subdir util
1 parent d75f98e commit 8851b7b

File tree

2 files changed

+87
-135
lines changed

2 files changed

+87
-135
lines changed

‎IntelPresentMon/CommonUtilities/file/SecureSubdirectory.cpp‎

Lines changed: 85 additions & 135 deletions
Original file line numberDiff line numberDiff line change
@@ -2,28 +2,25 @@
22
#include "../win/WinAPI.h"
33
#include "../win/Handle.h"
44
#include "../win/HrError.h"
5-
#include <winternl.h>
5+
#include "../win/Security.h"
66
#include <winioctl.h>
7-
#include <sddl.h>
87
#include <aclapi.h>
98
#include <vector>
109
#include <filesystem>
1110
#include "../Exception.h"
1211
#include "../log/Log.h"
1312

14-
#pragma comment(lib, "Advapi32.lib")
15-
#pragma comment(lib, "Ntdll.lib")
16-
1713
namespace fs = std::filesystem;
1814

1915
namespace pmon::util::file
2016
{
2117
// internal helpers
2218
namespace
2319
{
20+
// open a directory into handle without following reparse at the path leaf
2421
win::Handle OpenDirNoFollow_(const fs::path& dirPath,
25-
DWORD desiredAccess = FILE_LIST_DIRECTORY | READ_CONTROL | WRITE_DAC | SYNCHRONIZE,
26-
DWORD share = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE)
22+
DWORD desiredAccess = FILE_LIST_DIRECTORY | READ_CONTROL | WRITE_DAC | SYNCHRONIZE | DELETE,
23+
DWORD share = FILE_SHARE_READ | FILE_SHARE_WRITE)
2724
{
2825
auto h = (win::Handle)CreateFileW(
2926
dirPath.c_str(),
@@ -52,25 +49,20 @@ namespace pmon::util::file
5249
hdr.ReparseTag = tag;
5350
hdr.ReparseDataLength = 0;
5451
hdr.Reserved = 0;
55-
5652
DWORD bytes = 0;
57-
BOOL ok = DeviceIoControl(
53+
return (bool)DeviceIoControl(
5854
h,
5955
FSCTL_DELETE_REPARSE_POINT,
6056
&hdr,
6157
sizeof(hdr),
6258
nullptr,
6359
0,
6460
&bytes,
65-
nullptr);
66-
67-
if (!ok) {
68-
pmlog_warn("Could not delete reparse point by handle").hr();
69-
return false;
70-
}
71-
return true;
61+
nullptr
62+
);
7263
}
7364

65+
// check if a directory referenced by the handle has a reparse point
7466
std::optional<DWORD> IsReparseByHandle_(HANDLE h)
7567
{
7668
FILE_ATTRIBUTE_TAG_INFO tag{};
@@ -85,142 +77,101 @@ namespace pmon::util::file
8577
}
8678
}
8779

88-
// Create/open a directory named 'leafName' as a child of 'parent' without following reparses.
80+
// Create/open a directory named 'leafName' as a child of 'parent'
81+
// If running elevated, additionally undertake the following defensive measures:
8982
// If the entry is a reparse point, attempt to convert it in place by deleting its reparse
9083
// attribute; if that fails, delete and recreate as a real directory.
91-
// Apply SYSTEM-only DACL by handle if isElevated == true.
92-
fs::path CreateOrOpenDirSystemOnlySecure_(const fs::path& parent,
84+
win::Handle CreateOrOpenDirSystemOnlySecure_(const fs::path& parent,
9385
const std::wstring& leafName,
9486
bool isElevated)
9587
{
9688
if (leafName.find_first_of(L"/\\") != std::wstring::npos) {
9789
throw Except<Exception>("leafName must not contain path separators");
9890
}
9991

100-
// 1) Open parent directory by handle (no-follow) and verify/normalize reparse if present.
101-
DWORD parentAccess = FILE_LIST_DIRECTORY | SYNCHRONIZE | READ_CONTROL;
102-
if (isElevated) parentAccess |= WRITE_DAC;
92+
const auto fullpath = parent / leafName;
10393

104-
auto hParent = (win::Handle)OpenDirNoFollow_(parent, parentAccess);
105-
if (!hParent) {
106-
throw Except<win::HrError>("Open parent directory failed");
94+
// setup ACL to apply
95+
UniqueLocalPtr<void> pSecDesc;
96+
if (isElevated) {
97+
pSecDesc = win::MakeSecurityDescriptor("D:P(A;OICI;FA;;;SY)");
10798
}
108-
109-
if (auto reparseTag = IsReparseByHandle_(hParent)) {
110-
// Try to delete the reparse attribute on the parent (best-effort).
111-
if (!TryDeleteReparsePointByHandle_(hParent, *reparseTag)) {
112-
throw Except<Exception>("Parent directory is a reparse point and could not be normalized");
99+
SECURITY_ATTRIBUTES secAttr{
100+
.nLength = sizeof(SECURITY_ATTRIBUTES),
101+
.lpSecurityDescriptor = pSecDesc.get(),
102+
};
103+
104+
// 1. try and create with ACL
105+
const auto CreateNew = [&] {
106+
if (CreateDirectoryW(fullpath.c_str(), &secAttr)) {
107+
// successfully created new directory
108+
pmlog_dbg("Created new temp subdir").pmwatch(fullpath.string());
109+
return true;
113110
}
114-
// Re-check; if still reparse, bail.
115-
if (IsReparseByHandle_(hParent)) {
116-
throw Except<Exception>("Parent directory remains a reparse point after normalization");
111+
else if (HRESULT hr = GetLastError(); hr != ERROR_ALREADY_EXISTS) {
112+
throw Except<win::HrError>(hr, "Failed to create secure subdir");
117113
}
118-
}
119-
120-
// 2) Create/open the child directory *by name relative to parent handle* via NtCreateFile.
121-
UNICODE_STRING uName{};
122-
uName.Buffer = const_cast<wchar_t*>(leafName.c_str());
123-
uName.Length = static_cast<USHORT>(leafName.size() * sizeof(WCHAR));
124-
uName.MaximumLength = uName.Length;
125-
126-
OBJECT_ATTRIBUTES oa{};
127-
InitializeObjectAttributes(&oa, &uName, OBJ_CASE_INSENSITIVE, hParent, nullptr);
128-
129-
IO_STATUS_BLOCK ios{};
130-
win::Handle hChild;
114+
return false;
115+
};
116+
const auto isFresh = CreateNew();
131117

132-
ACCESS_MASK childAccess = FILE_LIST_DIRECTORY | SYNCHRONIZE | READ_CONTROL | FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES;
133-
if (isElevated) childAccess |= WRITE_DAC;
134-
NTSTATUS st = NtCreateFile(
135-
hChild.ClearAndGetAddressOf(),
136-
childAccess,
137-
&oa,
138-
&ios,
139-
nullptr, // AllocationSize
140-
FILE_ATTRIBUTE_DIRECTORY,
141-
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
142-
FILE_OPEN_IF, // create if missing, open if exists
143-
FILE_DIRECTORY_FILE | FILE_SYNCHRONOUS_IO_NONALERT, // directory open/create
144-
nullptr,
145-
0);
146-
if (st < 0) {
147-
throw Except<Exception>(std::format("NtCreateFile(directory) failed; NTSTATUS:{}", st));
118+
// 2. open the existing dir without following any reparse on it
119+
auto hExistingSubdir = OpenDirNoFollow_(fullpath);
120+
if (!hExistingSubdir) {
121+
throw Except<win::HrError>("Failed to open existing subdir by handle");
148122
}
149123

150-
// 3) If the child is a reparse point, attempt to normalize it.
151-
if (auto reparseTag = IsReparseByHandle_(hChild)) {
152-
if (bool normalized = TryDeleteReparsePointByHandle_(hChild, *reparseTag); !normalized) {
153-
// Close the handle and attempt delete-and-recreate as a plain directory.
154-
hChild.Clear();
155-
156-
fs::path full = fs::path(parent) / leafName;
157-
if (!RemoveDirectoryW(full.c_str())) {
158-
pmlog_warn("Failed remove reparse as dir").hr();
159-
// As a last-ditch attempt, if it's a file reparse, try DeleteFileW
160-
if (!DeleteFileW(full.c_str())) {
161-
pmlog_error("Could not remove reparse point").hr();
162-
}
124+
// if dir is not fresh and we are elevated, we have defensive work to do
125+
if (!isFresh && isElevated) {
126+
// 3. own the existing dir
127+
{
128+
// get the dacl out of the security description
129+
PACL pDacl = nullptr;
130+
BOOL daclPresent = FALSE, daclDefaulted = FALSE;
131+
if (!GetSecurityDescriptorDacl(pSecDesc.get(), &daclPresent, &pDacl, &daclDefaulted)) {
132+
throw Except<win::HrError>("GetSecurityDescriptorDacl failed");
163133
}
164-
165-
// Recreate with FILE_CREATE to ensure a brand-new real directory
166-
st = NtCreateFile(
167-
hChild.ClearAndGetAddressOf(),
168-
childAccess,
169-
&oa,
170-
&ios,
171-
nullptr,
172-
FILE_ATTRIBUTE_DIRECTORY,
173-
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
174-
FILE_CREATE,
175-
FILE_DIRECTORY_FILE | FILE_SYNCHRONOUS_IO_NONALERT,
176-
nullptr,
177-
0);
178-
if (st < 0) {
179-
throw Except<Exception>(std::format(
180-
"Recreate plain directory replacing reparse point failed; NTSTATUS:{}", st));
134+
// set security on the directory object
135+
if (auto rc = SetSecurityInfo(
136+
hExistingSubdir,
137+
SE_FILE_OBJECT,
138+
DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION,
139+
nullptr, // owner
140+
nullptr, // group
141+
pDacl,
142+
nullptr // sacl
143+
); rc != ERROR_SUCCESS) {
144+
throw Except<win::HrError>(HRESULT(rc), "SetSecurityInfo(DACL) failed");
181145
}
182146
}
183-
// After dealing with reparse point, confirm it's now plain.
184-
if (IsReparseByHandle_(hChild)) {
185-
throw Except<Exception>("Directory remains a reparse point after cleansing efforts");
186-
}
187-
}
188-
189-
// 4) Apply SYSTEM-only DACL by handle (no path) - only if running elevated.
190-
if (isElevated) {
191-
// SDDL: D:P(A;OICI;FA;;;SY) -> protected DACL, SYSTEM full, inherits to children.
192-
LPCWSTR sddl = L"D:P(A;OICI;FA;;;SY)";
193-
PSECURITY_DESCRIPTOR psd = nullptr;
194-
if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(sddl, SDDL_REVISION_1, &psd, nullptr)) {
195-
throw Except<win::HrError>("ConvertStringSecurityDescriptorToSecurityDescriptorW failed");
196-
}
197-
198-
PACL dacl = nullptr;
199-
BOOL daclPresent = FALSE, daclDefaulted = FALSE;
200-
if (!GetSecurityDescriptorDacl(psd, &daclPresent, &dacl, &daclDefaulted)) {
201-
::LocalFree(psd);
202-
throw Except<win::HrError>("GetSecurityDescriptorDacl failed");
203-
}
204147

205-
DWORD rc = SetSecurityInfo(
206-
hChild,
207-
SE_FILE_OBJECT,
208-
DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION,
209-
nullptr, // owner
210-
nullptr, // group
211-
dacl,
212-
nullptr // sacl
213-
);
214-
215-
::LocalFree(psd);
216-
217-
if (rc != ERROR_SUCCESS) {
218-
throw Except<win::HrError>((HRESULT)rc, "SetSecurityInfo(DACL) failed");
148+
// 3. check for reparse
149+
if (auto tag = IsReparseByHandle_(hExistingSubdir)) {
150+
pmlog_warn("detected reparse point when estabilishing subdir").pmwatch(fullpath.string());
151+
// 3a. try and remove reparse point
152+
if (TryDeleteReparsePointByHandle_(hExistingSubdir, *tag)) {
153+
pmlog_dbg("deleted reparse point from subdir");
154+
}
155+
else {
156+
pmlog_warn("Failed to delete reparse point from subdir");
157+
// 3b. try to delete entire directory
158+
hExistingSubdir.Clear();
159+
fs::remove_all(fullpath);
160+
// 3c. try and create anew with ACL
161+
if (!CreateNew()) {
162+
throw Except<Exception>("Failed to create new directory after deleting existing");
163+
}
164+
hExistingSubdir = OpenDirNoFollow_(fullpath);
165+
}
166+
// 3d. final check that directory no longer has reparse
167+
if (IsReparseByHandle_(hExistingSubdir)) {
168+
throw Except<Exception>("Could not neutralize reparse obstacle");
169+
}
219170
}
220171
}
221172

222-
// 5) Return the final absolute path for convenience.
223-
return parent / leafName;
173+
// we are in the clear
174+
return hExistingSubdir;
224175
}
225176
}
226177

@@ -230,14 +181,11 @@ namespace pmon::util::file
230181
bool deleteOnDestruct,
231182
bool clearOnConstruct)
232183
{
233-
if (name.empty()) {
234-
throw Except<Exception>("name must not be empty");
235-
}
236-
237184
SecureSubdirectory d;
238185
d.isElevated_ = isElevated;
239186
d.deleteOnDestruct_ = deleteOnDestruct;
240-
d.path_ = CreateOrOpenDirSystemOnlySecure_(parent, name, isElevated);
187+
d.path_ = parent / name;
188+
d.hDirectory_ = CreateOrOpenDirSystemOnlySecure_(parent, name, isElevated);
241189

242190
if (clearOnConstruct) {
243191
d.Clear();
@@ -270,7 +218,8 @@ namespace pmon::util::file
270218
:
271219
path_(std::move(other.path_)),
272220
deleteOnDestruct_(other.deleteOnDestruct_),
273-
isElevated_(other.isElevated_)
221+
isElevated_(other.isElevated_),
222+
hDirectory_(std::move(other.hDirectory_))
274223
{
275224
other.deleteOnDestruct_ = false;
276225
other.path_.clear();
@@ -291,6 +240,7 @@ namespace pmon::util::file
291240
path_ = std::move(other.path_);
292241
deleteOnDestruct_ = other.deleteOnDestruct_;
293242
isElevated_ = other.isElevated_;
243+
hDirectory_ = std::move(other.hDirectory_);
294244
other.deleteOnDestruct_ = false;
295245
other.path_.clear();
296246
other.isElevated_ = false;

‎IntelPresentMon/CommonUtilities/file/SecureSubdirectory.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
#include <filesystem>
44
#include <string>
5+
#include "../win/Handle.h"
56

67
namespace pmon::util::file
78
{
@@ -33,6 +34,7 @@ namespace pmon::util::file
3334

3435
private:
3536
// data
37+
win::Handle hDirectory_;
3638
std::filesystem::path path_;
3739
bool deleteOnDestruct_ = false;
3840
bool isElevated_ = false;

0 commit comments

Comments
 (0)