Skip to content

Commit 8f3ac7e

Browse files
committed
[ADVAPI32][RPCRT4][UMPNPMGR][WINLOGON] Fix buffer size retrieval for MakeSelfRelativeSD() (reactos#8395)
Based on Timo's observation in PR reactos#8387. And improve debug ERR strings for last-error values.
1 parent c76049e commit 8f3ac7e

File tree

4 files changed

+73
-102
lines changed

4 files changed

+73
-102
lines changed

base/services/umpnpmgr/install.c

Lines changed: 19 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -369,6 +369,7 @@ CreatePnpInstallEventSecurity(
369369
goto Quit;
370370
}
371371

372+
/* Compute the size needed for the DACL and allocate it */
372373
DaclSize = sizeof(ACL) +
373374
sizeof(ACCESS_ALLOWED_ACE) + GetLengthSid(SystemSid) +
374375
sizeof(ACCESS_ALLOWED_ACE) + GetLengthSid(AdminsSid);
@@ -379,7 +380,6 @@ CreatePnpInstallEventSecurity(
379380
ErrCode = ERROR_OUTOFMEMORY;
380381
goto Quit;
381382
}
382-
383383
if (!InitializeAcl(Dacl, DaclSize, ACL_REVISION))
384384
{
385385
ErrCode = GetLastError();
@@ -428,48 +428,40 @@ CreatePnpInstallEventSecurity(
428428
goto Quit;
429429
}
430430

431-
if (!MakeSelfRelativeSD(&AbsoluteSd, NULL, &Size) && GetLastError() == ERROR_INSUFFICIENT_BUFFER)
431+
/* Retrieve the size needed for the relative SD */
432+
if (MakeSelfRelativeSD(&AbsoluteSd, NULL, &Size) ||
433+
(GetLastError() != ERROR_INSUFFICIENT_BUFFER))
432434
{
433-
RelativeSd = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, Size);
434-
if (RelativeSd == NULL)
435-
{
436-
ErrCode = ERROR_OUTOFMEMORY;
437-
goto Quit;
438-
}
435+
ErrCode = GetLastError();
436+
goto Quit;
437+
}
439438

440-
if (!MakeSelfRelativeSD(&AbsoluteSd, RelativeSd, &Size))
441-
{
442-
ErrCode = GetLastError();
443-
goto Quit;
444-
}
439+
/* Build the relative SD */
440+
RelativeSd = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, Size);
441+
if (RelativeSd == NULL)
442+
{
443+
ErrCode = ERROR_OUTOFMEMORY;
444+
goto Quit;
445+
}
446+
if (!MakeSelfRelativeSD(&AbsoluteSd, RelativeSd, &Size))
447+
{
448+
ErrCode = GetLastError();
449+
HeapFree(GetProcessHeap(), 0, RelativeSd);
450+
goto Quit;
445451
}
446452

447453
*EventSd = RelativeSd;
448454
ErrCode = ERROR_SUCCESS;
449455

450456
Quit:
451457
if (SystemSid)
452-
{
453458
FreeSid(SystemSid);
454-
}
455459

456460
if (AdminsSid)
457-
{
458461
FreeSid(AdminsSid);
459-
}
460462

461463
if (Dacl)
462-
{
463464
HeapFree(GetProcessHeap(), 0, Dacl);
464-
}
465-
466-
if (ErrCode != ERROR_SUCCESS)
467-
{
468-
if (RelativeSd)
469-
{
470-
HeapFree(GetProcessHeap(), 0, RelativeSd);
471-
}
472-
}
473465

474466
return ErrCode;
475467
}

