diff --git a/src/Build.UnitTests/Utilities_Tests.cs b/src/Build.UnitTests/Utilities_Tests.cs index f6c62d77ccd..2219cc6bb72 100644 --- a/src/Build.UnitTests/Utilities_Tests.cs +++ b/src/Build.UnitTests/Utilities_Tests.cs @@ -64,26 +64,6 @@ public void GetTextFromTextNodeWithXmlComment7() // Should get XML; note space after x added Assert.Equal("", xmlContents); } - - [Fact] - public void HandshakesDiffer() - { - int numHandshakeOptions = (int)Math.Pow(2, Enum.GetNames(typeof(HandshakeOptions)).Length - 1); - Dictionary handshakes = new Dictionary(); - for (int i = 0; i < numHandshakeOptions; i++) - { - long nextKey = CommunicationsUtilities.GetHostHandshake((HandshakeOptions)i); - if (handshakes.TryGetValue(nextKey, out int collision)) - { - _output.WriteLine("There was a collision between {0} and {1}.", collision, i); - } - else - { - handshakes.Add(nextKey, i); - } - } - handshakes.Count.ShouldBe(numHandshakeOptions, "two or more combinations of handshake options hashed to the same value"); - } } public class UtilitiesTestReadOnlyLoad : UtilitiesTest diff --git a/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs b/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs index a30017ca426..caeb65b2bce 100644 --- a/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs +++ b/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using Microsoft.Build.Internal; using Microsoft.Build.Shared; namespace Microsoft.Build.BackEnd @@ -50,17 +51,9 @@ internal NodeEndpointOutOfProc( /// /// Returns the host handshake for this node endpoint /// - protected override long GetHostHandshake() + protected override Handshake GetHandshake() { - return NodeProviderOutOfProc.GetHostHandshake(_enableReuse, _lowPriority); - } - - /// - /// Returns the client handshake for this node endpoint - /// - protected override long GetClientHandshake() - { - return NodeProviderOutOfProc.GetClientHandshake(_enableReuse, _lowPriority); + return new Handshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: false, is64Bit: EnvironmentUtilities.Is64BitProcess, nodeReuse: _enableReuse, lowPriority: _lowPriority)); } #region Structs diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs index 63ca1cadc74..951d54fa83d 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs @@ -63,19 +63,10 @@ public int AvailableNodes /// /// Is reuse of build nodes allowed? /// Is the build running at low priority? - internal static long GetHostHandshake(bool enableNodeReuse, bool enableLowPriority) + internal static Handshake GetHandshake(bool enableNodeReuse, bool enableLowPriority) { CommunicationsUtilities.Trace("MSBUILDNODEHANDSHAKESALT=\"{0}\", msbuildDirectory=\"{1}\", enableNodeReuse={2}, enableLowPriority={3}", Traits.MSBuildNodeHandshakeSalt, BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32, enableNodeReuse, enableLowPriority); - return CommunicationsUtilities.GetHostHandshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: false, nodeReuse: enableNodeReuse, lowPriority: enableLowPriority, is64Bit: EnvironmentUtilities.Is64BitProcess)); - } - - /// - /// Magic number sent by the client to the host during the handshake. - /// Munged version of the host handshake. - /// - internal static long GetClientHandshake(bool enableNodeReuse, bool enableLowPriority) - { - return CommunicationsUtilities.GetClientHandshake(CommunicationsUtilities.GetHandshakeOptions(false, nodeReuse: enableNodeReuse, lowPriority: enableLowPriority, is64Bit: EnvironmentUtilities.Is64BitProcess)); + return new Handshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: false, nodeReuse: enableNodeReuse, lowPriority: enableLowPriority, is64Bit: EnvironmentUtilities.Is64BitProcess)); } /// @@ -100,8 +91,8 @@ public bool CreateNode(int nodeId, INodePacketFactory factory, NodeConfiguration // Make it here. CommunicationsUtilities.Trace("Starting to acquire a new or existing node to establish node ID {0}...", nodeId); - long hostHandShake = NodeProviderOutOfProc.GetHostHandshake(ComponentHost.BuildParameters.EnableNodeReuse, ComponentHost.BuildParameters.LowPriority); - NodeContext context = GetNode(null, commandLineArgs, nodeId, factory, hostHandShake, NodeProviderOutOfProc.GetClientHandshake(ComponentHost.BuildParameters.EnableNodeReuse, ComponentHost.BuildParameters.LowPriority), NodeContextTerminated); + Handshake hostHandshake = new Handshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: false, nodeReuse: ComponentHost.BuildParameters.EnableNodeReuse, lowPriority: ComponentHost.BuildParameters.LowPriority, is64Bit: EnvironmentUtilities.Is64BitProcess)); + NodeContext context = GetNode(null, commandLineArgs, nodeId, factory, hostHandshake, NodeContextTerminated); if (null != context) { diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs index 3b69a13427a..ace54e8b94c 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs @@ -125,12 +125,12 @@ protected void ShutdownAllNodes(bool nodeReuse, NodeContextTerminateDelegate ter int timeout = 30; // Attempt to connect to the process with the handshake without low priority. - Stream nodeStream = TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHostHandshake(nodeReuse, false), NodeProviderOutOfProc.GetClientHandshake(nodeReuse, false)); + Stream nodeStream = TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, false)); if (null == nodeStream) { // If we couldn't connect attempt to connect to the process with the handshake including low priority. - nodeStream = TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHostHandshake(nodeReuse, true), NodeProviderOutOfProc.GetClientHandshake(nodeReuse, true)); + nodeStream = TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, true)); } if (null != nodeStream) @@ -148,7 +148,7 @@ protected void ShutdownAllNodes(bool nodeReuse, NodeContextTerminateDelegate ter /// Finds or creates a child process which can act as a node. /// /// The pipe stream representing the node. - protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, int nodeId, INodePacketFactory factory, long hostHandshake, long clientHandshake, NodeContextTerminateDelegate terminateNode) + protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, int nodeId, INodePacketFactory factory, Handshake hostHandshake, NodeContextTerminateDelegate terminateNode) { #if DEBUG if (Execution.BuildManager.WaitForDebugger) @@ -188,7 +188,7 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in } // Get the full context of this inspection so that we can always skip this process when we have the same taskhost context - string nodeLookupKey = GetProcessesToIgnoreKey(hostHandshake, clientHandshake, nodeProcess.Id); + string nodeLookupKey = GetProcessesToIgnoreKey(hostHandshake, nodeProcess.Id); if (_processesToIgnore.Contains(nodeLookupKey)) { continue; @@ -198,7 +198,7 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in _processesToIgnore.Add(nodeLookupKey); // Attempt to connect to each process in turn. - Stream nodeStream = TryConnectToProcess(nodeProcess.Id, 0 /* poll, don't wait for connections */, hostHandshake, clientHandshake); + Stream nodeStream = TryConnectToProcess(nodeProcess.Id, 0 /* poll, don't wait for connections */, hostHandshake); if (nodeStream != null) { // Connection successful, use this node. @@ -242,14 +242,14 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in // Create the node process int msbuildProcessId = LaunchNode(msbuildLocation, commandLineArgs); - _processesToIgnore.Add(GetProcessesToIgnoreKey(hostHandshake, clientHandshake, msbuildProcessId)); + _processesToIgnore.Add(GetProcessesToIgnoreKey(hostHandshake, msbuildProcessId)); // Note, when running under IMAGEFILEEXECUTIONOPTIONS registry key to debug, the process ID // gotten back from CreateProcess is that of the debugger, which causes this to try to connect // to the debugger process. Instead, use MSBUILDDEBUGONSTART=1 // Now try to connect to it. - Stream nodeStream = TryConnectToProcess(msbuildProcessId, TimeoutForNewNodeCreation, hostHandshake, clientHandshake); + Stream nodeStream = TryConnectToProcess(msbuildProcessId, TimeoutForNewNodeCreation, hostHandshake); if (nodeStream != null) { // Connection successful, use this node. @@ -292,9 +292,9 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in /// Generate a string from task host context and the remote process to be used as key to lookup processes we have already /// attempted to connect to or are already connected to /// - private string GetProcessesToIgnoreKey(long hostHandshake, long clientHandshake, int nodeProcessId) + private string GetProcessesToIgnoreKey(Handshake hostHandshake, int nodeProcessId) { - return hostHandshake.ToString(CultureInfo.InvariantCulture) + "|" + clientHandshake.ToString(CultureInfo.InvariantCulture) + "|" + nodeProcessId.ToString(CultureInfo.InvariantCulture); + return hostHandshake.ToString() + "|" + nodeProcessId.ToString(CultureInfo.InvariantCulture); } #if !FEATURE_PIPEOPTIONS_CURRENTUSERONLY @@ -321,7 +321,7 @@ private void ValidateRemotePipeSecurityOnWindows(NamedPipeClientStream nodeStrea /// /// Attempts to connect to the specified process. /// - private Stream TryConnectToProcess(int nodeProcessId, int timeout, long hostHandshake, long clientHandshake) + private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake handshake) { // Try and connect to the process. string pipeName = NamedPipeUtil.GetPipeNameOrPath("MSBuild" + nodeProcessId); @@ -351,22 +351,23 @@ private Stream TryConnectToProcess(int nodeProcessId, int timeout, long hostHand } #endif - CommunicationsUtilities.Trace("Writing handshake to pipe {0}", pipeName); - nodeStream.WriteLongForHandshake(hostHandshake); + int[] handshakeComponents = handshake.RetrieveHandshakeComponents(); + for (int i = 0; i < handshakeComponents.Length; i++) + { + CommunicationsUtilities.Trace("Writing handshake part {0} to pipe {1}", i, pipeName); + nodeStream.WriteIntForHandshake(handshakeComponents[i]); + } + + // This indicates that we have finished all the parts of our handshake; hopefully the endpoint has as well. + nodeStream.WriteEndOfHandshakeSignal(); CommunicationsUtilities.Trace("Reading handshake from pipe {0}", pipeName); + #if NETCOREAPP2_1 || MONO - long handshake = nodeStream.ReadLongForHandshake(timeout); + nodeStream.ReadEndOfHandshakeSignal(true, timeout); #else - long handshake = nodeStream.ReadLongForHandshake(); + nodeStream.ReadEndOfHandshakeSignal(true); #endif - - if (handshake != clientHandshake) - { - CommunicationsUtilities.Trace("Handshake failed. Received {0} from client not {1}. Probably the client is a different MSBuild build.", handshake, clientHandshake); - throw new InvalidOperationException(); - } - // We got a connection. CommunicationsUtilities.Trace("Successfully connected to pipe {0}...!", pipeName); return nodeStream; diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs index 44a2de9a749..a0c30b87202 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs @@ -200,16 +200,7 @@ public void ShutdownConnectedNodes(bool enableReuse) /// public void ShutdownAllNodes() { - bool nodeReuse = ComponentHost.BuildParameters.EnableNodeReuse; - - // To avoid issues with mismatched priorities not shutting - // down all the nodes on exit, we will attempt to shutdown - // all matching notes with and without the priroity bit set. - // So precompute both versions of the handshake now. - long hostHandshake = NodeProviderOutOfProc.GetHostHandshake(nodeReuse, enableLowPriority: false); - long hostHandshakeWithLow = NodeProviderOutOfProc.GetHostHandshake(nodeReuse, enableLowPriority: true); - - ShutdownAllNodes(nodeReuse, NodeContextTerminated); + ShutdownAllNodes(ComponentHost.BuildParameters.EnableNodeReuse, NodeContextTerminated); } #endregion @@ -535,8 +526,7 @@ internal bool CreateNode(HandshakeOptions hostContext, INodePacketFactory factor commandLineArgs, (int)hostContext, this, - CommunicationsUtilities.GetHostHandshake(hostContext), - CommunicationsUtilities.GetClientHandshake(hostContext), + new Handshake(hostContext), NodeContextTerminated ); diff --git a/src/MSBuild/NodeEndpointOutOfProcTaskHost.cs b/src/MSBuild/NodeEndpointOutOfProcTaskHost.cs index 6029f16baf0..a03db6f636f 100644 --- a/src/MSBuild/NodeEndpointOutOfProcTaskHost.cs +++ b/src/MSBuild/NodeEndpointOutOfProcTaskHost.cs @@ -27,17 +27,9 @@ internal NodeEndpointOutOfProcTaskHost(string pipeName) /// /// Returns the host handshake for this node endpoint /// - protected override long GetHostHandshake() + protected override Handshake GetHandshake() { - return CommunicationsUtilities.GetHostHandshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: true)); - } - - /// - /// Returns the client handshake for this node endpoint - /// - protected override long GetClientHandshake() - { - return CommunicationsUtilities.GetClientHandshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: true)); + return new Handshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: true)); } } } diff --git a/src/Shared/CommunicationsUtilities.cs b/src/Shared/CommunicationsUtilities.cs index 2bb61d35127..30eb1a79b79 100644 --- a/src/Shared/CommunicationsUtilities.cs +++ b/src/Shared/CommunicationsUtilities.cs @@ -51,7 +51,59 @@ internal enum HandshakeOptions /// /// Building with BelowNormal priority /// - LowPriority = 16 + LowPriority = 16, + + /// + /// Building with administrator privileges + /// + Administrator = 32 + } + + internal readonly struct Handshake + { + readonly int options; + readonly int salt; + readonly int fileVersionMajor; + readonly int fileVersionMinor; + readonly int fileVersionBuild; + readonly int fileVersionPrivate; + readonly int sessionId; + + internal Handshake(HandshakeOptions nodeType) + { + // We currently use 6 bits of this 32-bit integer. Very old builds will instantly reject any handshake that does not start with F5 or 06; slightly old builds always lead with 00. + // This indicates in the first byte that we are a modern build. + options = (int)nodeType | 0x01000000; + string handshakeSalt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"); + string toolsDirectory = (nodeType & HandshakeOptions.X64) == HandshakeOptions.X64 ? BuildEnvironmentHelper.Instance.MSBuildToolsDirectory64 : BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32; + salt = CommunicationsUtilities.GetHandshakeHashCode(handshakeSalt + toolsDirectory); + Version fileVersion = new Version(FileVersionInfo.GetVersionInfo(Assembly.GetExecutingAssembly().Location).FileVersion); + fileVersionMajor = fileVersion.Major; + fileVersionMinor = fileVersion.Minor; + fileVersionBuild = fileVersion.Build; + fileVersionPrivate = fileVersion.Revision; + sessionId = Process.GetCurrentProcess().SessionId; + } + + // This is used as a key, so it does not need to be human readable. + public override string ToString() + { + return String.Format("{0} {1} {2} {3} {4} {5} {6}", options, salt, fileVersionMajor, fileVersionMinor, fileVersionBuild, fileVersionPrivate, sessionId); + } + + internal int[] RetrieveHandshakeComponents() + { + return new int[] + { + CommunicationsUtilities.AvoidEndOfHandshakeSignal(options), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(salt), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMajor), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMinor), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionBuild), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionPrivate), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(sessionId) + }; + } } /// @@ -60,19 +112,14 @@ internal enum HandshakeOptions static internal class CommunicationsUtilities { /// - /// The timeout to connect to a node. - /// - private const int DefaultNodeConnectionTimeout = 900 * 1000; // 15 minutes; enough time that a dev will typically do another build in this time - - /// - /// Flag if we have already calculated the FileVersion hashcode + /// Indicates to the NodeEndpoint that all the various parts of the Handshake have been sent. /// - private static bool s_fileVersionChecked; + private const int EndOfHandshakeSignal = -0x2a2a2a2a; /// - /// A hashcode calculated from the fileversion + /// The timeout to connect to a node. /// - private static int s_fileVersionHash; + private const int DefaultNodeConnectionTimeout = 900 * 1000; // 15 minutes; enough time that a dev will typically do another build in this time /// /// Whether to trace communications @@ -102,48 +149,6 @@ static internal int NodeConnectionTimeout get { return GetIntegerVariableOrDefault("MSBUILDNODECONNECTIONTIMEOUT", DefaultNodeConnectionTimeout); } } - /// - /// Looks up the file version and caches the hashcode - /// This file version hashcode is used in calculating the handshake - /// - private static int FileVersionHash - { - get - { - if (!s_fileVersionChecked) - { - // We only hash in any complus_installroot value, not a file version. - // This is because in general msbuildtaskhost.exe does not load any assembly that - // the parent process loads, so they can't compare the version of a particular assembly. - // They can't compare their own versions, because if one of them is serviced, they - // won't match any more. The only known incompatibility is between a razzle and non-razzle - // parent and child. COMPLUS_Version can (and typically will) differ legitimately between - // them, so just check COMPLUS_InstallRoot. - string complusInstallRoot = Environment.GetEnvironmentVariable("COMPLUS_INSTALLROOT"); - - // This is easier in .NET 4+: - // var fileIdentity = typeof(CommunicationsUtilities).GetTypeInfo().Assembly.GetCustomAttribute().InformationalVersion; - // but we need to be 3.5 compatible here to work in MSBuildTaskHost - string fileIdentity = null; - foreach (var attribute in typeof(CommunicationsUtilities).GetTypeInfo().Assembly.GetCustomAttributes(false)) - { - if (attribute is AssemblyInformationalVersionAttribute informationalVersionAttribute) - { - fileIdentity = informationalVersionAttribute.InformationalVersion; - break; - } - } - - ErrorUtilities.VerifyThrow(fileIdentity != null, "Did not successfully retrieve InformationalVersion."); - - s_fileVersionHash = GetHandshakeHashCode(complusInstallRoot ?? fileIdentity); - s_fileVersionChecked = true; - } - - return s_fileVersionHash; - } - } - /// /// Get environment block /// @@ -292,69 +297,17 @@ internal static void SetEnvironment(IDictionary newEnvironment) } /// - /// Given a base handshake, generates the real handshake based on e.g. elevation level. - /// - private static long GenerateHostHandshakeFromBase(long baseHandshake) - { -#if FEATURE_SECURITY_PRINCIPAL_WINDOWS - // If we are running in elevated privs, we will only accept a handshake from an elevated process as well. - WindowsPrincipal principal = new WindowsPrincipal(WindowsIdentity.GetCurrent()); - - // Both the client and the host will calculate this separately, and the idea is that if they come out the same - // then we can be sufficiently confident that the other side has the same elevation level as us. This is complementary - // to the username check which is also done on connection. - if (principal.IsInRole(WindowsBuiltInRole.Administrator)) - { - unchecked - { - baseHandshake = baseHandshake ^ 0x5c5c5c5c5c5c5c5c + Process.GetCurrentProcess().SessionId; - } - } -#endif - - // Mask out the first byte. Modern builds expect the first byte to be zero to indicate that they are modern - // and should be treated as such. Older builds used a non-zero initial byte. See here: - // https://github.com/microsoft/msbuild/blob/584ca5f11b28971f5651b4b8de5f173ad1cb2786/src/Shared/NodeEndpointOutOfProcBase.cs#L403. - return baseHandshake & 0x00FFFFFFFFFFFFFF; - } - - /// - /// Magic number sent by the host to the client during the handshake. - /// Derived from the binary timestamp to avoid mixing binary versions. + /// Indicate to the client that all elements of the Handshake have been sent. /// - internal static long GetHostHandshake(HandshakeOptions nodeType) + internal static void WriteEndOfHandshakeSignal(this PipeStream stream) { - string salt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"); - string toolsDirectory = (nodeType & HandshakeOptions.X64) == HandshakeOptions.X64 ? BuildEnvironmentHelper.Instance.MSBuildToolsDirectory64 : BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32; - int nodeHandshakeSalt = GetHandshakeHashCode(salt + toolsDirectory); - - Trace("MSBUILDNODEHANDSHAKESALT=\"{0}\", msbuildDirectory=\"{1}\", nodeType={2}, FileVersionHash={3}", salt, toolsDirectory, nodeType, FileVersionHash); - - // FileVersionHash (32 bits) is shifted 8 bits to avoid session ID collision - // HandshakeOptions (5 bits) is shifted just after the FileVersionHash - // remaining bits of nodeHandshakeSalt (32 bits truncated to 11) are shifted next - // nodeHandshakeSalt | HandshakeOptions | fileVersionHash | SessionID - // 0000 0000 0000 0000 000 0 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 - unchecked - { - ulong baseHandshake = ((ulong)(uint)nodeHandshakeSalt << 45) | ((ulong)(uint)nodeType << 40) | ((ulong)(uint)FileVersionHash << 8); - return GenerateHostHandshakeFromBase((long)baseHandshake); - } - } - - /// - /// Magic number sent by the client to the host during the handshake. - /// Munged version of the host handshake. - /// - internal static long GetClientHandshake(HandshakeOptions hostContext) - { - return ~GetHostHandshake(hostContext); + stream.WriteIntForHandshake(EndOfHandshakeSignal); } /// /// Extension method to write a series of bytes to a stream /// - internal static void WriteLongForHandshake(this PipeStream stream, long value) + internal static void WriteIntForHandshake(this PipeStream stream, int value) { byte[] bytes = BitConverter.GetBytes(value); @@ -365,39 +318,49 @@ internal static void WriteLongForHandshake(this PipeStream stream, long value) Array.Reverse(bytes); } - ErrorUtilities.VerifyThrow(bytes.Length == 8, "Long should be 8 bytes"); + ErrorUtilities.VerifyThrow(bytes.Length == 4, "Int should be 4 bytes"); stream.Write(bytes, 0, bytes.Length); } - /// - /// Extension method to read a series of bytes from a stream - /// - internal static long ReadLongForHandshake(this PipeStream stream + internal static void ReadEndOfHandshakeSignal(this PipeStream stream, bool isProvider #if NETCOREAPP2_1 || MONO - , int handshakeReadTimeout + , int timeout #endif ) { - return stream.ReadLongForHandshake((byte[])null, 0 + // Accept only the first byte of the EndOfHandshakeSignal + int valueRead = stream.ReadIntForHandshake(0x00 #if NETCOREAPP2_1 || MONO - , handshakeReadTimeout + , timeout #endif ); + + if (valueRead != EndOfHandshakeSignal) + { + if (isProvider) + { + CommunicationsUtilities.Trace("Handshake failed on part {0}. Probably the client is a different MSBuild build.", valueRead); + } + else + { + CommunicationsUtilities.Trace("Expected end of handshake signal but received {0}. Probably the host is a different MSBuild build.", valueRead); + } + throw new InvalidOperationException(); + } } /// /// Extension method to read a series of bytes from a stream. /// If specified, leading byte matches one in the supplied array if any, returns rejection byte and throws IOException. /// - internal static long ReadLongForHandshake(this PipeStream stream, byte[] leadingBytesToReject, - byte rejectionByteToReturn + internal static int ReadIntForHandshake(this PipeStream stream, int byteToAccept #if NETCOREAPP2_1 || MONO , int timeout #endif ) { - byte[] bytes = new byte[8]; + byte[] bytes = new byte[4]; #if NETCOREAPP2_1 || MONO if (!NativeMethodsShared.IsWindows) @@ -437,24 +400,18 @@ byte rejectionByteToReturn throw new IOException(String.Format(CultureInfo.InvariantCulture, "Unexpected end of stream while reading for handshake")); } - if (i == 0 && leadingBytesToReject != null) - { - foreach (byte reject in leadingBytesToReject) - { - if (read == reject) - { - stream.WriteByte(rejectionByteToReturn); // disconnect the host + bytes[i] = Convert.ToByte(read); - throw new IOException(String.Format(CultureInfo.InvariantCulture, "Client: rejected old host. Received byte {0} but this matched a byte to reject.", bytes[i])); // disconnect and quit - } - } + if (i == 0 && byteToAccept != 0x00 && byteToAccept != bytes[0]) + { + stream.WriteIntForHandshake(0x0F0F0F0F); + stream.WriteIntForHandshake(0x0F0F0F0F); + throw new InvalidOperationException(String.Format(CultureInfo.InvariantCulture, "Client: rejected old host. Received byte {0} instead of {1}.", bytes[0], byteToAccept)); } - - bytes[i] = Convert.ToByte(read); } } - long result; + int result; try { @@ -465,7 +422,7 @@ byte rejectionByteToReturn Array.Reverse(bytes); } - result = BitConverter.ToInt64(bytes, 0 /* start index */); + result = BitConverter.ToInt32(bytes, 0 /* start index */); } catch (ArgumentException ex) { @@ -533,6 +490,16 @@ internal static HandshakeOptions GetHandshakeOptions(bool taskHost, bool is64Bit { context |= HandshakeOptions.LowPriority; } +#if FEATURE_SECURITY_PRINCIPAL_WINDOWS + // If we are running in elevated privs, we will only accept a handshake from an elevated process as well. + // Both the client and the host will calculate this separately, and the idea is that if they come out the same + // then we can be sufficiently confident that the other side has the same elevation level as us. This is complementary + // to the username check which is also done on connection. + if (new WindowsPrincipal(WindowsIdentity.GetCurrent()).IsInRole(WindowsBuiltInRole.Administrator)) + { + context |= HandshakeOptions.Administrator; + } +#endif return context; } @@ -646,5 +613,10 @@ internal static int GetHandshakeHashCode(string fileVersion) } } } + + internal static int AvoidEndOfHandshakeSignal(int x) + { + return x == EndOfHandshakeSignal ? ~x : x; + } } } diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index 05b8ae7a4a5..e416ea03574 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -239,12 +239,7 @@ internal void InternalConstruct(string pipeName) /// /// Returns the host handshake for this node endpoint /// - protected abstract long GetHostHandshake(); - - /// - /// Returns the client handshake for this node endpoint - /// - protected abstract long GetClientHandshake(); + protected abstract Handshake GetHandshake(); /// /// Updates the current link status if it has changed and notifies any registered delegates. @@ -329,8 +324,6 @@ private void InitializeAsyncPacketThread() private void PacketPumpProc() { NamedPipeServerStream localPipeServer = _pipeServer; - PipeStream localWritePipe = _pipeServer; - PipeStream localReadPipe = _pipeServer; AutoResetEvent localPacketAvailable = _packetAvailable; AutoResetEvent localTerminatePacketPump = _terminatePacketPump; @@ -340,6 +333,7 @@ private void PacketPumpProc() bool gotValidConnection = false; while (!gotValidConnection) { + gotValidConnection = true; DateTime restartWaitTime = DateTime.UtcNow; // We only wait to wait the difference between now and the last original start time, in case we have multiple hosts attempting @@ -352,14 +346,11 @@ private void PacketPumpProc() // Wait for a connection #if FEATURE_APM IAsyncResult resultForConnection = localPipeServer.BeginWaitForConnection(null, null); -#else - Task connectionTask = localPipeServer.WaitForConnectionAsync(); -#endif CommunicationsUtilities.Trace("Waiting for connection {0} ms...", waitTimeRemaining); - -#if FEATURE_APM bool connected = resultForConnection.AsyncWaitHandle.WaitOne(waitTimeRemaining, false); #else + Task connectionTask = localPipeServer.WaitForConnectionAsync(); + CommunicationsUtilities.Trace("Waiting for connection {0} ms...", waitTimeRemaining); bool connected = connectionTask.Wait(waitTimeRemaining); #endif if (!connected) @@ -374,57 +365,57 @@ private void PacketPumpProc() localPipeServer.EndWaitForConnection(resultForConnection); #endif - // The handshake protocol is a simple long exchange. The host sends us a long, and we - // respond with another long. Once the handshake is complete, both sides can be assured the - // other is ready to accept data. - // To avoid mixing client and server builds, the long is the MSBuild binary timestamp. - - // Compatibility issue here. - // Previous builds of MSBuild 4.0 would exchange just a byte. - // Host would send either 0x5F or 0x60 depending on whether it was the toolset or not respectively. - // Client would return either 0xF5 or 0x06 respectively. - // Therefore an old host on a machine with new clients running will hang, - // sending a byte and waiting for a byte until it eventually times out; - // because the new client will want 7 more bytes before it returns anything. - // The other way around is not a problem, because the old client would immediately return the (wrong) - // byte on receiving the first byte of the long sent by the new host, and the new host would disconnect. - // To avoid the hang, special case here: - // Make sure our handshakes always start with 00. - // If we received ONLY one byte AND it's 0x5F or 0x60, return 0xFF (it doesn't matter what as long as - // it will cause the host to reject us; new hosts expect 00 and old hosts expect F5 or 06). + // The handshake protocol is a series of int exchanges. The host sends us a each component, and we + // verify it. Afterwards, the host sends an "End of Handshake" signal, to which we respond in kind. + // Once the handshake is complete, both sides can be assured the other is ready to accept data. + Handshake handshake = GetHandshake(); try { - long handshake = localReadPipe.ReadLongForHandshake(/* reject these leads */ new byte[] { 0x5F, 0x60 }, 0xFF /* this will disconnect the host; it expects leading 00 or F5 or 06 */ + int[] handshakeComponents = handshake.RetrieveHandshakeComponents(); + for (int i = 0; i < handshakeComponents.Length; i++) + { + int handshakePart = _pipeServer.ReadIntForHandshake(i == 0 ? 0x01 : 0x00 /* this will disconnect a < 16.8 host; it expects leading 00 or F5 or 06. 0x00 is a wildcard */ #if NETCOREAPP2_1 || MONO , ClientConnectTimeout /* wait a long time for the handshake from this side */ #endif ); -#if FEATURE_SECURITY_PERMISSIONS - WindowsIdentity currentIdentity = WindowsIdentity.GetCurrent(); -#endif + if (handshakePart != handshakeComponents[i]) + { + CommunicationsUtilities.Trace("Handshake failed. Received {0} from host not {1}. Probably the host is a different MSBuild build.", handshakePart, handshakeComponents[i]); + _pipeServer.WriteIntForHandshake(i + 1); + gotValidConnection = false; + break; + } + } - if (handshake != GetHostHandshake()) + if (gotValidConnection) { - CommunicationsUtilities.Trace("Handshake failed. Received {0} from host not {1}. Probably the host is a different MSBuild build.", handshake, GetHostHandshake()); - localPipeServer.Disconnect(); - continue; - } + // To ensure that our handshake and theirs have the same number of bytes, receive and send a magic number indicating EOS. +#if NETCOREAPP2_1 || MONO + _pipeServer.ReadEndOfHandshakeSignal(false, ClientConnectTimeout); /* wait a long time for the handshake from this side */ +#else + _pipeServer.ReadEndOfHandshakeSignal(false); +#endif + CommunicationsUtilities.Trace("Successfully connected to parent."); + _pipeServer.WriteEndOfHandshakeSignal(); #if FEATURE_SECURITY_PERMISSIONS - // We will only talk to a host that was started by the same user as us. Even though the pipe access is set to only allow this user, we want to ensure they - // haven't attempted to change those permissions out from under us. This ensures that the only way they can truly gain access is to be impersonating the - // user we were started by. - WindowsIdentity clientIdentity = null; - localPipeServer.RunAsClient(delegate () { clientIdentity = WindowsIdentity.GetCurrent(true); }); - - if (clientIdentity == null || !String.Equals(clientIdentity.Name, currentIdentity.Name, StringComparison.OrdinalIgnoreCase)) - { - CommunicationsUtilities.Trace("Handshake failed. Host user is {0} but we were created by {1}.", (clientIdentity == null) ? "" : clientIdentity.Name, currentIdentity.Name); - localPipeServer.Disconnect(); - continue; - } + // We will only talk to a host that was started by the same user as us. Even though the pipe access is set to only allow this user, we want to ensure they + // haven't attempted to change those permissions out from under us. This ensures that the only way they can truly gain access is to be impersonating the + // user we were started by. + WindowsIdentity currentIdentity = WindowsIdentity.GetCurrent(); + WindowsIdentity clientIdentity = null; + localPipeServer.RunAsClient(delegate () { clientIdentity = WindowsIdentity.GetCurrent(true); }); + + if (clientIdentity == null || !String.Equals(clientIdentity.Name, currentIdentity.Name, StringComparison.OrdinalIgnoreCase)) + { + CommunicationsUtilities.Trace("Handshake failed. Host user is {0} but we were created by {1}.", (clientIdentity == null) ? "" : clientIdentity.Name, currentIdentity.Name); + gotValidConnection = false; + continue; + } #endif + } } catch (IOException e) { @@ -432,19 +423,25 @@ private void PacketPumpProc() // 1. The host (OOP main node) connects to us, it immediately checks for user privileges // and if they don't match it disconnects immediately leaving us still trying to read the blank handshake // 2. The host is too old sending us bits we automatically reject in the handshake + // 3. We expected to read the EndOfHandshake signal, but we received something else CommunicationsUtilities.Trace("Client connection failed but we will wait for another connection. Exception: {0}", e.Message); + + gotValidConnection = false; + } + catch (InvalidOperationException) + { + gotValidConnection = false; + } + + if (!gotValidConnection) + { if (localPipeServer.IsConnected) { localPipeServer.Disconnect(); } - continue; } - gotValidConnection = true; - - CommunicationsUtilities.Trace("Writing handshake to parent"); - localWritePipe.WriteLongForHandshake(GetClientHandshake()); ChangeLinkStatus(LinkStatus.Active); } catch (Exception e) @@ -467,8 +464,8 @@ private void PacketPumpProc() } RunReadLoop( - new BufferedReadStream(localReadPipe), - localWritePipe, + new BufferedReadStream(_pipeServer), + _pipeServer, localPacketQueue, localPacketAvailable, localTerminatePacketPump); CommunicationsUtilities.Trace("Ending read loop");