Restrict embedded XML (dotnet/linker#1675)

* Restrict embedded XML

Embedded attributes and substitutions may only modify the containing assembly.

* Allow specifying resource name in referenced assembly

In the test infrastructure.

* Fix formatting

* PR feedback

- Allow '*' in corelib embedded attribute XML
- Reword warning messages
- Avoid calling virtual method from ctor

* Fix test on mono

Macking the core library causes problems for PEVerify:
- Removed reference to corelib creates invalid typerefs
- System types like Object aren't found

* PR feedback

* PR feedback

Commit migrated from dotnet/linker@9c88abd

src/tools/illink/docs/error-codes.md

@@ -1467,3 +1467,29 @@ This is technically possible if a custom assembly defines `DynamicDependencyAttr
   [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
   object TestProperty { get; set; }
   ```
+
+### 'IL2100': XML contains unsupported wildcard for assembly "fullname" attribute
+
+- A wildcard "fullname" for an assembly in XML is only valid for link attribute XML on the command-line, not for descriptor or substitution XML or for embedded attribute XML. Specify a specific assembly name instead.
+
+  ```XML
+  <!-- IL2100: XML contains unsupported wildcard for assembly "fullname" attribute -->
+  <linker>
+    <assembly fullname="*">
+      <type fullname="MyType" />
+    </assembly>
+  </linker>
+  ```
+
+### 'IL2101': Embedded XML in assembly 'assembly' contains assembly "fullname" attribute for another assembly 'assembly'
+
+- Embedded attribute or substitution XML may only contain elements that apply to the containing assembly. Attempting to modify another assembly will not have any effect.
+
+  ```XML
+  <!-- IL2101: Embedded XML in assembly 'ContainingAssembly' contains assembly "fullname" attribute for another assembly 'OtherAssembly' -->
+  <linker>
+    <assembly fullname="OtherAssembly">
+      <type fullname="MyType" />
+    </assembly>
+  </linker>
+  ```

src/tools/illink/src/linker/Linker.Steps/BodySubstituterStep.cs

@@ -24,10 +24,10 @@ protected override void Process ()
 			ProcessXml (Context.StripSubstitutions, Context.IgnoreSubstitutions);
 		}
 
-		protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNodeIterator iterator, bool warnOnUnresolvedTypes)
+		protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes)
 		{
-			ProcessTypes (assembly, iterator, warnOnUnresolvedTypes);
-			ProcessResources (assembly, iterator.Current.SelectChildren ("resource", ""));
+			ProcessTypes (assembly, nav, warnOnUnresolvedTypes);
+			ProcessResources (assembly, nav.SelectChildren ("resource", ""));
 		}
 
 		protected override TypeDefinition ProcessExportedType (ExportedType exported, AssemblyDefinition assembly) => null;

src/tools/illink/src/linker/Linker.Steps/LinkAttributesStep.cs

@@ -197,14 +197,30 @@ protected override void Process ()
 			ProcessXml (Context.StripLinkAttributes, Context.IgnoreLinkAttributes);
 		}
 
-		protected override bool AllowAllAssembliesSelector { get => true; }
+		protected override AllowedAssemblies AllowedAssemblySelector {
+			get {
+				if (_resourceAssembly == null)
+					return AllowedAssemblies.AllAssemblies;
+
+				// Corelib XML may contain assembly wildcard to support compiler-injected attribute types
+#if NETCOREAPP
+				const string coreLib = "System.Private.CoreLib";
+#else
+				const string coreLib = "mscorlib";
+#endif
+				if (_resourceAssembly.Name.Name == coreLib)
+					return AllowedAssemblies.AllAssemblies;
+
+				return AllowedAssemblies.ContainingAssembly;
+			}
+		}
 