base/system/winlogon/security.c

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,11 @@ ConvertToSelfRelative(
6767
PSECURITY_DESCRIPTOR RelativeSd;
6868
DWORD DescriptorLength = 0;
6969

70-
/* Determine the size for our buffer to allocate */
71-
if (!MakeSelfRelativeSD(AbsoluteSd, NULL, &DescriptorLength) && GetLastError() != ERROR_INSUFFICIENT_BUFFER)
70+
/* Determine the size for allocating our buffer */
71+
if (MakeSelfRelativeSD(AbsoluteSd, NULL, &DescriptorLength) ||
72+
(GetLastError() != ERROR_INSUFFICIENT_BUFFER))
7273
{
73-
ERR("ConvertToSelfRelative(): Unexpected error code (error code %lu -- must be ERROR_INSUFFICIENT_BUFFER)\n", GetLastError());
74+
ERR("ConvertToSelfRelative(): error %lu, expected ERROR_INSUFFICIENT_BUFFER\n", GetLastError());
7475
return NULL;
7576
}
7677

@@ -80,14 +81,14 @@ ConvertToSelfRelative(
8081
DescriptorLength);
8182
if (RelativeSd == NULL)
8283
{
83-
ERR("ConvertToSelfRelative(): Failed to allocate buffer for relative SD!\n");
84+
ERR("ConvertToSelfRelative(): Failed to allocate buffer for relative SD\n");
8485
return NULL;
8586
}
8687

8788
/* Convert the security descriptor now */
8889
if (!MakeSelfRelativeSD(AbsoluteSd, RelativeSd, &DescriptorLength))
8990
{
90-
ERR("ConvertToSelfRelative(): Failed to convert the security descriptor to a self relative format (error code %lu)\n", GetLastError());
91+
ERR("ConvertToSelfRelative(): Failed to convert the security descriptor to a self relative format (error %lu)\n", GetLastError());
9192
RtlFreeHeap(RtlGetProcessHeap(), 0, RelativeSd);
9293
return NULL;
9394
}

dll/win32/advapi32/misc/logon.c

Lines changed: 22 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ CreateDefaultProcessSecurityCommon(
164164
TokenOwnerSize);
165165
if (OwnerOfToken == NULL)
166166
{
167-
ERR("CreateDefaultProcessSecurityCommon(): Failed to allocate buffer for token owner!\n");
167+
ERR("CreateDefaultProcessSecurityCommon(): Failed to allocate buffer for token owner\n");
168168
return FALSE;
169169
}
170170

@@ -200,7 +200,7 @@ CreateDefaultProcessSecurityCommon(
200200
PrimaryGroupSize);
201201
if (PrimaryGroupOfToken == NULL)
202202
{
203-
ERR("CreateDefaultProcessSecurityCommon(): Failed to allocate buffer for primary group token!\n");
203+
ERR("CreateDefaultProcessSecurityCommon(): Failed to allocate buffer for primary group token\n");
204204
Success = FALSE;
205205
goto Quit;
206206
}
@@ -225,7 +225,7 @@ CreateDefaultProcessSecurityCommon(
225225
0, 0, 0, 0, 0, 0, 0,
226226
&SystemSid))
227227
{
228-
ERR("CreateDefaultProcessSecurityCommon(): Failed to create Local System SID (error code %d)\n", GetLastError());
228+
ERR("CreateDefaultProcessSecurityCommon(): Failed to create Local System SID (error %lu)\n", GetLastError());
229229
Success = FALSE;
230230
goto Quit;
231231
}
@@ -245,15 +245,15 @@ CreateDefaultProcessSecurityCommon(
245245
DaclSize);
246246
if (Dacl == NULL)
247247
{
248-
ERR("CreateDefaultProcessSecurityCommon(): Failed to allocate buffer for DACL!\n");
248+
ERR("CreateDefaultProcessSecurityCommon(): Failed to allocate buffer for DACL\n");
249249
Success = FALSE;
250250
goto Quit;
251251
}
252252

