[msbuild] Don't let binding sidecar manifest metadata redirect native reference output paths

dotnet/targets/Xamarin.Shared.Sdk.targets

@@ -2098,6 +2098,7 @@
 			Condition="'$(IsMacEnabled)' == 'true'"
 			SessionId="$(BuildSessionId)"
 			DynamicLibrary="@(_DynamicLibraryToReidentify)"
+			IntermediateNativeLibraryDir="$(_IntermediateNativeLibraryDir)"
 			SdkDevPath="$(_SdkDevPath)"
 		/>
 	</Target>

msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx

@@ -1666,4 +1666,28 @@
         <comment>Shown when an MSBuild task writes to the console instead of using MSBuild logging.
 {0} - The stack trace of the offending call.</comment>
     </data>
+    <data name="W7179" xml:space="preserve">
+        <value>Ignoring the metadata '{0}' for the native reference '{1}' in the binding resource package '{2}': this metadata is computed by the build and can't be set in a binding resource package manifest.</value>
+        <comment>Shown when a binding resource package's manifest contains build-controlled path/layout/identity metadata that isn't allowed to be set from a manifest.
+{0} - The name of the metadata (e.g. "RelativePath").
+{1} - The name of the native reference (e.g. "libfoo.dylib").
+{2} - The path to the binding resource package.</comment>
+    </data>
+    <data name="E7180" xml:space="preserve">
+        <value>The native reference '{0}' in the binding resource package '{1}' is invalid: the name must be a relative path without '..' segments.</value>
+        <comment>Shown when a binding resource package's manifest contains a native reference whose name is an absolute path or contains '..' traversal segments.
+{0} - The invalid native reference name.
+{1} - The path to the binding resource package.</comment>
+    </data>
+    <data name="E7181" xml:space="preserve">
+        <value>The native library can't be reidentified to '{0}' because that path is outside the intended output directory '{1}'.</value>
+        <comment>Shown when the computed reidentified path for a native library would write outside the intended intermediate output directory.
+{0} - The computed (out-of-bounds) reidentified path.
+{1} - The intended output directory.</comment>
+    </data>
+    <data name="W7182" xml:space="preserve">
+        <value>Ignoring a native reference with no name in the binding resource package '{0}'.</value>
+        <comment>Shown when a binding resource package's manifest contains a native reference whose 'Name' attribute is missing or empty.
+{0} - The path to the binding resource package.</comment>
+    </data>
 </root>

msbuild/Xamarin.MacDev.Tasks/Tasks/InstallNameTool.cs

@@ -7,6 +7,7 @@
 using Microsoft.Build.Framework;
 
 using Xamarin.Messaging.Build.Client;
+using Xamarin.Utils;
 
 #nullable enable
 
@@ -15,6 +16,12 @@ public class InstallNameTool : XamarinTask, ITaskCallback {
 		[Required]
 		public ITaskItem [] DynamicLibrary { get; set; } = [];
 
+		// The intended output directory for reidentified native libraries. Used to make sure we never
+		// write outside this directory, even if the reidentified path was influenced by metadata that
+		// originates from a (passive) binding resource package manifest.
+		[Required]
+		public string IntermediateNativeLibraryDir { get; set; } = "";
+
 		// This isn't consumed from the targets files, but it's needed for VSX to create corresponding
 		// files on Windows.
 		[Output]
@@ -34,6 +41,17 @@ public override bool Execute ()
 				// Make sure we use the correct path separator, these are relative paths, so it doesn't look
 				// like MSBuild does the conversion automatically.
 				var target = input.GetMetadata ("ReidentifiedPath").Replace ('\\', Path.DirectorySeparatorChar);
+
+				// Defense-in-depth: the 'ReidentifiedPath' can be influenced by metadata that originates
+				// from a (passive) binding resource package manifest. Make sure we
+				// never create directories or write files outside the intended intermediate output
+				// directory, even if the path contains traversal segments, is absolute, or uses symlinks.
+				if (!PathUtils.IsPathContained (IntermediateNativeLibraryDir, target)) {
+					Log.LogError (MSBStrings.E7181 /* The native library can't be reidentified to '{0}' because that path is outside the intended output directory '{1}'. */, target, IntermediateNativeLibraryDir);
+					processes [i] = System.Threading.Tasks.Task.CompletedTask;
+					continue;
+				}
+
 				var temporaryTarget = target + ".tmp";
 
 				// install_name_tool modifies the file in-place, so copy it first to a temporary file first.
@@ -48,11 +66,17 @@ public override bool Execute ()
 				arguments.Add (temporaryTarget);
 
 				processes [i] = ExecuteAsync ("xcrun", arguments).ContinueWith ((v) => {
-					if (v.IsFaulted)
+					if (v.IsFaulted) {
+						// install_name_tool faulted; don't leave the temporary copy behind.
+						File.Delete (temporaryTarget);
 						throw v.Exception;
-					if (v.Status == TaskStatus.RanToCompletion) {
+					}
+					if (v.Status == TaskStatus.RanToCompletion && v.Result.ExitCode == 0) {
 						File.Delete (target);
 						File.Move (temporaryTarget, target);
+					} else {
+						// install_name_tool failed; don't leave the temporary copy behind.
+						File.Delete (temporaryTarget);
 					}
 				});
 
@@ -61,6 +85,9 @@ public override bool Execute ()
 
 			Task.WaitAll (processes);
 
+			// Drop any items we skipped because their reidentified path wasn't contained.
+			ReidentifiedDynamicLibrary = ReidentifiedDynamicLibrary.Where (item => item is not null).ToArray ();
+
 			return !Log.HasLoggedErrors;
 		}
 

msbuild/Xamarin.MacDev.Tasks/Tasks/ResolveNativeReferences.cs

@@ -299,6 +299,42 @@ void SetMetadataNativeLibrary (ITaskItem item, string nativeLibraryPath)
 			item.SetMetadata ("RelativePath", Path.Combine (FrameworksDirectory, Path.GetFileName (Path.GetDirectoryName (item.ItemSpec)!)));
 		}
 