-		protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNodeIterator iterator, bool warnOnUnresolvedTypes)
+		protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes)
 		{
-			IEnumerable<CustomAttribute> attributes = ProcessAttributes (iterator.Current, assembly);
+			IEnumerable<CustomAttribute> attributes = ProcessAttributes (nav, assembly);
 			if (attributes.Any ())
 				Context.CustomAttributes.AddCustomAttributes (assembly, attributes);
-			ProcessTypes (assembly, iterator, warnOnUnresolvedTypes);
+			ProcessTypes (assembly, nav, warnOnUnresolvedTypes);
 		}
 
 		protected override void ProcessType (TypeDefinition type, XPathNavigator nav)

src/tools/illink/src/linker/Linker.Steps/ProcessLinkerXmlStepBase.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Globalization;
 using System.Linq;
 using System.Text.RegularExpressions;
@@ -7,6 +7,14 @@
 
 namespace Mono.Linker.Steps
 {
+	[Flags]
+	public enum AllowedAssemblies
+	{
+		ContainingAssembly = 0x1,
+		AnyAssembly = 0x2 | ContainingAssembly,
+		AllAssemblies = 0x4 | AnyAssembly
+	}
+
 	public abstract class ProcessLinkerXmlStepBase : LoadReferencesStep
 	{
 		const string FullNameAttributeName = "fullname";
@@ -25,7 +33,7 @@ public abstract class ProcessLinkerXmlStepBase : LoadReferencesStep
 		protected readonly string _xmlDocumentLocation;
 		readonly XPathDocument _document;
 		readonly EmbeddedResource _resource;
-		readonly AssemblyDefinition _resourceAssembly;
+		protected readonly AssemblyDefinition _resourceAssembly;
 
 		protected ProcessLinkerXmlStepBase (XPathDocument document, string xmlDocumentLocation)
 		{
@@ -44,6 +52,9 @@ protected ProcessLinkerXmlStepBase (XPathDocument document, EmbeddedResource res
 
 		protected virtual void ProcessXml (bool stripResource, bool ignoreResource)
 		{
+			if (!AllowedAssemblySelector.HasFlag (AllowedAssemblies.AnyAssembly) && _resourceAssembly == null)
+				throw new InvalidOperationException ("The containing assembly must be specified for XML which is restricted to modifying that assembly only.");
+
 			try {
 				XPathNavigator nav = _document.CreateNavigator ();
 
@@ -63,48 +74,65 @@ protected virtual void ProcessXml (bool stripResource, bool ignoreResource)
 
 				ProcessAssemblies (nav.SelectChildren ("assembly", ""));
 
+				// For embedded XML, allow not specifying the assembly explicitly in XML.
+				if (_resourceAssembly != null)
+					ProcessAssembly (_resourceAssembly, nav, warnOnUnresolvedTypes: true);
+
 			} catch (Exception ex) when (!(ex is LinkerFatalErrorException)) {
 				throw new LinkerFatalErrorException (MessageContainer.CreateErrorMessage ($"Error processing '{_xmlDocumentLocation}'", 1013), ex);
 			}
 		}
 
-		protected virtual bool AllowAllAssembliesSelector { get => false; }
+		protected virtual AllowedAssemblies AllowedAssemblySelector { get => _resourceAssembly != null ? AllowedAssemblies.ContainingAssembly : AllowedAssemblies.AnyAssembly; }
 
 		protected virtual void ProcessAssemblies (XPathNodeIterator iterator)
 		{
 			while (iterator.MoveNext ()) {
-				bool processAllAssemblies = AllowAllAssembliesSelector && GetFullName (iterator.Current) == AllAssembliesFullName;
+				bool processAllAssemblies = GetFullName (iterator.Current) == AllAssembliesFullName;
+				if (processAllAssemblies && AllowedAssemblySelector != AllowedAssemblies.AllAssemblies) {
+					Context.LogWarning ($"XML contains unsupported wildcard for assembly \"fullname\" attribute", 2100, _xmlDocumentLocation);
+					continue;
+				}
 
 				// Errors for invalid assembly names should show up even if this element will be
 				// skipped due to feature conditions.
 				var name = processAllAssemblies ? null : GetAssemblyName (iterator.Current);
 
+				AssemblyDefinition assemblyToProcess = null;
+				if (!AllowedAssemblySelector.HasFlag (AllowedAssemblies.AnyAssembly)) {
+					if (_resourceAssembly.Name.Name != name.Name) {
+						Context.LogWarning ($"Embedded XML in assembly '{_resourceAssembly.Name.Name}' contains assembly \"fullname\" attribute for another assembly '{name}'", 2101, _xmlDocumentLocation);
+						continue;
+					}
+					assemblyToProcess = _resourceAssembly;
+				}
+
 				if (!ShouldProcessElement (iterator.Current))
 					continue;
 
 				if (processAllAssemblies) {
 					foreach (AssemblyDefinition assembly in Context.GetAssemblies ())
-						ProcessAssembly (assembly, iterator, warnOnUnresolvedTypes: false);
+						ProcessAssembly (assembly, iterator.Current, warnOnUnresolvedTypes: false);
 				} else {
-					AssemblyDefinition assembly = GetAssembly (Context, name);
+					AssemblyDefinition assembly = assemblyToProcess ?? GetAssembly (Context, name);
 
 					if (assembly == null) {
 						Context.LogWarning ($"Could not resolve assembly '{name.Name}'", 2007, _xmlDocumentLocation);
 						continue;
 					}
 
-					ProcessAssembly (assembly, iterator, warnOnUnresolvedTypes: true);
+					ProcessAssembly (assembly, iterator.Current, warnOnUnresolvedTypes: true);
 				}
 			}
 		}
 
-		protected abstract void ProcessAssembly (AssemblyDefinition assembly, XPathNodeIterator iterator, bool warnOnUnresolvedTypes);
+		protected abstract void ProcessAssembly (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes);
 
-		protected virtual void ProcessTypes (AssemblyDefinition assembly, XPathNodeIterator iterator, bool warnOnUnresolvedTypes)
+		protected virtual void ProcessTypes (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes)
 		{
-			iterator = iterator.Current.SelectChildren (TypeElementName, XmlNamespace);
+			var iterator = nav.SelectChildren (TypeElementName, XmlNamespace);
 			while (iterator.MoveNext ()) {
-				XPathNavigator nav = iterator.Current;
+				nav = iterator.Current;
 
 				if (!ShouldProcessElement (nav))
 					continue;

src/tools/illink/src/linker/Linker.Steps/ResolveFromXmlStep.cs

@@ -67,25 +67,27 @@ protected override void Process ()
 			ProcessXml (Context.StripDescriptors, Context.IgnoreDescriptors);
 		}
 
-		protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNodeIterator iterator, bool warnOnUnresolvedTypes)
+		protected override AllowedAssemblies AllowedAssemblySelector { get => AllowedAssemblies.AnyAssembly; }
+
+		protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes)
 		{
 #if !FEATURE_ILLINK
-			if (IsExcluded (iterator.Current))
+			if (IsExcluded (nav))
 				return;
 #endif
 
-			if (GetTypePreserve (iterator.Current) == TypePreserve.All) {
+			if (GetTypePreserve (nav) == TypePreserve.All) {
 				foreach (var type in assembly.MainModule.Types)
 					MarkAndPreserveAll (type);
 			} else {
-				ProcessTypes (assembly, iterator, warnOnUnresolvedTypes);
-				ProcessNamespaces (assembly, iterator);
+				ProcessTypes (assembly, nav, warnOnUnresolvedTypes);
+				ProcessNamespaces (assembly, nav);
 			}
 		}
 
-		void ProcessNamespaces (AssemblyDefinition assembly, XPathNodeIterator iterator)
+		void ProcessNamespaces (AssemblyDefinition assembly, XPathNavigator nav)
 		{
-			iterator = iterator.Current.SelectChildren (NamespaceElementName, XmlNamespace);
+			var iterator = nav.SelectChildren (NamespaceElementName, XmlNamespace);
 			while (iterator.MoveNext ()) {
 				if (!ShouldProcessElement (iterator.Current))
 					continue;

src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Metadata/SetupCompileAfterAttribute.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 
 namespace Mono.Linker.Tests.Cases.Expectations.Metadata
 {
@@ -8,13 +8,26 @@ namespace Mono.Linker.Tests.Cases.Expectations.Metadata
 	[AttributeUsage (AttributeTargets.Class, AllowMultiple = true)]
 	public class SetupCompileAfterAttribute : BaseMetadataAttribute
 	{
-		public SetupCompileAfterAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, string[] resources = null, string additionalArguments = null, string compilerToUse = null)
+		public SetupCompileAfterAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, object[] resources = null, string additionalArguments = null, string compilerToUse = null)
 		{
 			if (sourceFiles == null)
 				throw new ArgumentNullException (nameof (sourceFiles));
 
 			if (string.IsNullOrEmpty (outputName))
 				throw new ArgumentException ("Value cannot be null or empty.", nameof (outputName));
+
+			if (resources != null) {
+				foreach (var res in resources) {
+					if (res is string)
+						continue;
+					if (res is string[] stringArray) {
+						if (stringArray.Length != 2)
+							throw new ArgumentException ("Entry in object[] cannot be a string[] unless it has exactly two elements, for the resource path and name", nameof (resources));
+						continue;
+					}
+					throw new ArgumentException ("Each value in the object[] must be a string or a string[], either a resource path, or a path and name", nameof (resources));
+				}
+			}
 		}
 	}
 }

src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Metadata/SetupCompileBeforeAttribute.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 
 namespace Mono.Linker.Tests.Cases.Expectations.Metadata
 {
@@ -8,22 +8,48 @@ namespace Mono.Linker.Tests.Cases.Expectations.Metadata
 	[AttributeUsage (AttributeTargets.Class, AllowMultiple = true)]
 	public class SetupCompileBeforeAttribute : BaseMetadataAttribute
 	{
-		public SetupCompileBeforeAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, string[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false)
+		public SetupCompileBeforeAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, object[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false)
 		{
 			if (sourceFiles == null)
 				throw new ArgumentNullException (nameof (sourceFiles));
 
 			if (string.IsNullOrEmpty (outputName))
 				throw new ArgumentException ("Value cannot be null or empty.", nameof (outputName));
+
+			if (resources != null) {
+				foreach (var res in resources) {
+					if (res is string)
+						continue;
+					if (res is string[] stringArray) {
+						if (stringArray.Length != 2)
+							throw new ArgumentException ("Entry in object[] cannot be a string[] unless it has exactly two elements, for the resource path and name", nameof (resources));
+						continue;
+					}
+					throw new ArgumentException ("Each value in the object[] must be a string or a string[], either a resource path, or a path and name", nameof (resources));
+				}
+			}
 		}
 
-		public SetupCompileBeforeAttribute (string outputName, Type[] typesToIncludeSourceFor, string[] references = null, string[] defines = null, string[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false)
+		public SetupCompileBeforeAttribute (string outputName, Type[] typesToIncludeSourceFor, string[] references = null, string[] defines = null, object[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false)
 		{
 			if (typesToIncludeSourceFor == null)
 				throw new ArgumentNullException (nameof (typesToIncludeSourceFor));
 
 			if (string.IsNullOrEmpty (outputName))
 				throw new ArgumentException ("Value cannot be null or empty.", nameof (outputName));
+
+			if (resources != null) {
+				foreach (var res in resources) {
+					if (res is string)
+						continue;
+					if (res is string[] stringArray) {
+						if (stringArray.Length != 2)
+							throw new ArgumentException ("Entry in object[] cannot be a string[] unless it has exactly two elements, for the resource path and name", nameof (resources));
+						continue;
+					}
+					throw new ArgumentException ("Each value in the object[] must be a string or a string[], either a resource path, or a path and name", nameof (resources));
+				}
+			}
 		}
 	}
 }

src/tools/illink/test/Mono.Linker.Tests.Cases/DynamicDependencies/DynamicDependencyMethodInNonReferencedAssemblyWithEmbeddedXml.cs

@@ -1,4 +1,4 @@
-using System.Diagnostics.CodeAnalysis;
+using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
 using Mono.Linker.Tests.Cases.DynamicDependencies.Dependencies;
 using Mono.Linker.Tests.Cases.Expectations.Assertions;
@@ -15,7 +15,7 @@ namespace Mono.Linker.Tests.Cases.DynamicDependencies
 		"DynamicDependencyMethodInNonReferencedAssemblyLibrary.dll",
 		new[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.cs" },
 		references: new[] { "base.dll" },
-		resources: new[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.xml" },
+		resources: new object[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.xml" },
 		addAsReference: false)]
 	[KeptAssembly ("base.dll")]
 	[RemovedMemberInAssembly ("DynamicDependencyMethodInNonReferencedAssemblyLibrary.dll", "Mono.Linker.Tests.Cases.DynamicDependencies.Dependencies.DynamicDependencyMethodInNonReferencedAssemblyLibrary", "UnusedMethod()")]

src/tools/illink/test/Mono.Linker.Tests.Cases/DynamicDependencies/DynamicDependencyOnUnusedMethodInNonReferencedAssemblyWithEmbeddedXml.cs

@@ -1,4 +1,4 @@
-using System.Diagnostics.CodeAnalysis;
+using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
 using Mono.Linker.Tests.Cases.DynamicDependencies.Dependencies;
 using Mono.Linker.Tests.Cases.Expectations.Assertions;
@@ -15,7 +15,7 @@ namespace Mono.Linker.Tests.Cases.DynamicDependencies
 		"DynamicDependencyMethodInNonReferencedAssemblyLibrary.dll",
 		new[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.cs" },
 		references: new[] { "base.dll" },
-		resources: new[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.xml" },
+		resources: new object[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.xml" },
 		addAsReference: false)]
 	[KeptAssembly ("base.dll")]
 	[RemovedAssembly ("DynamicDependencyMethodInNonReferencedAssemblyLibrary.dll")]

src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/EmbeddedAttributeErrorCases.cs

@@ -0,0 +1,6 @@
+namespace Mono.Linker.Tests.Cases.LinkAttributes.Dependencies
+{
+	public class EmbeddedAttributeErrorCases
+	{
+	}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/EmbeddedAttributeErrorCases.xml

@@ -0,0 +1,10 @@
+<linker>
+  <!-- IL2100 -->
+  <assembly fullname="*" />
+  <!-- IL2101 -->
+  <assembly fullname="test">
+    <type fullname="Mono.Linker.Tests.Cases.LinkAttributes.LinkAttributeErrorCases/ReferencedFromOtherAssembly">
+      <attribute fullname="Mono.Linker.Tests.Cases.LinkAttributes.LinkAttributeErrorCases/FirstAttribute" />
+    </type>
+  </assembly>
+</linker>

src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/MockCorelib.cs

@@ -0,0 +1,14 @@
+#if INCLUDE_MOCK_CORELIB
+
+using System;
+
+[assembly: MockCorelibAttributeToRemove]
+
+namespace System
+{
+	public class MockCorelibAttributeToRemove : Attribute
+	{
+	}
+}
+
+#endif

src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/MockCorelib.xml

@@ -0,0 +1,7 @@
+<linker>
+  <assembly fullname="*">
+    <type fullname="System.MockCorelibAttributeToRemove">
+      <attribute internal="RemoveAttributeInstances" />
+    </type>
+  </assembly>
+</linker>