253253
/* Initialize the DACL */
254254
if (!InitializeAcl(Dacl, DaclSize, ACL_REVISION))
255255
{
256-
ERR("CreateDefaultProcessSecurityCommon(): Failed to initialize DACL (error code %d)\n", GetLastError());
256+
ERR("CreateDefaultProcessSecurityCommon(): Failed to initialize DACL (error %lu)\n", GetLastError());
257257
Success = FALSE;
258258
goto Quit;
259259
}
@@ -264,7 +264,7 @@ CreateDefaultProcessSecurityCommon(
264264
GENERIC_ALL,
265265
OwnerSid))
266266
{
267-
ERR("CreateDefaultProcessSecurityCommon(): Failed to set up ACE for owner (error code %d)\n", GetLastError());
267+
ERR("CreateDefaultProcessSecurityCommon(): Failed to set up ACE for owner (error %lu)\n", GetLastError());
268268
Success = FALSE;
269269
goto Quit;
270270
}
@@ -275,39 +275,39 @@ CreateDefaultProcessSecurityCommon(
275275
GENERIC_ALL,
276276
SystemSid))
277277
{
278-
ERR("CreateDefaultProcessSecurityCommon(): Failed to set up ACE for SYSTEM (error code %d)\n", GetLastError());
278+
ERR("CreateDefaultProcessSecurityCommon(): Failed to set up ACE for SYSTEM (error %lu)\n", GetLastError());
279279
Success = FALSE;
280280
goto Quit;
281281
}
282282

283283
/* Initialize the descriptor in absolute format */
284284
if (!InitializeSecurityDescriptor(&AbsoluteSd, SECURITY_DESCRIPTOR_REVISION))
285285
{
286-
ERR("CreateDefaultProcessSecurityCommon(): Failed to initialize absolute security descriptor (error code %d)\n", GetLastError());
286+
ERR("CreateDefaultProcessSecurityCommon(): Failed to initialize absolute security descriptor (error %lu)\n", GetLastError());
287287
Success = FALSE;
288288
goto Quit;
289289
}
290290

291291
/* Set the DACL to the security descriptor */
292292
if (!SetSecurityDescriptorDacl(&AbsoluteSd, TRUE, Dacl, FALSE))
293293
{
294-
ERR("CreateDefaultProcessSecurityCommon(): Failed to set up DACL to absolute security descriptor (error code %d)\n", GetLastError());
294+
ERR("CreateDefaultProcessSecurityCommon(): Failed to set up DACL to absolute security descriptor (error %lu)\n", GetLastError());
295295
Success = FALSE;
296296
goto Quit;
297297
}
298298

299299
/* Set the owner for this descriptor */
300300
if (!SetSecurityDescriptorOwner(&AbsoluteSd, OwnerSid, FALSE))
301301
{
302-
ERR("CreateDefaultProcessSecurityCommon(): Failed to set up owner to absolute security descriptor (error code %d)\n", GetLastError());
302+
ERR("CreateDefaultProcessSecurityCommon(): Failed to set up owner to absolute security descriptor (error %lu)\n", GetLastError());
303303
Success = FALSE;
304304
goto Quit;
305305
}
306306