+		// Metadata we must NOT copy from a binding resource package's 'manifest' file. A binding resource
+		// package is passive data that may come from a restored package, so its manifest must only
+		// describe native-reference content - it must not be able to inject the build-controlled
+		// path/layout/identity metadata below, which is used to compute where a native library is laid
+		// out, reidentified or published (and could otherwise redirect output outside the intended output
+		// directory). Everything else - including binding-defined metadata such as 'NoDSymUtil' or
+		// 'NoSymbolStrip' - is legitimate native-reference content and is copied as-is.
+		static readonly HashSet<string> blockedManifestMetadata = new HashSet<string> (StringComparer.OrdinalIgnoreCase) {
+			"RelativePath",
+			"ReidentifiedPath",
+			"ComputedRelativePath",
+			"DynamicLibraryId",
+			"PublishFolderType",
+			"TargetDirectory",
+			"SourceDirectory",
+		};
+
+		// Returns true if 'path' is not a safe relative subpath: i.e. it's rooted/absolute, drive-qualified,
+		// or contains a '..' traversal segment. Used to validate untrusted path-shaped values that come
+		// from a binding resource package manifest before they're combined with a directory.
+		static bool IsUnsafeRelativePath (string path)
+		{
+			if (string.IsNullOrEmpty (path))
+				return false;
+			// Drive-qualified (e.g. "C:..." on Windows) or alternate-data-stream separators.
+			if (path.IndexOf (':') >= 0)
+				return true;
+			if (Path.IsPathRooted (path))
+				return true;
+			foreach (var segment in path.Split ('/', '\\')) {
+				if (segment == "..")
+					return true;
+			}
+			return false;
+		}
+
 		void ProcessSidecar (ITaskItem r, string resources, List<ITaskItem> native_frameworks, List<string> createdFiles, CancellationToken? cancellationToken)
 		{
 			if (!TryGetSidecarManifest (Log, resources, out var manifestContents))
@@ -310,6 +346,19 @@ void ProcessSidecar (ITaskItem r, string resources, List<ITaskItem> native_frame
 			foreach (XmlNode referenceNode in document.GetElementsByTagName ("NativeReference")) {
 				ITaskItem t = new TaskItem (r);
 				var name = referenceNode.Attributes? ["Name"]?.Value.Trim ('\\', '/') ?? string.Empty;
+				// A native reference without a (usable) name can't be processed; skip it instead of falling
+				// through and accidentally treating the managed reference itself as a native reference.
+				if (string.IsNullOrEmpty (name)) {
+					Log.LogWarning (MSBStrings.W7182 /* Ignoring a native reference with no name in the binding resource package '{0}'. */, r.ItemSpec);
+					continue;
+				}
+				// The 'Name' is combined with the binding resource package's directory to locate (and
+				// potentially extract) the native reference, so make sure it can't be used to escape the
+				// binding resource package using an absolute path or '..' traversal segments.
+				if (IsUnsafeRelativePath (name)) {
+					Log.LogError (MSBStrings.E7180 /* The native reference '{0}' in the binding resource package '{1}' is invalid: the name must be a relative path without '..' segments. */, name, r.ItemSpec);
+					continue;
+				}
 				if (name.EndsWith (".xcframework", StringComparison.Ordinal) || name.EndsWith (".xcframework.zip", StringComparison.Ordinal)) {
 					if (!TryResolveXCFramework (this, TargetFrameworkMoniker, SdkIsSimulator, Architectures, resources, name, GetIntermediateDecompressionDir (resources), createdFiles, cancellationToken, out var nativeLibraryPath))
 						continue;
@@ -359,9 +408,24 @@ void ProcessSidecar (ITaskItem r, string resources, List<ITaskItem> native_frame
 				t.SetMetadata ("LinkWithSwiftSystemLibraries", "False");
 				t.SetMetadata ("SmartLink", "True");
 
-				// values from manifest, overriding defaults if provided
-				foreach (XmlNode attribute in referenceNode.ChildNodes)
-					t.SetMetadata (attribute.Name, attribute.InnerText);
+				// Values from the manifest, overriding the defaults above if provided. Skip the
+				// build-controlled path/layout/identity metadata: the manifest is passive data and must not
+				// be able to inject metadata that could redirect output outside the intended directory.
+				foreach (XmlNode attribute in referenceNode.ChildNodes) {
+					if (attribute.NodeType != XmlNodeType.Element)
+						continue;
+					if (blockedManifestMetadata.Contains (attribute.Name)) {
+						Log.LogWarning (MSBStrings.W7179 /* Ignoring the metadata '{0}' for the native reference '{1}' in the binding resource package '{2}': this metadata is computed by the build and can't be set in a binding resource package manifest. */, attribute.Name, name, r.ItemSpec);
+						continue;
+					}
+					try {
+						t.SetMetadata (attribute.Name, attribute.InnerText);
+					} catch (ArgumentException) {
+						// Reserved MSBuild metadata names (e.g. 'FullPath') can't be set and shouldn't appear
+						// in a manifest; ignore (and warn about) them rather than failing the build.
+						Log.LogWarning (MSBStrings.W7179 /* Ignoring the metadata '{0}' for the native reference '{1}' in the binding resource package '{2}': this metadata is computed by the build and can't be set in a binding resource package manifest. */, attribute.Name, name, r.ItemSpec);
+					}
+				}
 
 				native_frameworks.Add (t);
 			}

tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/InstallNameToolTaskTest.cs

@@ -0,0 +1,85 @@
+using System.IO;
+
+using Microsoft.Build.Framework;
+using Microsoft.Build.Utilities;
+
+using NUnit.Framework;
+
+#nullable enable
+
+namespace Xamarin.MacDev.Tasks {
+
+	// Tests for the InstallNameTool task: a reidentified native library must never be written outside the
+	// intended intermediate output directory, even if the 'ReidentifiedPath' was influenced by metadata
+	// that originates from a (passive) binding resource package manifest.
+	[TestFixture]
+	public class InstallNameToolTaskTest : TestBase {
+
+		[TestCase ("traversal")]
+		[TestCase ("absolute")]
+		[TestCase ("mixedseparators")]
+		public void RefusesToWriteOutOfRoot (string kind)
+		{
+			var tmp = Cache.CreateTemporaryDirectory ();
+			var root = Path.Combine (tmp, "obj", "nativelibraries");
+			Directory.CreateDirectory (root);
+			var src = Path.Combine (tmp, "libpayload.dylib");
+			File.WriteAllText (src, "fake dylib");
+			var escapeTarget = Path.Combine (tmp, "ESCAPED", "libpayload.dylib");
+
+			string reidentifiedPath;
+			switch (kind) {
+			case "traversal":
+				reidentifiedPath = Path.Combine (root, "..", "..", "ESCAPED", "libpayload.dylib");
+				break;
+			case "absolute":
+				reidentifiedPath = escapeTarget;
+				break;
+			case "mixedseparators":
+				reidentifiedPath = root + @"\..\..\ESCAPED\libpayload.dylib";
+				break;
+			default:
+				throw new System.NotImplementedException (kind);
+			}
+
+			var task = CreateTask<InstallNameTool> ();
+			task.IntermediateNativeLibraryDir = root;
+			var item = new TaskItem (src);
+			item.SetMetadata ("ReidentifiedPath", reidentifiedPath);
+			item.SetMetadata ("DynamicLibraryId", "@executable_path/libpayload.dylib");
+			task.DynamicLibrary = new ITaskItem [] { item };
+
+			ExecuteTask (task, 1);
+
+			// Nothing was created outside the intended directory (not even the temporary file).
+			Assert.That (Path.Combine (tmp, "ESCAPED"), Does.Not.Exist, "no escaped directory");
+			Assert.That (escapeTarget, Does.Not.Exist, "no escaped file");
+			Assert.That (escapeTarget + ".tmp", Does.Not.Exist, "no escaped temp file");
+		}
+
+		[Test]
+		public void CleansUpTemporaryFileWhenInstallNameToolFails ()
+		{
+			// When install_name_tool fails (here: the input isn't a valid Mach-O file) the temporary copy
+			// must not be left behind.
+			var tmp = Cache.CreateTemporaryDirectory ();
+			var root = Path.Combine (tmp, "obj", "nativelibraries");
+			Directory.CreateDirectory (root);
+			var src = Path.Combine (tmp, "libpayload.dylib");
+			File.WriteAllText (src, "this is not a valid Mach-O file");
+			var target = Path.Combine (root, "Contents", "MonoBundle", "libpayload.dylib");
+
+			var task = CreateTask<InstallNameTool> ();
+			task.IntermediateNativeLibraryDir = root;
+			var item = new TaskItem (src);
+			item.SetMetadata ("ReidentifiedPath", target);
+			item.SetMetadata ("DynamicLibraryId", "@executable_path/libpayload.dylib");
+			task.DynamicLibrary = new ITaskItem [] { item };
+
+			ExecuteTask (task, 1);
+
+			Assert.That (target + ".tmp", Does.Not.Exist, "temporary file cleaned up");
+			Assert.That (target, Does.Not.Exist, "target not created on failure");
+		}
+	}
+}