From 474a353a3e588912022082f3c559ecfddfe0e83e Mon Sep 17 00:00:00 2001 From: Alex Soto Date: Tue, 23 Jun 2026 16:37:37 -0400 Subject: [PATCH 1/2] [msbuild] Don't let binding sidecar manifest metadata redirect native reference output paths. A binding `.resources` sidecar manifest is passive data that may come from a restored package, but `ResolveNativeReferences` copied every manifest element into item metadata. Path/identity metadata such as `RelativePath`, `ReidentifiedPath` or `DynamicLibraryId` could then flow into the reidentified native-library path and make `InstallNameTool` create directories and write files outside the intended intermediate output directory. Only copy an allow-list of metadata from the manifest (warning on the rest), reject native reference names containing rooted/`..` traversal segments, and add a containment check in `InstallNameTool` so a reidentified library can never be written outside the intended output directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dotnet/targets/Xamarin.Shared.Sdk.targets | 1 + .../MSBStrings.resx | 19 +++ .../Tasks/CreateBindingResourcePackage.cs | 2 +- .../Tasks/InstallNameTool.cs | 66 +++++++++- .../Tasks/ResolveNativeReferences.cs | 60 ++++++++- .../TaskTests/InstallNameToolTaskTest.cs | 115 ++++++++++++++++++ .../ResolveNativeReferencesSidecarTest.cs | 89 ++++++++++++++ 7 files changed, 347 insertions(+), 5 deletions(-) create mode 100644 tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/InstallNameToolTaskTest.cs create mode 100644 tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/ResolveNativeReferencesSidecarTest.cs diff --git a/dotnet/targets/Xamarin.Shared.Sdk.targets b/dotnet/targets/Xamarin.Shared.Sdk.targets index a40c8eeb6a41..edf3226b5cae 100644 --- a/dotnet/targets/Xamarin.Shared.Sdk.targets +++ b/dotnet/targets/Xamarin.Shared.Sdk.targets @@ -2098,6 +2098,7 @@ Condition="'$(IsMacEnabled)' == 'true'" SessionId="$(BuildSessionId)" DynamicLibrary="@(_DynamicLibraryToReidentify)" + IntermediateNativeLibraryDir="$(_IntermediateNativeLibraryDir)" SdkDevPath="$(_SdkDevPath)" /> diff --git a/msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx b/msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx index 0741be004f37..b2600d09f8b1 100644 --- a/msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx +++ b/msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx @@ -1666,4 +1666,23 @@ Shown when an MSBuild task writes to the console instead of using MSBuild logging. {0} - The stack trace of the offending call. + + Ignoring unsupported metadata '{0}' for the native reference '{1}' in the binding resource package '{2}'. + Shown when a binding resource package's manifest contains metadata that isn't on the allow-list of supported metadata. +{0} - The name of the unsupported metadata (e.g. "RelativePath"). +{1} - The name of the native reference (e.g. "libfoo.dylib"). +{2} - The path to the binding resource package. + + + The native reference '{0}' in the binding resource package '{1}' is invalid: the name must be a relative path without '..' segments. + 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. + + + The native library can't be reidentified to '{0}' because that path is outside the intended output directory '{1}'. + 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. + diff --git a/msbuild/Xamarin.MacDev.Tasks/Tasks/CreateBindingResourcePackage.cs b/msbuild/Xamarin.MacDev.Tasks/Tasks/CreateBindingResourcePackage.cs index 722104a9e119..9a3e8feeda99 100644 --- a/msbuild/Xamarin.MacDev.Tasks/Tasks/CreateBindingResourcePackage.cs +++ b/msbuild/Xamarin.MacDev.Tasks/Tasks/CreateBindingResourcePackage.cs @@ -139,7 +139,7 @@ bool ContainsSymlinks (ITaskItem [] items) return false; } - string [] NativeReferenceAttributeNames = new string [] { "Kind", "ForceLoad", "SmartLink", "Frameworks", "WeakFrameworks", "LinkerFlags", "NeedsGccExceptionHandling", "IsCxx", "LinkWithSwiftSystemLibraries" }; + internal static readonly string [] NativeReferenceAttributeNames = new string [] { "Kind", "ForceLoad", "SmartLink", "Frameworks", "WeakFrameworks", "LinkerFlags", "NeedsGccExceptionHandling", "IsCxx", "LinkWithSwiftSystemLibraries" }; string CreateManifest (string resourcePath) { diff --git a/msbuild/Xamarin.MacDev.Tasks/Tasks/InstallNameTool.cs b/msbuild/Xamarin.MacDev.Tasks/Tasks/InstallNameTool.cs index 341c6dbe1b01..c8caae02f309 100644 --- a/msbuild/Xamarin.MacDev.Tasks/Tasks/InstallNameTool.cs +++ b/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 (!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. @@ -50,7 +68,7 @@ public override bool Execute () processes [i] = ExecuteAsync ("xcrun", arguments).ContinueWith ((v) => { if (v.IsFaulted) throw v.Exception; - if (v.Status == TaskStatus.RanToCompletion) { + if (v.Status == TaskStatus.RanToCompletion && v.Result.ExitCode == 0) { File.Delete (target); File.Move (temporaryTarget, target); } @@ -61,9 +79,55 @@ 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; } + // Returns true if 'target' (which may not exist yet) is located within 'root' after fully + // canonicalizing both paths (resolving '..' segments and symbolic links). Used to make sure we + // never write reidentified native libraries outside the intended intermediate output directory, + // even if the path was influenced by untrusted binding resource package metadata. + public static bool IsPathContained (string root, string target) + { + if (string.IsNullOrEmpty (root) || string.IsNullOrEmpty (target)) + return false; + + var canonicalRoot = CanonicalizeForContainment (root); + var canonicalTarget = CanonicalizeForContainment (target); + + var rootWithSeparator = canonicalRoot.TrimEnd (Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar; + return canonicalTarget.StartsWith (rootWithSeparator, StringComparison.Ordinal); + } + + // Canonicalizes a path for a containment check: makes it absolute and resolves '..'/'.' segments, + // then resolves symbolic links for the longest existing prefix (the target file usually doesn't + // exist yet, so we resolve symlinks on the nearest existing ancestor and re-append the remainder). + static string CanonicalizeForContainment (string path) + { + // The root can come from MSBuild with Windows separators (e.g. 'obj\...\nativelibraries\') + // while the target has already been separator-normalized, so normalize here as well. + path = path.Replace ('\\', Path.DirectorySeparatorChar); + var full = Path.GetFullPath (path); + + var existing = full; + var remainder = new List (); + while (existing.Length > 0 && !Directory.Exists (existing) && !File.Exists (existing)) { + var parent = Path.GetDirectoryName (existing); + if (string.IsNullOrEmpty (parent) || parent == existing) + break; + remainder.Insert (0, Path.GetFileName (existing)); + existing = parent; + } + + var canonical = PathUtils.ResolveSymbolicLinks (existing) ?? existing; + foreach (var segment in remainder) + canonical = Path.Combine (canonical, segment); + + return canonical; + } + public bool ShouldCopyToBuildServer (ITaskItem item) => true; public bool ShouldCreateOutputFile (ITaskItem item) => true; public IEnumerable GetAdditionalItemsToBeCopied () => Enumerable.Empty (); diff --git a/msbuild/Xamarin.MacDev.Tasks/Tasks/ResolveNativeReferences.cs b/msbuild/Xamarin.MacDev.Tasks/Tasks/ResolveNativeReferences.cs index 73c8dcc7dee5..6fb316caaa88 100644 --- a/msbuild/Xamarin.MacDev.Tasks/Tasks/ResolveNativeReferences.cs +++ b/msbuild/Xamarin.MacDev.Tasks/Tasks/ResolveNativeReferences.cs @@ -299,6 +299,44 @@ void SetMetadataNativeLibrary (ITaskItem item, string nativeLibraryPath) item.SetMetadata ("RelativePath", Path.Combine (FrameworksDirectory, Path.GetFileName (Path.GetDirectoryName (item.ItemSpec)!))); } + // The metadata we accept 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 + // path/layout/identity metadata (e.g. 'RelativePath', 'ReidentifiedPath', 'ComputedRelativePath' + // or 'DynamicLibraryId') that could redirect this task's (or a downstream task's) output outside + // the intended output directory. + static readonly HashSet allowedManifestMetadata = CreateAllowedManifestMetadata (); + + static HashSet CreateAllowedManifestMetadata () + { + // The attributes the producer writes into the manifest (see CreateBindingResourcePackage). + var allowed = new HashSet (CreateBindingResourcePackage.NativeReferenceAttributeNames, StringComparer.OrdinalIgnoreCase); + // Additional safe (non-path) behavior flags the consuming build honors, but which aren't part + // of the producer's fixed attribute list. + allowed.Add ("CopyToAppBundle"); + allowed.Add ("LinkToExecutable"); + return allowed; + } + + // 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 native_frameworks, List createdFiles, CancellationToken? cancellationToken) { if (!TryGetSidecarManifest (Log, resources, out var manifestContents)) @@ -310,6 +348,13 @@ void ProcessSidecar (ITaskItem r, string resources, List native_frame foreach (XmlNode referenceNode in document.GetElementsByTagName ("NativeReference")) { ITaskItem t = new TaskItem (r); var name = referenceNode.Attributes? ["Name"]?.Value.Trim ('\\', '/') ?? string.Empty; + // 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 +404,18 @@ void ProcessSidecar (ITaskItem r, string resources, List 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. Only an allow-list of + // metadata is copied: the manifest is passive data and must not be able to inject + // path/layout/identity metadata that could redirect output outside the intended directory. + foreach (XmlNode attribute in referenceNode.ChildNodes) { + if (attribute.NodeType != XmlNodeType.Element) + continue; + if (allowedManifestMetadata.Contains (attribute.Name)) { + t.SetMetadata (attribute.Name, attribute.InnerText); + } else { + Log.LogWarning (MSBStrings.W7179 /* Ignoring unsupported metadata '{0}' for the native reference '{1}' in the binding resource package '{2}'. */, attribute.Name, name, r.ItemSpec); + } + } native_frameworks.Add (t); } diff --git a/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/InstallNameToolTaskTest.cs b/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/InstallNameToolTaskTest.cs new file mode 100644 index 000000000000..40c405d414e5 --- /dev/null +++ b/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/InstallNameToolTaskTest.cs @@ -0,0 +1,115 @@ +using System.IO; + +using Microsoft.Build.Framework; +using Microsoft.Build.Utilities; + +using NUnit.Framework; + +using Xamarin.Utils; + +#nullable enable + +namespace Xamarin.MacDev.Tasks { + + // Regression tests for the containment check that makes sure a reidentified native library is never + // written outside the intended intermediate output directory, even if the 'ReidentifiedPath' was + // influenced by metadata that originates from a (passive, potentially untrusted) binding resource + // package manifest. + [TestFixture] + public class InstallNameToolTaskTest : TestBase { + + [Test] + public void IsPathContained_Contained () + { + var tmp = Cache.CreateTemporaryDirectory (); + var root = Path.Combine (tmp, "obj", "nativelibraries"); + Directory.CreateDirectory (root); + + Assert.That (InstallNameTool.IsPathContained (root, Path.Combine (root, "Contents", "MonoBundle", "lib.dylib")), Is.True, "subdir"); + Assert.That (InstallNameTool.IsPathContained (root, Path.Combine (root, "lib.dylib")), Is.True, "direct child"); + // A trailing separator on the root must not change the result. + Assert.That (InstallNameTool.IsPathContained (root + Path.DirectorySeparatorChar, Path.Combine (root, "lib.dylib")), Is.True, "trailing separator root"); + // A Windows-style (backslash) root must be normalized to match a slash-normalized target + // (this happens on remote Windows -> Mac builds). + Assert.That (InstallNameTool.IsPathContained (root.Replace (Path.DirectorySeparatorChar, '\\'), Path.Combine (root, "lib.dylib")), Is.True, "backslash root"); + } + + [Test] + public void IsPathContained_NotContained () + { + var tmp = Cache.CreateTemporaryDirectory (); + var root = Path.Combine (tmp, "obj", "nativelibraries"); + Directory.CreateDirectory (root); + + // '..' traversal that escapes the root. + Assert.That (InstallNameTool.IsPathContained (root, Path.Combine (root, "..", "..", "escape.dylib")), Is.False, "traversal"); + // An absolute path outside the root. + Assert.That (InstallNameTool.IsPathContained (root, Path.Combine (tmp, "escape.dylib")), Is.False, "outside"); + // A sibling directory that merely shares the root as a string prefix must not be considered contained. + Assert.That (InstallNameTool.IsPathContained (root, root + "EVIL" + Path.DirectorySeparatorChar + "lib.dylib"), Is.False, "sibling prefix"); + // The root itself isn't a contained target (it's a directory, not a file under the root). + Assert.That (InstallNameTool.IsPathContained (root, root), Is.False, "root itself"); + // Empty inputs are never contained. + Assert.That (InstallNameTool.IsPathContained ("", Path.Combine (root, "lib.dylib")), Is.False, "empty root"); + Assert.That (InstallNameTool.IsPathContained (root, ""), Is.False, "empty target"); + } + + [Test] + public void IsPathContained_SymlinkEscape () + { + var tmp = Cache.CreateTemporaryDirectory (); + var root = Path.Combine (tmp, "obj", "nativelibraries"); + Directory.CreateDirectory (root); + var outside = Path.Combine (tmp, "outside"); + Directory.CreateDirectory (outside); + + // A symlink inside the root that points outside the root must not be usable to escape. + var link = Path.Combine (root, "link"); + PathUtils.CreateSymlink (link, outside); + + Assert.That (InstallNameTool.IsPathContained (root, Path.Combine (link, "evil.dylib")), Is.False, "symlink escape"); + } + + [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 (); + 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"); + } + } +} diff --git a/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/ResolveNativeReferencesSidecarTest.cs b/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/ResolveNativeReferencesSidecarTest.cs new file mode 100644 index 000000000000..3b30854728da --- /dev/null +++ b/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/ResolveNativeReferencesSidecarTest.cs @@ -0,0 +1,89 @@ +using System.IO; +using System.Linq; + +using Microsoft.Build.Framework; +using Microsoft.Build.Utilities; + +using NUnit.Framework; + +#nullable enable + +namespace Xamarin.MacDev.Tasks { + + // Regression tests for https://github.com/dotnet/macios - a binding resource package's 'manifest' is + // passive data that may come from a restored (and potentially untrusted) package, so it must not be + // able to inject path/layout/identity metadata that could redirect this task's (or a downstream + // task's) output outside the intended output directory. + [TestFixture] + public class ResolveNativeReferencesSidecarTest : TestBase { + + ResolveNativeReferences CreateSidecarTask (string manifestContents, out string tmpdir) + { + tmpdir = Cache.CreateTemporaryDirectory (); + var resources = Path.Combine (tmpdir, "Binding.resources"); + Directory.CreateDirectory (resources); + File.WriteAllText (Path.Combine (resources, "manifest"), manifestContents); + // Create the (fake) native library referenced by the manifest, for realism. + File.WriteAllText (Path.Combine (resources, "libpayload.dylib"), "fake dylib"); + + var task = CreateTask (); + task.Architectures = "arm64"; + task.FrameworksDirectory = "Frameworks"; + task.IntermediateOutputPath = Path.Combine (tmpdir, "obj"); + task.SdkIsSimulator = false; + task.TargetFrameworkMoniker = "net9.0-macos"; + task.References = new ITaskItem [] { new TaskItem (Path.Combine (tmpdir, "Binding.dll")) }; + return task; + } + + [Test] + public void StripsUnsafeManifestMetadata () + { + var manifest = @" + + Dynamic + True + CoreFoundation + ../../../../../../tmp/escape/libpayload.dylib + /tmp/escape/libpayload.dylib + @executable_path/../../../../escape + +"; + var task = CreateSidecarTask (manifest, out var _); + + ExecuteTask (task, 0); + + var item = task.NativeFrameworks!.Single (v => v.GetMetadata ("Kind") == "Dynamic"); + + // Allowed (non-path) metadata is preserved (overriding the defaults). + Assert.That (item.GetMetadata ("ForceLoad"), Is.EqualTo ("True"), "ForceLoad"); + Assert.That (item.GetMetadata ("Frameworks"), Is.EqualTo ("CoreFoundation"), "Frameworks"); + + // Path/layout/identity metadata is NOT copied from the manifest. + Assert.That (item.GetMetadata ("RelativePath"), Is.Empty, "RelativePath"); + Assert.That (item.GetMetadata ("ReidentifiedPath"), Is.Empty, "ReidentifiedPath"); + Assert.That (item.GetMetadata ("DynamicLibraryId"), Is.Empty, "DynamicLibraryId"); + + // A warning is emitted for each ignored metadata. + var warnings = Engine.Logger.WarningsEvents.Select (v => v.Message ?? "").ToArray (); + Assert.That (warnings.Count (v => v.Contains ("RelativePath")), Is.EqualTo (1), "RelativePath warning"); + Assert.That (warnings.Count (v => v.Contains ("ReidentifiedPath")), Is.EqualTo (1), "ReidentifiedPath warning"); + Assert.That (warnings.Count (v => v.Contains ("DynamicLibraryId")), Is.EqualTo (1), "DynamicLibraryId warning"); + } + + [Test] + public void RejectsTraversalInName () + { + var manifest = @" + + Dynamic + +"; + var task = CreateSidecarTask (manifest, out var _); + + ExecuteTask (task, 1); + + Assert.That (Engine.Logger.ErrorEvents.Single ().Message, Does.Contain ("'..'"), "error mentions traversal"); + } + } +} From 5367b2671ee93106069f3967cda87a5e46c58e68 Mon Sep 17 00:00:00 2001 From: Alex Soto Date: Wed, 24 Jun 2026 10:10:45 -0400 Subject: [PATCH 2/2] [msbuild] Use a block-list for binding sidecar manifest metadata + review fixes. Real bindings carry legitimate native-reference metadata beyond a fixed set (e.g. NoDSymUtil, NoSymbolStrip), so filter the manifest with a block-list of the build-controlled path/identity sinks instead of an allow-list - otherwise that metadata is dropped (fixes the BundleStructureTest CI regression). Also from review: ignore reserved MSBuild metadata names instead of crashing the task, skip nameless native references, delete the temporary file when install_name_tool fails, reject target == root in the containment check, and move IsPathContained into PathUtils. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../MSBStrings.resx | 11 ++- .../Tasks/CreateBindingResourcePackage.cs | 2 +- .../Tasks/InstallNameTool.cs | 53 ++---------- .../Tasks/ResolveNativeReferences.cs | 58 +++++++------ .../TaskTests/InstallNameToolTaskTest.cs | 86 ++++++------------- .../ResolveNativeReferencesSidecarTest.cs | 76 +++++++++++++--- .../UtilityTests.cs | 55 ++++++++++++ tools/common/PathUtils.cs | 50 +++++++++++ 8 files changed, 247 insertions(+), 144 deletions(-) diff --git a/msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx b/msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx index b2600d09f8b1..0602550cda86 100644 --- a/msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx +++ b/msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx @@ -1667,9 +1667,9 @@ {0} - The stack trace of the offending call. - Ignoring unsupported metadata '{0}' for the native reference '{1}' in the binding resource package '{2}'. - Shown when a binding resource package's manifest contains metadata that isn't on the allow-list of supported metadata. -{0} - The name of the unsupported metadata (e.g. "RelativePath"). + 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. + 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. @@ -1685,4 +1685,9 @@ {0} - The computed (out-of-bounds) reidentified path. {1} - The intended output directory. + + Ignoring a native reference with no name in the binding resource package '{0}'. + 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. + diff --git a/msbuild/Xamarin.MacDev.Tasks/Tasks/CreateBindingResourcePackage.cs b/msbuild/Xamarin.MacDev.Tasks/Tasks/CreateBindingResourcePackage.cs index 9a3e8feeda99..722104a9e119 100644 --- a/msbuild/Xamarin.MacDev.Tasks/Tasks/CreateBindingResourcePackage.cs +++ b/msbuild/Xamarin.MacDev.Tasks/Tasks/CreateBindingResourcePackage.cs @@ -139,7 +139,7 @@ bool ContainsSymlinks (ITaskItem [] items) return false; } - internal static readonly string [] NativeReferenceAttributeNames = new string [] { "Kind", "ForceLoad", "SmartLink", "Frameworks", "WeakFrameworks", "LinkerFlags", "NeedsGccExceptionHandling", "IsCxx", "LinkWithSwiftSystemLibraries" }; + string [] NativeReferenceAttributeNames = new string [] { "Kind", "ForceLoad", "SmartLink", "Frameworks", "WeakFrameworks", "LinkerFlags", "NeedsGccExceptionHandling", "IsCxx", "LinkWithSwiftSystemLibraries" }; string CreateManifest (string resourcePath) { diff --git a/msbuild/Xamarin.MacDev.Tasks/Tasks/InstallNameTool.cs b/msbuild/Xamarin.MacDev.Tasks/Tasks/InstallNameTool.cs index c8caae02f309..9e189dfab358 100644 --- a/msbuild/Xamarin.MacDev.Tasks/Tasks/InstallNameTool.cs +++ b/msbuild/Xamarin.MacDev.Tasks/Tasks/InstallNameTool.cs @@ -46,7 +46,7 @@ public override bool Execute () // 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 (!IsPathContained (IntermediateNativeLibraryDir, target)) { + 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; @@ -66,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 && 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); } }); @@ -85,49 +91,6 @@ public override bool Execute () return !Log.HasLoggedErrors; } - // Returns true if 'target' (which may not exist yet) is located within 'root' after fully - // canonicalizing both paths (resolving '..' segments and symbolic links). Used to make sure we - // never write reidentified native libraries outside the intended intermediate output directory, - // even if the path was influenced by untrusted binding resource package metadata. - public static bool IsPathContained (string root, string target) - { - if (string.IsNullOrEmpty (root) || string.IsNullOrEmpty (target)) - return false; - - var canonicalRoot = CanonicalizeForContainment (root); - var canonicalTarget = CanonicalizeForContainment (target); - - var rootWithSeparator = canonicalRoot.TrimEnd (Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar; - return canonicalTarget.StartsWith (rootWithSeparator, StringComparison.Ordinal); - } - - // Canonicalizes a path for a containment check: makes it absolute and resolves '..'/'.' segments, - // then resolves symbolic links for the longest existing prefix (the target file usually doesn't - // exist yet, so we resolve symlinks on the nearest existing ancestor and re-append the remainder). - static string CanonicalizeForContainment (string path) - { - // The root can come from MSBuild with Windows separators (e.g. 'obj\...\nativelibraries\') - // while the target has already been separator-normalized, so normalize here as well. - path = path.Replace ('\\', Path.DirectorySeparatorChar); - var full = Path.GetFullPath (path); - - var existing = full; - var remainder = new List (); - while (existing.Length > 0 && !Directory.Exists (existing) && !File.Exists (existing)) { - var parent = Path.GetDirectoryName (existing); - if (string.IsNullOrEmpty (parent) || parent == existing) - break; - remainder.Insert (0, Path.GetFileName (existing)); - existing = parent; - } - - var canonical = PathUtils.ResolveSymbolicLinks (existing) ?? existing; - foreach (var segment in remainder) - canonical = Path.Combine (canonical, segment); - - return canonical; - } - public bool ShouldCopyToBuildServer (ITaskItem item) => true; public bool ShouldCreateOutputFile (ITaskItem item) => true; public IEnumerable GetAdditionalItemsToBeCopied () => Enumerable.Empty (); diff --git a/msbuild/Xamarin.MacDev.Tasks/Tasks/ResolveNativeReferences.cs b/msbuild/Xamarin.MacDev.Tasks/Tasks/ResolveNativeReferences.cs index 6fb316caaa88..da54198cb28f 100644 --- a/msbuild/Xamarin.MacDev.Tasks/Tasks/ResolveNativeReferences.cs +++ b/msbuild/Xamarin.MacDev.Tasks/Tasks/ResolveNativeReferences.cs @@ -299,24 +299,22 @@ void SetMetadataNativeLibrary (ITaskItem item, string nativeLibraryPath) item.SetMetadata ("RelativePath", Path.Combine (FrameworksDirectory, Path.GetFileName (Path.GetDirectoryName (item.ItemSpec)!))); } - // The metadata we accept 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 - // path/layout/identity metadata (e.g. 'RelativePath', 'ReidentifiedPath', 'ComputedRelativePath' - // or 'DynamicLibraryId') that could redirect this task's (or a downstream task's) output outside - // the intended output directory. - static readonly HashSet allowedManifestMetadata = CreateAllowedManifestMetadata (); - - static HashSet CreateAllowedManifestMetadata () - { - // The attributes the producer writes into the manifest (see CreateBindingResourcePackage). - var allowed = new HashSet (CreateBindingResourcePackage.NativeReferenceAttributeNames, StringComparer.OrdinalIgnoreCase); - // Additional safe (non-path) behavior flags the consuming build honors, but which aren't part - // of the producer's fixed attribute list. - allowed.Add ("CopyToAppBundle"); - allowed.Add ("LinkToExecutable"); - return allowed; - } + // 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 blockedManifestMetadata = new HashSet (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 @@ -348,6 +346,12 @@ void ProcessSidecar (ITaskItem r, string resources, List 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. @@ -404,16 +408,22 @@ void ProcessSidecar (ITaskItem r, string resources, List native_frame t.SetMetadata ("LinkWithSwiftSystemLibraries", "False"); t.SetMetadata ("SmartLink", "True"); - // Values from the manifest, overriding the defaults above if provided. Only an allow-list of - // metadata is copied: the manifest is passive data and must not be able to inject - // path/layout/identity metadata that could redirect output outside the intended directory. + // 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 (allowedManifestMetadata.Contains (attribute.Name)) { + 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); - } else { - Log.LogWarning (MSBStrings.W7179 /* Ignoring unsupported metadata '{0}' for the native reference '{1}' in the binding resource package '{2}'. */, attribute.Name, name, r.ItemSpec); + } 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); } } diff --git a/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/InstallNameToolTaskTest.cs b/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/InstallNameToolTaskTest.cs index 40c405d414e5..c56e02f01869 100644 --- a/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/InstallNameToolTaskTest.cs +++ b/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/InstallNameToolTaskTest.cs @@ -5,71 +5,16 @@ using NUnit.Framework; -using Xamarin.Utils; - #nullable enable namespace Xamarin.MacDev.Tasks { - // Regression tests for the containment check that makes sure a reidentified native library is never - // written outside the intended intermediate output directory, even if the 'ReidentifiedPath' was - // influenced by metadata that originates from a (passive, potentially untrusted) binding resource - // package manifest. + // 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 { - [Test] - public void IsPathContained_Contained () - { - var tmp = Cache.CreateTemporaryDirectory (); - var root = Path.Combine (tmp, "obj", "nativelibraries"); - Directory.CreateDirectory (root); - - Assert.That (InstallNameTool.IsPathContained (root, Path.Combine (root, "Contents", "MonoBundle", "lib.dylib")), Is.True, "subdir"); - Assert.That (InstallNameTool.IsPathContained (root, Path.Combine (root, "lib.dylib")), Is.True, "direct child"); - // A trailing separator on the root must not change the result. - Assert.That (InstallNameTool.IsPathContained (root + Path.DirectorySeparatorChar, Path.Combine (root, "lib.dylib")), Is.True, "trailing separator root"); - // A Windows-style (backslash) root must be normalized to match a slash-normalized target - // (this happens on remote Windows -> Mac builds). - Assert.That (InstallNameTool.IsPathContained (root.Replace (Path.DirectorySeparatorChar, '\\'), Path.Combine (root, "lib.dylib")), Is.True, "backslash root"); - } - - [Test] - public void IsPathContained_NotContained () - { - var tmp = Cache.CreateTemporaryDirectory (); - var root = Path.Combine (tmp, "obj", "nativelibraries"); - Directory.CreateDirectory (root); - - // '..' traversal that escapes the root. - Assert.That (InstallNameTool.IsPathContained (root, Path.Combine (root, "..", "..", "escape.dylib")), Is.False, "traversal"); - // An absolute path outside the root. - Assert.That (InstallNameTool.IsPathContained (root, Path.Combine (tmp, "escape.dylib")), Is.False, "outside"); - // A sibling directory that merely shares the root as a string prefix must not be considered contained. - Assert.That (InstallNameTool.IsPathContained (root, root + "EVIL" + Path.DirectorySeparatorChar + "lib.dylib"), Is.False, "sibling prefix"); - // The root itself isn't a contained target (it's a directory, not a file under the root). - Assert.That (InstallNameTool.IsPathContained (root, root), Is.False, "root itself"); - // Empty inputs are never contained. - Assert.That (InstallNameTool.IsPathContained ("", Path.Combine (root, "lib.dylib")), Is.False, "empty root"); - Assert.That (InstallNameTool.IsPathContained (root, ""), Is.False, "empty target"); - } - - [Test] - public void IsPathContained_SymlinkEscape () - { - var tmp = Cache.CreateTemporaryDirectory (); - var root = Path.Combine (tmp, "obj", "nativelibraries"); - Directory.CreateDirectory (root); - var outside = Path.Combine (tmp, "outside"); - Directory.CreateDirectory (outside); - - // A symlink inside the root that points outside the root must not be usable to escape. - var link = Path.Combine (root, "link"); - PathUtils.CreateSymlink (link, outside); - - Assert.That (InstallNameTool.IsPathContained (root, Path.Combine (link, "evil.dylib")), Is.False, "symlink escape"); - } - [TestCase ("traversal")] [TestCase ("absolute")] [TestCase ("mixedseparators")] @@ -111,5 +56,30 @@ public void RefusesToWriteOutOfRoot (string kind) 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 (); + 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"); + } } } diff --git a/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/ResolveNativeReferencesSidecarTest.cs b/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/ResolveNativeReferencesSidecarTest.cs index 3b30854728da..aa1df655b54b 100644 --- a/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/ResolveNativeReferencesSidecarTest.cs +++ b/tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/ResolveNativeReferencesSidecarTest.cs @@ -10,10 +10,10 @@ namespace Xamarin.MacDev.Tasks { - // Regression tests for https://github.com/dotnet/macios - a binding resource package's 'manifest' is - // passive data that may come from a restored (and potentially untrusted) package, so it must not be - // able to inject path/layout/identity metadata that could redirect this task's (or a downstream - // task's) output outside the intended output directory. + // Regression tests for the binding resource package 'manifest' handling: a manifest is passive data + // that may come from a restored package, so it must not be able to inject path/layout/identity + // metadata that could redirect this task's (or a downstream task's) output outside the intended + // output directory. [TestFixture] public class ResolveNativeReferencesSidecarTest : TestBase { @@ -44,9 +44,15 @@ public void StripsUnsafeManifestMetadata () Dynamic True CoreFoundation + true + true ../../../../../../tmp/escape/libpayload.dylib /tmp/escape/libpayload.dylib + ../../escape/libpayload.dylib @executable_path/../../../../escape + Unknown + /tmp/escape + /tmp/escape "; var task = CreateSidecarTask (manifest, out var _); @@ -55,20 +61,64 @@ public void StripsUnsafeManifestMetadata () var item = task.NativeFrameworks!.Single (v => v.GetMetadata ("Kind") == "Dynamic"); - // Allowed (non-path) metadata is preserved (overriding the defaults). + // Legitimate native-reference metadata (standard + binding-defined) is preserved. Assert.That (item.GetMetadata ("ForceLoad"), Is.EqualTo ("True"), "ForceLoad"); Assert.That (item.GetMetadata ("Frameworks"), Is.EqualTo ("CoreFoundation"), "Frameworks"); + Assert.That (item.GetMetadata ("NoDSymUtil"), Is.EqualTo ("true"), "NoDSymUtil"); + Assert.That (item.GetMetadata ("NoSymbolStrip"), Is.EqualTo ("true"), "NoSymbolStrip"); - // Path/layout/identity metadata is NOT copied from the manifest. - Assert.That (item.GetMetadata ("RelativePath"), Is.Empty, "RelativePath"); - Assert.That (item.GetMetadata ("ReidentifiedPath"), Is.Empty, "ReidentifiedPath"); - Assert.That (item.GetMetadata ("DynamicLibraryId"), Is.Empty, "DynamicLibraryId"); + // Build-controlled path/layout/identity metadata is NOT copied from the manifest. + foreach (var blocked in new [] { "RelativePath", "ReidentifiedPath", "ComputedRelativePath", "DynamicLibraryId", "TargetDirectory", "SourceDirectory" }) + Assert.That (item.GetMetadata (blocked), Is.Empty, blocked); + // The code-set PublishFolderType survives (the manifest's value is ignored). + Assert.That (item.GetMetadata ("PublishFolderType"), Is.EqualTo ("DynamicLibrary"), "PublishFolderType"); - // A warning is emitted for each ignored metadata. + // A warning is emitted for each ignored (blocked) metadata - and only for those (use the quoted + // name so e.g. 'ComputedRelativePath' isn't counted as 'RelativePath'). var warnings = Engine.Logger.WarningsEvents.Select (v => v.Message ?? "").ToArray (); - Assert.That (warnings.Count (v => v.Contains ("RelativePath")), Is.EqualTo (1), "RelativePath warning"); - Assert.That (warnings.Count (v => v.Contains ("ReidentifiedPath")), Is.EqualTo (1), "ReidentifiedPath warning"); - Assert.That (warnings.Count (v => v.Contains ("DynamicLibraryId")), Is.EqualTo (1), "DynamicLibraryId warning"); + foreach (var blocked in new [] { "RelativePath", "ReidentifiedPath", "ComputedRelativePath", "DynamicLibraryId", "PublishFolderType", "TargetDirectory", "SourceDirectory" }) + Assert.That (warnings.Count (v => v.Contains ($"'{blocked}'")), Is.EqualTo (1), $"{blocked} warning"); + foreach (var allowed in new [] { "NoDSymUtil", "NoSymbolStrip", "ForceLoad", "Frameworks" }) + Assert.That (warnings.Count (v => v.Contains ($"'{allowed}'")), Is.EqualTo (0), $"no warning for {allowed}"); + } + + [Test] + public void IgnoresReservedMetadata () + { + // A crafted manifest must not be able to crash the task with reserved MSBuild metadata names + // (TaskItem.SetMetadata throws an ArgumentException for those). + var manifest = @" + + Dynamic + evil + evil + +"; + var task = CreateSidecarTask (manifest, out var _); + + // The task completes without throwing/erroring (reaching this assertion means it didn't crash). + ExecuteTask (task, 0); + + var item = task.NativeFrameworks!.Single (v => v.GetMetadata ("Kind") == "Dynamic"); + // The reserved metadata keeps its item-spec-derived value; the manifest can't override it. + Assert.That (item.GetMetadata ("Filename"), Is.EqualTo ("libpayload"), "Filename"); + } + + [Test] + public void IgnoresNativeReferenceWithoutName () + { + var manifest = @" + + Dynamic + +"; + var task = CreateSidecarTask (manifest, out var _); + + ExecuteTask (task, 0); + + // The nameless native reference is skipped (the managed reference itself isn't added). + Assert.That (task.NativeFrameworks!, Is.Empty, "no native frameworks"); + Assert.That (Engine.Logger.WarningsEvents.Count (v => (v.Message ?? "").Contains ("no name")), Is.EqualTo (1), "no-name warning"); } [Test] diff --git a/tests/msbuild/Xamarin.MacDev.Tasks.Tests/UtilityTests.cs b/tests/msbuild/Xamarin.MacDev.Tasks.Tests/UtilityTests.cs index 4dfc91c8446b..10ac1a27dc02 100644 --- a/tests/msbuild/Xamarin.MacDev.Tasks.Tests/UtilityTests.cs +++ b/tests/msbuild/Xamarin.MacDev.Tasks.Tests/UtilityTests.cs @@ -21,5 +21,60 @@ public void TestAbsoluteToRelativePath () rpath = PathUtils.AbsoluteToRelative ("/Users/user/source/Project", "/Users/user/Source/Project/Info.plist"); Assert.That (rpath, Is.EqualTo ("Info.plist"), "#1"); } + + [Test] + public void IsPathContained_Contained () + { + var tmp = Cache.CreateTemporaryDirectory (); + var root = Path.Combine (tmp, "obj", "nativelibraries"); + Directory.CreateDirectory (root); + + Assert.That (PathUtils.IsPathContained (root, Path.Combine (root, "Contents", "MonoBundle", "lib.dylib")), Is.True, "subdir"); + Assert.That (PathUtils.IsPathContained (root, Path.Combine (root, "lib.dylib")), Is.True, "direct child"); + // A trailing separator on the root must not change the result. + Assert.That (PathUtils.IsPathContained (root + Path.DirectorySeparatorChar, Path.Combine (root, "lib.dylib")), Is.True, "trailing separator root"); + // A Windows-style (backslash) root must be normalized to match a slash-normalized target + // (this happens on remote Windows -> Mac builds). + Assert.That (PathUtils.IsPathContained (root.Replace (Path.DirectorySeparatorChar, '\\'), Path.Combine (root, "lib.dylib")), Is.True, "backslash root"); + } + + [Test] + public void IsPathContained_NotContained () + { + var tmp = Cache.CreateTemporaryDirectory (); + var root = Path.Combine (tmp, "obj", "nativelibraries"); + Directory.CreateDirectory (root); + + // '..' traversal that escapes the root. + Assert.That (PathUtils.IsPathContained (root, Path.Combine (root, "..", "..", "escape.dylib")), Is.False, "traversal"); + // An absolute path outside the root. + Assert.That (PathUtils.IsPathContained (root, Path.Combine (tmp, "escape.dylib")), Is.False, "outside"); + // A sibling directory that merely shares the root as a string prefix must not be considered contained. + Assert.That (PathUtils.IsPathContained (root, root + "EVIL" + Path.DirectorySeparatorChar + "lib.dylib"), Is.False, "sibling prefix"); + // The root itself isn't a contained target (it's a directory, not a file under the root). + Assert.That (PathUtils.IsPathContained (root, root), Is.False, "root itself"); + // The root itself with trailing separator(s) is still not a contained target. + Assert.That (PathUtils.IsPathContained (root, root + Path.DirectorySeparatorChar), Is.False, "root itself + separator"); + Assert.That (PathUtils.IsPathContained (root + Path.DirectorySeparatorChar, root + Path.DirectorySeparatorChar), Is.False, "root+sep vs root+sep"); + // Empty inputs are never contained. + Assert.That (PathUtils.IsPathContained ("", Path.Combine (root, "lib.dylib")), Is.False, "empty root"); + Assert.That (PathUtils.IsPathContained (root, ""), Is.False, "empty target"); + } + + [Test] + public void IsPathContained_SymlinkEscape () + { + var tmp = Cache.CreateTemporaryDirectory (); + var root = Path.Combine (tmp, "obj", "nativelibraries"); + Directory.CreateDirectory (root); + var outside = Path.Combine (tmp, "outside"); + Directory.CreateDirectory (outside); + + // A symlink inside the root that points outside the root must not be usable to escape. + var link = Path.Combine (root, "link"); + PathUtils.CreateSymlink (link, outside); + + Assert.That (PathUtils.IsPathContained (root, Path.Combine (link, "evil.dylib")), Is.False, "symlink escape"); + } } } diff --git a/tools/common/PathUtils.cs b/tools/common/PathUtils.cs index 355253f5c2a7..dca93cb74b2d 100644 --- a/tools/common/PathUtils.cs +++ b/tools/common/PathUtils.cs @@ -72,6 +72,56 @@ static char ToOrdinalIgnoreCase (char c) } } + /// + /// Returns true if is located strictly within + /// after fully canonicalizing both paths (making them absolute, resolving '..'/'.' segments and + /// resolving symbolic links). Either path may point to a location that doesn't exist yet. This is + /// useful to make sure a computed output path can't escape an intended output directory, even if + /// the path was influenced by untrusted input (e.g. via traversal segments, absolute paths or + /// symlinks). + /// + public static bool IsPathContained (string root, string target) + { + if (string.IsNullOrEmpty (root) || string.IsNullOrEmpty (target)) + return false; + + var canonicalRoot = CanonicalizeForContainment (root).TrimEnd (Path.DirectorySeparatorChar); + var canonicalTarget = CanonicalizeForContainment (target).TrimEnd (Path.DirectorySeparatorChar); + + // The target must be strictly inside the root, not the root directory itself. + if (string.Equals (canonicalTarget, canonicalRoot, StringComparison.Ordinal)) + return false; + + return canonicalTarget.StartsWith (canonicalRoot + Path.DirectorySeparatorChar, StringComparison.Ordinal); + } + + // Canonicalizes a path for a containment check: makes it absolute and resolves '..'/'.' segments, + // then resolves symbolic links for the longest existing prefix (the target usually doesn't exist + // yet, so we resolve symlinks on the nearest existing ancestor and re-append the remainder). + static string CanonicalizeForContainment (string path) + { + // The path can come from MSBuild with Windows separators (e.g. 'obj\...\nativelibraries\'), + // so normalize the separators first. + path = path.Replace ('\\', Path.DirectorySeparatorChar); + var full = Path.GetFullPath (path); + + var existing = full; + var remainder = new List (); + while (existing.Length > 0 && !Directory.Exists (existing) && !File.Exists (existing)) { + var parent = Path.GetDirectoryName (existing); + if (string.IsNullOrEmpty (parent) || parent == existing) + break; + remainder.Insert (0, Path.GetFileName (existing)); + existing = parent; + } + + var canonical = ResolveSymbolicLinks (existing) ?? existing; + foreach (var segment in remainder) + canonical = Path.Combine (canonical, segment); + + return canonical; + } + /// This works like , except that it works as it executing on Windows on all platforms. /// The directory the return value should be relative to. Must be an absolute path. /// The path whose relative value should be computed. Must be an absolute path.