307307
/* Set the primary group for this descriptor */
308308
if (!SetSecurityDescriptorGroup(&AbsoluteSd, PrimaryGroupSid, FALSE))
309309
{
310-
ERR("CreateDefaultProcessSecurityCommon(): Failed to set up group to absolute security descriptor (error code %d)\n", GetLastError());
310+
ERR("CreateDefaultProcessSecurityCommon(): Failed to set up group to absolute security descriptor (error %lu)\n", GetLastError());
311311
Success = FALSE;
312312
goto Quit;
313313
}
@@ -318,9 +318,10 @@ CreateDefaultProcessSecurityCommon(
318318
* to hold the descriptor in a converted self
319319
* relative format.
320320
*/
321-
if (!MakeSelfRelativeSD(&AbsoluteSd, NULL, &RelativeSDSize) && GetLastError() != ERROR_INSUFFICIENT_BUFFER)
321+
if (MakeSelfRelativeSD(&AbsoluteSd, NULL, &RelativeSDSize) ||
322+
(GetLastError() != ERROR_INSUFFICIENT_BUFFER))
322323
{
323-
ERR("CreateDefaultProcessSecurityCommon(): Unexpected error code (error code %d -- must be ERROR_INSUFFICIENT_BUFFER)\n", GetLastError());
324+
ERR("CreateDefaultProcessSecurityCommon(): error %lu, expected ERROR_INSUFFICIENT_BUFFER\n", GetLastError());
324325
Success = FALSE;
325326
goto Quit;
326327
}
@@ -331,15 +332,16 @@ CreateDefaultProcessSecurityCommon(
331332
RelativeSDSize);
332333
if (RelativeSD == NULL)
333334
{
334-
ERR("CreateDefaultProcessSecurityCommon(): Failed to allocate buffer for self relative descriptor!\n");
335+
ERR("CreateDefaultProcessSecurityCommon(): Failed to allocate relative SD\n");
335336
Success = FALSE;
336337
goto Quit;
337338
}
338339

339340
/* Convert to a self relative format now */
340341
if (!MakeSelfRelativeSD(&AbsoluteSd, RelativeSD, &RelativeSDSize))
341342
{
342-
ERR("CreateDefaultProcessSecurityCommon(): Failed to allocate relative SD, buffer too smal (error code %d)\n", GetLastError());
343+
ERR("CreateDefaultProcessSecurityCommon(): Failed to allocate relative SD (error %lu)\n", GetLastError());
344+
RtlFreeHeap(RtlGetProcessHeap(), 0, RelativeSD);
343345
Success = FALSE;
344346
goto Quit;
345347
}
@@ -362,14 +364,6 @@ CreateDefaultProcessSecurityCommon(
362364
if (Dacl != NULL)
363365
RtlFreeHeap(RtlGetProcessHeap(), 0, Dacl);
364366

365-
if (Success == FALSE)
366-
{
367-
if (RelativeSD != NULL)
368-
{
369-
RtlFreeHeap(RtlGetProcessHeap(), 0, RelativeSD);
370-
}
371-
}
372-
373367
return Success;
374368
}
375369

@@ -412,7 +406,7 @@ InsertProcessSecurityCommon(
412406
DACL_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION,
413407
ProcessSecurity))
414408
{
415-
ERR("InsertProcessSecurityCommon(): Failed to set security for process (error code %d)\n", GetLastError());
409+
ERR("InsertProcessSecurityCommon(): Failed to set security for process (error %lu)\n", GetLastError());
416410
return FALSE;
417411
}
418412

@@ -421,7 +415,7 @@ InsertProcessSecurityCommon(
421415
DACL_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION,
422416
ThreadSecurity))
423417
{
424-
ERR("InsertProcessSecurityCommon(): Failed to set security for thread (error code %d)\n", GetLastError());
418+
ERR("InsertProcessSecurityCommon(): Failed to set security for thread (error %lu)\n", GetLastError());
425419
return FALSE;
426420
}
427421

@@ -730,7 +724,7 @@ CreateProcessAsUserCommon(
730724
*/
731725
if (!CreateDefaultProcessSecurityCommon(hToken, &DefaultSd))
732726
{
733-
ERR("Failed to create common security descriptor for the token for new process!\n");
727+
ERR("Failed to create common security descriptor for the token for new process\n");
734728
Success = FALSE;
735729
goto Quit;
736730
}
@@ -865,7 +859,7 @@ CreateProcessAsUserCommon(
865859
ProcessSd,
866860
ThreadSd))
867861
{
868-
ERR("Failed to set new security information for process and thread!\n");
862+
ERR("Failed to set new security information for process and thread\n");
869863
NtClose(hTokenDup);
870864
Success = FALSE;
871865
goto Quit;
@@ -881,7 +875,7 @@ CreateProcessAsUserCommon(
881875
* ourselves as job done. The newly created process will use
882876
* the default security context at this point anyway.
883877
*/
884-
TRACE("No token supplied, the process will use default security context!\n");
878+
TRACE("No token supplied, the process will use default security context\n");
885879
Success = TRUE;
886880

887881
Quit:

0 commit comments

Comments
 (0)