[msbuild] Don't let binding sidecar manifest metadata redirect native reference output paths#25790
Conversation
… 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>
There was a problem hiding this comment.
Pull request overview
Hardens the MSBuild native-reference pipeline against untrusted binding sidecar manifests by preventing manifest metadata from influencing output paths and by enforcing output-path containment during dylib reidentification.
Changes:
- Restricts sidecar-manifest metadata copying in
ResolveNativeReferencesto an allow-list (warning on ignored keys) and rejects rooted/..traversal inNativeReference Name. - Adds a containment check in
InstallNameToolto prevent writing reidentified dylibs outside the intended intermediate directory, and wires the root directory from targets. - Adds regression tests covering ignored manifest metadata, traversal rejection, containment logic, and attempted path escapes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/ResolveNativeReferencesSidecarTest.cs | Adds regression tests ensuring unsafe manifest metadata is ignored and traversal in Name is rejected. |
| tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/InstallNameToolTaskTest.cs | Adds regression tests for containment checks and refusal to write outside the intended directory (including symlink escape). |
| msbuild/Xamarin.MacDev.Tasks/Tasks/ResolveNativeReferences.cs | Implements allow-listed manifest metadata copying + traversal/rooted validation for sidecar Name. |
| msbuild/Xamarin.MacDev.Tasks/Tasks/InstallNameTool.cs | Adds required IntermediateNativeLibraryDir, containment enforcement, and path canonicalization helpers. |
| msbuild/Xamarin.MacDev.Tasks/Tasks/CreateBindingResourcePackage.cs | Exposes the producer’s manifest attribute list for reuse by the consumer allow-list. |
| msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx | Adds localized warning/error strings for ignored metadata, invalid names, and out-of-root reidentification. |
| dotnet/targets/Xamarin.Shared.Sdk.targets | Passes the intermediate native library root into InstallNameTool. |
| 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; | ||
| } |
| 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); |
| 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); | ||
| } |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The failing tests look related to these changes. |
…view 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>
✅ [PR Build #5367b26] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #5367b26] Build passed (Build packages) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [PR Build #5367b26] Build passed (Build macOS tests) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #5367b26] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 207 tests passed 🎉 Tests counts✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
A binding
.resourcessidecar manifest is passive data that may come from arestored package, but
ResolveNativeReferencescopied every manifest element intoitem metadata. Build-controlled path/identity metadata such as
RelativePath,ReidentifiedPathorDynamicLibraryIdcould then flow into the native-libraryoutput path and make
InstallNameToolcreate directories and write files outsidethe intended intermediate output directory.
This PR:
build-controlled path/layout/identity sinks (
RelativePath,ReidentifiedPath,ComputedRelativePath,DynamicLibraryId,PublishFolderType,TargetDirectory,SourceDirectory), warning when one is dropped. Everything else — includingbinding-defined metadata like
NoDSymUtil/NoSymbolStrip— is legitimatenative-reference content and is copied as-is.
..traversal segments, and ignores reserved MSBuild metadata names (e.g.
FullPath)instead of crashing the task.
PathUtils.IsPathContainedcontainment check so a reidentified nativelibrary can never be written outside the intended output directory, and deletes
the temporary file if
install_name_toolfails.