https://git.reactos.org/?p=reactos.git;a=commitdiff;h=389a2da7ff94cfcefff32…
commit 389a2da7ff94cfcefff3220797f61311519d6fa0
Author: Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Thu May 19 23:43:18 2022 +0200
Commit: Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Mon May 23 19:30:34 2022 +0200
[NTOS:SE] SepCaptureAcl(): Add missing validation of the captured ACL (#4523)
- The ACL is however not validated when the function is called within
kernel mode and no capture is actually being done.
- Simplify aspects of the function (returning early when possible).
---
ntoskrnl/se/acl.c | 85 ++++++++++++++++++++++++++++-------------------------
ntoskrnl/se/token.c | 2 +-
2 files changed, 46 insertions(+), 41 deletions(-)
diff --git a/ntoskrnl/se/acl.c b/ntoskrnl/se/acl.c
index e160b34c429..d7c53d4ac17 100644
--- a/ntoskrnl/se/acl.c
+++ b/ntoskrnl/se/acl.c
@@ -329,15 +329,15 @@ SepCreateImpersonationTokenDacl(
*
* @param[in] AccessMode
* Processor level access mode. The processor mode determines how
- * are the input arguments probed.
+ * the input arguments are probed.
*
* @param[in] PoolType
* Pool type for new captured ACL for creation. The pool type determines
- * how the ACL data should reside in the pool memory.
+ * in which memory pool the ACL data should reside.
*
* @param[in] CaptureIfKernel
- * If set to TRUE and the processor access mode being KernelMode, we're
- * capturing an ACL directly in the kernel. Otherwise we're capturing
+ * If set to TRUE and the processor access mode being KernelMode, we are
+ * capturing an ACL directly in the kernel. Otherwise we are capturing
* within a kernel mode driver.
*
* @param[out] CapturedAcl
@@ -357,11 +357,19 @@ SepCaptureAcl(
_Out_ PACL *CapturedAcl)
{
PACL NewAcl;
- ULONG AclSize = 0;
- NTSTATUS Status = STATUS_SUCCESS;
+ ULONG AclSize;
PAGED_CODE();
+ /* If in kernel mode and we do not capture, just
+ * return the given ACL and don't validate it. */
+ if ((AccessMode == KernelMode) && !CaptureIfKernel)
+ {
+ *CapturedAcl = InputAcl;
+ return STATUS_SUCCESS;
+ }
+
+ /* Otherwise, capture and validate the ACL, depending on the access mode */
if (AccessMode != KernelMode)
{
_SEH2_TRY
@@ -381,59 +389,56 @@ SepCaptureAcl(
}
_SEH2_END;
+ /* Validate the minimal size an ACL can have */
+ if (AclSize < sizeof(ACL))
+ return STATUS_INVALID_ACL;
+
NewAcl = ExAllocatePoolWithTag(PoolType,
AclSize,
TAG_ACL);
- if (NewAcl != NULL)
- {
- _SEH2_TRY
- {
- RtlCopyMemory(NewAcl,
- InputAcl,
- AclSize);
+ if (!NewAcl)
+ return STATUS_INSUFFICIENT_RESOURCES;
- *CapturedAcl = NewAcl;
- }
- _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
- {
- /* Free the ACL and return the exception code */
- ExFreePoolWithTag(NewAcl, TAG_ACL);
- _SEH2_YIELD(return _SEH2_GetExceptionCode());
- }
- _SEH2_END;
+ _SEH2_TRY
+ {
+ RtlCopyMemory(NewAcl, InputAcl, AclSize);
}
- else
+ _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
{
- Status = STATUS_INSUFFICIENT_RESOURCES;
+ /* Free the ACL and return the exception code */
+ ExFreePoolWithTag(NewAcl, TAG_ACL);
+ _SEH2_YIELD(return _SEH2_GetExceptionCode());
}
- }
- else if (!CaptureIfKernel)
- {
- *CapturedAcl = InputAcl;
+ _SEH2_END;
}
else
{
AclSize = InputAcl->AclSize;
+ /* Validate the minimal size an ACL can have */
+ if (AclSize < sizeof(ACL))
+ return STATUS_INVALID_ACL;
+
NewAcl = ExAllocatePoolWithTag(PoolType,
AclSize,
TAG_ACL);
+ if (!NewAcl)
+ return STATUS_INSUFFICIENT_RESOURCES;
- if (NewAcl != NULL)
- {
- RtlCopyMemory(NewAcl,
- InputAcl,
- AclSize);
+ RtlCopyMemory(NewAcl, InputAcl, AclSize);
+ }
- *CapturedAcl = NewAcl;
- }
- else
- {
- Status = STATUS_INSUFFICIENT_RESOURCES;
- }
+ /* Validate the captured ACL */
+ if (!RtlValidAcl(NewAcl))
+ {
+ /* Free the ACL and fail */
+ ExFreePoolWithTag(NewAcl, TAG_ACL);
+ return STATUS_INVALID_ACL;
}
- return Status;
+ /* It's valid, return it */
+ *CapturedAcl = NewAcl;
+ return STATUS_SUCCESS;
}
/**
diff --git a/ntoskrnl/se/token.c b/ntoskrnl/se/token.c
index 1415e36009d..f215f4c1fe2 100644
--- a/ntoskrnl/se/token.c
+++ b/ntoskrnl/se/token.c
@@ -4518,7 +4518,7 @@ NtSetInformationToken(
{
PACL CapturedAcl;
- /* Capture and copy the dacl */
+ /* Capture, validate, and copy the DACL */
Status = SepCaptureAcl(InputAcl,
PreviousMode,
PagedPool,
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=d21895e6ef982c2927857…
commit d21895e6ef982c2927857cb18d6fb30370b5db04
Author: Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Fri May 6 20:34:52 2022 +0200
Commit: Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Mon May 23 19:30:13 2022 +0200
[WIN32K:NTUSER] Only call IntFreeSecurityBuffer() when needed, don't free NULL buffers.
Addendum to 878c2f44.
---
win32ss/user/ntuser/desktop.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/win32ss/user/ntuser/desktop.c b/win32ss/user/ntuser/desktop.c
index fcb3a8aa009..2344fea0c01 100644
--- a/win32ss/user/ntuser/desktop.c
+++ b/win32ss/user/ntuser/desktop.c
@@ -563,7 +563,7 @@ IntResolveDesktop(
LUID ProcessLuid;
USHORT StrSize;
SIZE_T MemSize;
- PSECURITY_DESCRIPTOR ServiceSD = NULL;
+ PSECURITY_DESCRIPTOR ServiceSD;
POBJECT_ATTRIBUTES ObjectAttributes = NULL;
PUNICODE_STRING ObjectName;
UNICODE_STRING WinStaName, DesktopName;
@@ -1022,16 +1022,15 @@ IntResolveDesktop(
ObjectName->Length = (USHORT)(wcslen(ObjectName->Buffer) * sizeof(WCHAR));
/*
- * Set up a security descriptor for the service.
- * A service is generally based upon a desktop
- * and a window station. The newly created window
- * station and desktop will get this security descriptor
+ * Set up a security descriptor for the new service's window station.
+ * A service has an associated window station and desktop. The newly
+ * created window station and desktop will get this security descriptor
* if such objects weren't created before.
*/
Status = IntCreateServiceSecurity(&ServiceSD);
if (!NT_SUCCESS(Status))
{
- ERR("Failed to create a security descriptor for default window station, Status 0x%08lx\n", Status);
+ ERR("Failed to create a security descriptor for service window station, Status 0x%08lx\n", Status);
goto Quit;
}
@@ -1051,6 +1050,9 @@ IntResolveDesktop(
KernelMode,
MAXIMUM_ALLOWED,
0, 0, 0, 0, 0);
+
+ IntFreeSecurityBuffer(ServiceSD);
+
if (!NT_SUCCESS(Status))
{
ASSERT(hWinSta == NULL);
@@ -1200,8 +1202,6 @@ Quit:
{
*phWinSta = hWinSta;
*phDesktop = hDesktop;
-
- IntFreeSecurityBuffer(ServiceSD);
return STATUS_SUCCESS;
}
else
@@ -1218,9 +1218,6 @@ Quit:
if (hWinSta)
ObCloseHandle(hWinSta, UserMode);
- if (ServiceSD)
- IntFreeSecurityBuffer(ServiceSD);
-
SetLastNtError(Status);
return Status;
}
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=63e59e10f30c7f0214599…
commit 63e59e10f30c7f0214599736032deb14eadb23d3
Author: Hervé Poussineau <hpoussin(a)reactos.org>
AuthorDate: Sun May 22 22:43:12 2022 +0200
Commit: Hervé Poussineau <hpoussin(a)reactos.org>
CommitDate: Sun May 22 23:42:00 2022 +0200
Revert "[WIN32SS] Only refresh graphics mode list when iModeNum = 0"
This reverts commit c243133b2c598bd004229242f36d9159a8e45b99.
Without the revert, VirtualBox auto-resize doesn't work on first attempt,
but only on next ones.
However, according to documentation, we must only initialize device and
cache information when iModeNum = 0.
CORE-18189
---
win32ss/user/ntuser/display.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/win32ss/user/ntuser/display.c b/win32ss/user/ntuser/display.c
index c3751517527..8f15a68e1bf 100644
--- a/win32ss/user/ntuser/display.c
+++ b/win32ss/user/ntuser/display.c
@@ -412,36 +412,27 @@ UserEnumDisplaySettings(
PGRAPHICS_DEVICE pGraphicsDevice;
PDEVMODEENTRY pdmentry;
ULONG i, iFoundMode;
+ PPDEVOBJ ppdev;
TRACE("Enter UserEnumDisplaySettings('%wZ', %lu)\n",
pustrDevice, iModeNum);
/* Ask GDI for the GRAPHICS_DEVICE */
pGraphicsDevice = EngpFindGraphicsDevice(pustrDevice, 0);
- if (!pGraphicsDevice)
+ ppdev = EngpGetPDEV(pustrDevice);
+
+ if (!pGraphicsDevice || !ppdev)
{
/* No device found */
ERR("No device found!\n");
return STATUS_INVALID_PARAMETER_1;
}
- if (iModeNum == 0)
- {
- PPDEVOBJ ppdev;
- ppdev = EngpGetPDEV(pustrDevice);
- if (!ppdev)
- {
- /* No device found */
- ERR("No device found!\n");
- return STATUS_INVALID_PARAMETER_1;
- }
-
- /* Let's politely ask the driver for an updated mode list,
- * just in case there's something new in there (vbox) */
+ /* let's politely ask the driver for an updated mode list,
+ just in case there's something new in there (vbox) */
- PDEVOBJ_vRefreshModeList(ppdev);
- PDEVOBJ_vRelease(ppdev);
- }
+ PDEVOBJ_vRefreshModeList(ppdev);
+ PDEVOBJ_vRelease(ppdev);
iFoundMode = 0;
for (i = 0; i < pGraphicsDevice->cDevModes; i++)