From 2fa2c610761c4b2742cf8d633e61cbe89c6bed5d Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 15 Dec 2020 17:34:10 +0100 Subject: [PATCH 1/6] add missing CharSet = CharSet.Unicode so the NetUserDel can work properly --- src/libraries/System.Diagnostics.Process/tests/Interop.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/Interop.cs b/src/libraries/System.Diagnostics.Process/tests/Interop.cs index e7388724a38999..c41f0af212d079 100644 --- a/src/libraries/System.Diagnostics.Process/tests/Interop.cs +++ b/src/libraries/System.Diagnostics.Process/tests/Interop.cs @@ -75,7 +75,7 @@ public struct SID_AND_ATTRIBUTES [DllImport("netapi32.dll", CharSet = CharSet.Unicode, SetLastError = true)] internal static extern uint NetUserAdd(string servername, uint level, ref USER_INFO_1 buf, out uint parm_err); - [DllImport("netapi32.dll")] + [DllImport("netapi32.dll", CharSet = CharSet.Unicode)] internal static extern uint NetUserDel(string servername, string username); [DllImport("advapi32.dll")] From c11cc9be3149edd4b030b9f5524c13a3e1d0ba71 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 15 Dec 2020 17:34:53 +0100 Subject: [PATCH 2/6] make the user name more unique, ensure that it's always removed properly --- .../tests/Interop.cs | 8 +++++++- .../tests/ProcessStartInfoTests.cs | 19 +++++++------------ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/Interop.cs b/src/libraries/System.Diagnostics.Process/tests/Interop.cs index c41f0af212d079..449db3f9c3c985 100644 --- a/src/libraries/System.Diagnostics.Process/tests/Interop.cs +++ b/src/libraries/System.Diagnostics.Process/tests/Interop.cs @@ -94,7 +94,7 @@ internal static void NetUserAdd(string username, string password) uint parm_err; uint result = NetUserAdd(null, 1, ref userInfo, out parm_err); - if (result != 0) // NERR_Success + if (result != ExitCodes.NERR_Success) { // most likely result == ERROR_ACCESS_DENIED // due to running without elevated privileges @@ -132,5 +132,11 @@ internal static bool ProcessTokenToSid(SafeProcessHandle token, out SecurityIden Marshal.FreeHGlobal(tu); } } + + internal static class ExitCodes + { + internal const uint NERR_Success = 0; + internal const uint NERR_UserNotFound = 2221; + } } } diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs index 88b1573536ae38..901fc46a17a853 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs @@ -355,16 +355,12 @@ public void TestWorkingDirectoryPropertyInChildProcess() public void TestUserCredentialsPropertiesOnWindows() { // [SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="Unit test dummy credentials.")] - string username = "test", password = "PassWord123!!"; - try - { - Interop.NetUserAdd(username, password); - } - catch (Exception exc) - { - Console.Error.WriteLine("TestUserCredentialsPropertiesOnWindows: NetUserAdd failed: {0}", exc.Message); - return; // test is irrelevant if we can't add a user - } + string username = "testForDotnetRuntime", password = "PassWord123!!"; + + uint removalResult = Interop.NetUserDel(null, username); + Assert.True(removalResult == Interop.ExitCodes.NERR_Success || removalResult == Interop.ExitCodes.NERR_UserNotFound); + + Interop.NetUserAdd(username, password); bool hasStarted = false; SafeProcessHandle handle = null; @@ -399,8 +395,7 @@ public void TestUserCredentialsPropertiesOnWindows() } finally { - IEnumerable collection = new uint[] { 0 /* NERR_Success */, 2221 /* NERR_UserNotFound */ }; - Assert.Contains(Interop.NetUserDel(null, username), collection); + Assert.Equal(Interop.ExitCodes.NERR_Success, Interop.NetUserDel(null, username)); if (handle != null) handle.Dispose(); From 1f55d49a40bfc168748a3bdf7773a939766ed9e9 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 15 Dec 2020 19:29:09 +0100 Subject: [PATCH 3/6] ensure the new user can access the .exe (otherwise you get Access is denied exception) --- .../tests/ProcessStartInfoTests.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs index 901fc46a17a853..c7d1fe81dec3e5 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs @@ -17,6 +17,7 @@ using Microsoft.Win32; using Microsoft.Win32.SafeHandles; using Xunit; +using System.Security.AccessControl; namespace System.Diagnostics.Tests { @@ -370,6 +371,9 @@ public void TestUserCredentialsPropertiesOnWindows() { p = CreateProcessLong(); + // ensure the new user can access the .exe (otherwise you get Access is denied exception) + SetAccessControl(username, p.StartInfo.FileName, AccessControlType.Allow); + p.StartInfo.LoadUserProfile = true; p.StartInfo.UserName = username; p.StartInfo.PasswordInClearText = password; @@ -395,6 +399,8 @@ public void TestUserCredentialsPropertiesOnWindows() } finally { + SetAccessControl(username, p.StartInfo.FileName, AccessControlType.Deny); // revoke the access + Assert.Equal(Interop.ExitCodes.NERR_Success, Interop.NetUserDel(null, username)); if (handle != null) @@ -409,6 +415,14 @@ public void TestUserCredentialsPropertiesOnWindows() } } + private static void SetAccessControl(string userName, string filePath, AccessControlType accessControlType) + { + FileInfo fileInfo = new FileInfo(filePath); + FileSecurity accessControl = fileInfo.GetAccessControl(); + accessControl.AddAccessRule(new FileSystemAccessRule(userName, FileSystemRights.ReadAndExecute, accessControlType)); + fileInfo.SetAccessControl(accessControl); + } + private static List GetNamesOfUserProfiles() { List userNames = new List(); From 48206049093de1478dfdc3498d2ff21a6d182a94 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 15 Dec 2020 19:29:57 +0100 Subject: [PATCH 4/6] remove active issue attribute --- .../System.Diagnostics.Process/tests/ProcessStartInfoTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs index c7d1fe81dec3e5..4c3287f80cacdd 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs @@ -351,7 +351,6 @@ public void TestWorkingDirectoryPropertyInChildProcess() }, workingDirectory, new RemoteInvokeOptions { StartInfo = psi }).Dispose(); } - [ActiveIssue("https://github.com/dotnet/runtime/issues/18978")] [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported)), PlatformSpecific(TestPlatforms.Windows), OuterLoop] // Uses P/Invokes, Requires admin privileges public void TestUserCredentialsPropertiesOnWindows() { From 1fac03cfe48a185e73ea4b9f8cabdfdc4bc9a993 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 15 Dec 2020 19:30:41 +0100 Subject: [PATCH 5/6] nits --- .../System.Diagnostics.Process/tests/ProcessStartInfoTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs index 4c3287f80cacdd..361f01e234ff03 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs @@ -351,11 +351,11 @@ public void TestWorkingDirectoryPropertyInChildProcess() }, workingDirectory, new RemoteInvokeOptions { StartInfo = psi }).Dispose(); } - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported)), PlatformSpecific(TestPlatforms.Windows), OuterLoop] // Uses P/Invokes, Requires admin privileges + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported)), PlatformSpecific(TestPlatforms.Windows), OuterLoop] // Uses P/Invokes, Requires admin privileges public void TestUserCredentialsPropertiesOnWindows() { // [SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="Unit test dummy credentials.")] - string username = "testForDotnetRuntime", password = "PassWord123!!"; + const string username = "testForDotnetRuntime", password = "PassWord123!!"; uint removalResult = Interop.NetUserDel(null, username); Assert.True(removalResult == Interop.ExitCodes.NERR_Success || removalResult == Interop.ExitCodes.NERR_UserNotFound); From c52e74f2b884ff9537c173be0231852c724ceac2 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 16 Dec 2020 16:52:51 +0100 Subject: [PATCH 6/6] don't run this test on Nano, as it has no "netapi32.dll" --- .../tests/ProcessStartInfoTests.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs index 361f01e234ff03..8f401237a0a821 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs @@ -23,6 +23,8 @@ namespace System.Diagnostics.Tests { public class ProcessStartInfoTests : ProcessTestBase { + private static bool IsNotWindowsNanoServerAndRemoteExecutorIsSupported => PlatformDetection.IsNotWindowsNanoServer && RemoteExecutor.IsSupported; + [Fact] public void TestEnvironmentProperty() { @@ -351,7 +353,9 @@ public void TestWorkingDirectoryPropertyInChildProcess() }, workingDirectory, new RemoteInvokeOptions { StartInfo = psi }).Dispose(); } - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported)), PlatformSpecific(TestPlatforms.Windows), OuterLoop] // Uses P/Invokes, Requires admin privileges + [ConditionalFact(nameof(IsNotWindowsNanoServerAndRemoteExecutorIsSupported))] // Nano has no "netapi32.dll" + [PlatformSpecific(TestPlatforms.Windows)] + [OuterLoop("Requires admin privileges")] public void TestUserCredentialsPropertiesOnWindows() { // [SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="Unit test dummy credentials.")]