From a530bb1b8472b860f2d3e5370b262270f55eb326 Mon Sep 17 00:00:00 2001 From: Sergei Mileshin Date: Mon, 17 Jun 2019 05:05:45 +0300 Subject: [PATCH] Set deny-all DACL instead of removing symlink on halting Deleting symbolic link on device removal only still makes it possible to open it from the real path. Setting the deny-all DACL instead is a more reliable way of preventing clients reopening the device when it is being removed. Signed-off-by: Sergei Mileshin Signed-off-by: Simon Rozman --- undocumented.h | 2 ++ wintun.c | 38 +++++++++++++++++++++++++------------- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/undocumented.h b/undocumented.h index 7f7d225..112be0d 100644 --- a/undocumented.h +++ b/undocumented.h @@ -29,3 +29,5 @@ typedef struct _SYSTEM_HANDLE_INFORMATION_EX } SYSTEM_HANDLE_INFORMATION_EX, *PSYSTEM_HANDLE_INFORMATION_EX; extern NTSTATUS ZwQuerySystemInformation(SYSTEM_INFORMATION_CLASS SystemInformationClass, PVOID SystemInformation, ULONG SystemInformationLength, ULONG *ReturnLength); + +extern POBJECT_TYPE *IoDeviceObjectType; diff --git a/wintun.c b/wintun.c index 5804ccc..64c8d9c 100644 --- a/wintun.c +++ b/wintun.c @@ -107,8 +107,6 @@ typedef struct _TUN_CTX { } PacketQueue; NDIS_HANDLE NBLPool; - - ULONG NetLuidIndex; } TUN_CTX; static UINT NdisVersion; @@ -1076,7 +1074,6 @@ static NDIS_STATUS TunInitializeEx(NDIS_HANDLE MiniportAdapterHandle, NDIS_HANDL InterlockedExchange((LONG *)&ctx->State, TUN_STATE_INITIALIZING); InterlockedExchange((LONG *)&ctx->PowerState, NdisDeviceStateD0); ctx->MiniportAdapterHandle = MiniportAdapterHandle; - ctx->NetLuidIndex = (ULONG)MiniportInitParameters->NetLuid.Info.NetLuidIndex; ctx->Statistics.Header.Type = NDIS_OBJECT_TYPE_DEFAULT; ctx->Statistics.Header.Revision = NDIS_STATISTICS_INFO_REVISION_1; @@ -1240,6 +1237,29 @@ cleanup_NdisDeregisterDeviceEx: return status; } +_IRQL_requires_max_(PASSIVE_LEVEL) +static NTSTATUS TunDeviceSetDenyAllDacl(_In_ DEVICE_OBJECT *device_object) +{ + NTSTATUS status; + SECURITY_DESCRIPTOR sd; + ACL acl; + HANDLE handle; + + if (!NT_SUCCESS(status = RtlCreateSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION))) + return status; + if (!NT_SUCCESS(status = RtlCreateAcl(&acl, sizeof(ACL), ACL_REVISION))) + return status; + if (!NT_SUCCESS(status = RtlSetDaclSecurityDescriptor(&sd, TRUE, &acl, FALSE))) + return status; + if (!NT_SUCCESS(status = ObOpenObjectByPointer(device_object, OBJ_KERNEL_HANDLE, NULL, WRITE_DAC, *IoDeviceObjectType, KernelMode, &handle))) + return status; + + status = ZwSetSecurityObject(handle, DACL_SECURITY_INFORMATION, &sd); + + ZwClose(handle); + return status; +} + _IRQL_requires_max_(PASSIVE_LEVEL) static void TunForceHandlesClosed(_Inout_ TUN_CTX *ctx) { @@ -1321,16 +1341,8 @@ static void TunHaltEx(NDIS_HANDLE MiniportAdapterContext, NDIS_HALT_ACTION HaltA for (IRP *pending_irp; (pending_irp = IoCsqRemoveNextIrp(&ctx->Device.ReadQueue.Csq, NULL)) != NULL;) TunCompleteRequest(ctx, pending_irp, STATUS_FILE_FORCED_CLOSED, IO_NO_INCREMENT); - /* It's a bit annoying to reconstruct this here, but it's better than storing it, and - * although we could just get it from ndishandle+288, that's probably a bit dirty. */ - WCHAR symbolic_name[sizeof(L"\\DosDevices\\" TUN_DEVICE_NAME) / sizeof(WCHAR) + 10/*MAXULONG as string*/] = { 0 }; - UNICODE_STRING unicode_symbolic_name; - TunInitUnicodeString(&unicode_symbolic_name, symbolic_name); - RtlUnicodeStringPrintf(&unicode_symbolic_name, L"\\DosDevices\\" TUN_DEVICE_NAME, ctx->NetLuidIndex); - /* We first get rid of the symbolic link, to prevent userspace from accidently reopening - * this while we're waiting for the refcount to drop to zero. It might still be possible to - * open it from the real path, in which case, maybe we should consider setting a deny-all DACL. */ - IoDeleteSymbolicLink(&unicode_symbolic_name); + /* Setting a deny-all DACL we prevent userspace to open the device by symlink after TunForceHandlesClosed(). */ + TunDeviceSetDenyAllDacl(ctx->Device.Object); if (InterlockedGet64(&ctx->Device.RefCount)) TunForceHandlesClosed(ctx);