Improve MotW handling and clarify the MSB3821 (#14015)

Author: JanKrivanek

documentation/wiki/ChangeWaves.md

@@ -29,6 +29,9 @@ Change wave checks around features will be removed in the release that accompani
 
 ## Current Rotation of Change Waves
 
+### 18.9
+- [GenerateResource: typed ResX data/metadata entries in Mark-of-the-Web files are now treated as untrusted and blocked with MSB3821; unblock the file (or set MSBUILDDISABLEFEATURESFROMVERSION=18.9) to restore prior behavior. ResXFileRef entries are always blocked regardless of this wave.](https://github.com/dotnet/msbuild/pull/14015)
+
 ### 18.8
 - [RAR task: across multiple input properties, resolve relative paths against the project directory (not the process current directory)](https://github.com/dotnet/msbuild/pull/13319)
 - [Console, parallel console, and terminal loggers print the paths of log files written by registered loggers (e.g. file logger and binary logger) as part of the end-of-build summary.](https://github.com/dotnet/msbuild/pull/13577)

src/Framework/ChangeWaves.cs

@@ -36,7 +36,8 @@ internal static class ChangeWaves
         internal static readonly Version Wave18_6 = new Version(18, 6);
         internal static readonly Version Wave18_7 = new Version(18, 7);
         internal static readonly Version Wave18_8 = new Version(18, 8);
-        internal static readonly Version[] AllWaves = [Wave17_10, Wave17_12, Wave17_14, Wave18_3, Wave18_4, Wave18_5, Wave18_6, Wave18_7, Wave18_8];
+        internal static readonly Version Wave18_9 = new Version(18, 9);
+        internal static readonly Version[] AllWaves = [Wave17_10, Wave17_12, Wave17_14, Wave18_3, Wave18_4, Wave18_5, Wave18_6, Wave18_7, Wave18_8, Wave18_9];
 
         /// <summary>
         /// Special value indicating that all features behind all Change Waves should be enabled.

src/Tasks.UnitTests/ResourceHandling/GenerateResource_Tests.cs

@@ -4620,4 +4620,80 @@ internal bool DoesPathNeedQuotes(string path)
             return base.IsQuotingRequired(path);
         }
     }
+
+    /// <summary>
+    /// Tests for the content-only portion of the Mark-of-the-Web trust check
+    /// (<see cref="GenerateResource.ResourceFileContentsAreDangerous(System.IO.TextReader)"/>).
+    /// These exercise the danger classification independently of the platform-specific zone/COM check,
+    /// so they run on all target frameworks.
+    /// </summary>
+    public sealed class ResourceFileTrustTests
+    {
+        private static bool IsDangerous(string resxContents)
+            => GenerateResource.ResourceFileContentsAreDangerous(new StringReader(resxContents));
+
+        [Fact]
+        public void PlainStringEntryIsNotDangerous()
+        {
+            string resx = @"<root>
+  <data name=""Greeting"" xml:space=""preserve""><value>Hello</value></data>
+</root>";
+
+            IsDangerous(resx).ShouldBeFalse();
+        }
+
+        [Fact]
+        public void MimetypeEntryIsDangerous()
+        {
+            string resx = @"<root>
+  <data name=""Blob"" mimetype=""application/x-microsoft.net.object.bytearray.base64""><value>AAAA</value></data>
+</root>";
+
+            IsDangerous(resx).ShouldBeTrue();
+        }
+
+        [Fact]
+        public void ResXFileRefEntryIsDangerousEvenWhenWave18_9Disabled()
+        {
+            string resx = @"<root>
+  <data name=""Logo"" type=""System.Resources.ResXFileRef, System.Windows.Forms""><value>logo.txt;System.String, mscorlib</value></data>
+</root>";
+
+            // ResXFileRef detection is intentionally unconditional - it is not gated behind a ChangeWave -
+            // because it can trigger reading of an external linked file. Disable Wave18_9 to prove it still fires.
+            using TestEnvironment env = TestEnvironment.Create();
+            env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave18_9.ToString());
+            ChangeWaves.ResetStateForTests();
+
+            IsDangerous(resx).ShouldBeTrue();
+        }
+
+        [Fact]
+        public void TypedEntryIsDangerousWhenWave18_9Enabled()
+        {
+            string resx = @"<root>
+  <data name=""Count"" type=""System.Int32, mscorlib""><value>42</value></data>
+</root>";
+
+            using TestEnvironment env = TestEnvironment.Create();
+            env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", string.Empty);
+            ChangeWaves.ResetStateForTests();
+
+            IsDangerous(resx).ShouldBeTrue();
+        }
+
+        [Fact]
+        public void TypedEntryIsNotDangerousWhenWave18_9Disabled()
+        {
+            string resx = @"<root>
+  <data name=""Count"" type=""System.Int32, mscorlib""><value>42</value></data>
+</root>";
+
+            using TestEnvironment env = TestEnvironment.Create();
+            env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave18_9.ToString());
+            ChangeWaves.ResetStateForTests();
+
+            IsDangerous(resx).ShouldBeFalse();
+        }
+    }
 }

src/Tasks/GenerateResource.cs

@@ -1013,46 +1013,9 @@ private unsafe bool IsDangerous(String filename)
                 String.Equals(extension, ".resw", StringComparison.OrdinalIgnoreCase))
             {
                 // XML files are only dangerous if there are unrecognized objects in them
-                dangerous = false;
-
                 using FileStream stream = new FileStream(filename, FileMode.Open, FileAccess.Read, FileShare.Read);
-                using XmlTextReader reader = new XmlTextReader(stream);
-                reader.DtdProcessing = DtdProcessing.Ignore;
-                reader.XmlResolver = null;
-                try
-                {
-                    while (reader.Read())
-                    {
-                        if (reader.NodeType == XmlNodeType.Element)
-                        {
-                            string s = reader.LocalName;
-
-                            // We only want to parse data nodes,
-                            // the mimetype attribute gives the serializer
-                            // that's requested.
-                            if (reader.LocalName.Equals("data"))
-                            {
-                                if (reader["mimetype"] != null)
-                                {
-                                    dangerous = true;
-                                }
-                            }
-                            else if (reader.LocalName.Equals("metadata"))
-                            {
-                                if (reader["mimetype"] != null)
-                                {
-                                    dangerous = true;
-                                }
-                            }
-                        }
-                    }
-                }
-                catch
-                {
-                    // If we hit an error while parsing assume there's a dangerous type in this file.
-                    dangerous = true;
-                }
-                stream.Close();
+                using StreamReader streamReader = new StreamReader(stream);
+                dangerous = ResourceFileContentsAreDangerous(streamReader);
             }
 
             return dangerous;
@@ -1090,9 +1053,82 @@ private void RecordItemsForDisconnectIfNecessary(IEnumerable<ITaskItem> items)
 #endif // FEATURE_APPDOMAIN
 
         /// <summary>
-        /// Computes the path to ResGen.exe for use in logging and for passing to the
-        /// nested ResGen task.
+        /// Scans the contents of a <c>.resx</c>/<c>.resw</c> file and determines whether it contains any entry
+        /// that requires the file to be trusted before it is processed by <see cref="GenerateResource"/>.
         /// </summary>
+        /// <remarks>
+        /// This is the content-only portion of the Mark-of-the-Web trust check (see <c>IsDangerous</c>). It is kept
+        /// separate (and free of the zone/COM machinery) so it can be unit-tested on all target frameworks.
+        /// A resource is considered dangerous when a <c>data</c>/<c>metadata</c> element:
+        /// <list type="bullet">
+        /// <item>has a <c>mimetype</c> attribute (an arbitrary object will be deserialized), or</item>
+        /// <item>has a <c>type</c> attribute referencing <c>ResXFileRef</c> (an external linked file will be read), or</item>
+        /// <item>has any other <c>type</c> attribute - only when ChangeWave 18.9 is enabled - because resolving the
+        /// type eventually calls <c>Type.GetType</c>, which can probe for assemblies on disk.</item>
+        /// </list>
+        /// </remarks>
+        internal static bool ResourceFileContentsAreDangerous(TextReader textReader)
+        {
+            // XML files are only dangerous if there are unrecognized objects in them.
+            bool dangerous = false;
+
+            using XmlTextReader reader = new XmlTextReader(textReader);
+            reader.DtdProcessing = DtdProcessing.Ignore;
+            reader.XmlResolver = null;
+            try
+            {
+                while (reader.Read())
+                {
+                    if (reader.NodeType == XmlNodeType.Element)
+                    {
+                        string localName = reader.LocalName;
+
+                        // We only want to parse data nodes,
+                        // the mimetype attribute gives the serializer
+                        // that's requested.
+                        if (localName.Equals("data") || localName.Equals("metadata"))
+                        {
+                            if (reader["mimetype"] != null)
+                            {
+                                dangerous = true;
+                            }
+                            else
+                            {
+                                // ResXFileRef can reference external files
+                                string typeAttribute = reader["type"];
+                                if (typeAttribute != null)
+                                {
+                                    if (typeAttribute.IndexOf("ResXFileRef", StringComparison.Ordinal) >= 0)
+                                    {
+                                        dangerous = true;
+                                    }
+                                    else if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_9))
+                                    {
+                                        // Require any typed entry on a "Mark of the web" to be unblocked by user
+                                        dangerous = true;
+                                    }
+                                }
+                            }
+                        }
+                    }
+
+                    // The result is already determined; no need to keep parsing the rest of the file.
+                    if (dangerous)
+                    {
+                        break;
+                    }
+                }
+            }
+            catch
+            {
+                // If we hit an error while parsing assume there's a dangerous type in this file.
+                dangerous = true;
+            }
+
+            return dangerous;
+        }
+
+
         /// <returns>True if the path is found (or it doesn't matter because we're executing in memory), false otherwise</returns>
         private bool ComputePathToResGen()
         {

src/Tasks/Resources/Strings.resx

@@ -1160,7 +1160,7 @@
     <comment>{StrBegin="MSB3820: "}</comment>
   </data>
   <data name="GenerateResource.MOTW">
-    <value>MSB3821: Couldn't process file {0} due to its being in the Internet or Restricted zone or having the mark of the web on the file. Remove the mark of the web if you want to process these files.</value>
+    <value>MSB3821: Couldn't process file {0} due to its being in the Internet or Restricted zone or having the mark of the web on the file. Remove the mark of the web if you trust these files and want to process them. Please follow MSBuild secure usage best practices - https://aka.ms/msbuild-security-documentation</value>
     <comment>{StrBegin="MSB3821: "} "Internet zone", "Restricted zone", and "mark of the web" are Windows concepts that may have a specific translation.</comment>
   </data>
   <data name="GenerateResource.PreserializedResourcesRequiresExtensions">