Merge pull request #3068 from mRemoteNG/copilot/fix-sql-injection-issue

Fix WQL injection vulnerability in InstalledWindowsUpdateChecker

mRemoteNGInstaller/CustomActions/InstalledWindowsUpdateChecker.cs

@@ -2,12 +2,14 @@
 using System.Management;
 using System.Collections;
 using System.Collections.Generic;
+using System.Text.RegularExpressions;
 
 namespace CustomActions
 {
     public class InstalledWindowsUpdateChecker
     {
         private readonly ManagementScope _managementScope;
+        private static readonly Regex KbPattern = new Regex(@"^(KB)?\d+$", RegexOptions.IgnoreCase | RegexOptions.Compiled);
 
         public InstalledWindowsUpdateChecker()
         {
@@ -61,12 +63,45 @@ namespace CustomActions
             var counter = 0;
             foreach (var kb in kbList)
             {
+                var sanitizedKb = SanitizeKbId(kb);
+                if (string.IsNullOrEmpty(sanitizedKb))
+                    continue; // Skip invalid KB IDs
+                
                 if (counter > 0)
                     whereClause += " OR ";
-                whereClause += $"HotFixID='{kb}'";
+                whereClause += $"HotFixID='{sanitizedKb}'";
                 counter++;
             }
             return whereClause;
         }
+
+        /// <summary>
+        /// Sanitizes a KB ID to prevent WQL injection attacks.
+        /// KB IDs must match the pattern: optional "KB" prefix followed by digits,
+        /// or just digits. Any other characters are rejected.
+        /// </summary>
+        /// <param name="kbId">The KB ID to sanitize</param>
+        /// <returns>The sanitized KB ID, or empty string if invalid</returns>
+        private string SanitizeKbId(string kbId)
+        {
+            if (string.IsNullOrWhiteSpace(kbId))
+                return string.Empty;
+
+            // KB IDs should match the pattern: optional KB prefix followed by digits
+            // (e.g., KB1234567 or 1234567)
+            // Trim whitespace and check if it matches the expected pattern
+            var trimmedKb = kbId.Trim();
+            if (!KbPattern.IsMatch(trimmedKb))
+                return string.Empty;
+
+            // Normalize to uppercase
+            var normalizedKb = trimmedKb.ToUpperInvariant();
+            
+            // Ensure KB prefix is present (Win32_QuickFixEngineering always uses the KB prefix)
+            if (!normalizedKb.StartsWith("KB"))
+                normalizedKb = "KB" + normalizedKb;
+
+            return normalizedKb;
+        }
     }
 }

mRemoteNGTests/Installer/InstalledWindowsUpdateCheckerTests.cs

@@ -0,0 +1,208 @@
+using NUnit.Framework;
+using System;
+using System.Reflection;
+
+namespace mRemoteNGTests.Installer
+{
+    [TestFixture]
+    public class InstalledWindowsUpdateCheckerTests
+    {
+        private CustomActions.InstalledWindowsUpdateChecker _checker;
+        private MethodInfo _sanitizeKbIdMethod;
+        private MethodInfo _buildWhereClauseMethod;
+
+        [SetUp]
+        public void Setup()
+        {
+            _checker = new CustomActions.InstalledWindowsUpdateChecker();
+            
+            // Use reflection to access private methods for testing
+            var type = typeof(CustomActions.InstalledWindowsUpdateChecker);
+            _sanitizeKbIdMethod = type.GetMethod("SanitizeKbId", BindingFlags.NonPublic | BindingFlags.Instance);
+            _buildWhereClauseMethod = type.GetMethod("BuildWhereClauseFromKbList", BindingFlags.NonPublic | BindingFlags.Instance);
+        }
+
+        #region SanitizeKbId Tests
+
+        [Test]
+        public void SanitizeKbId_ValidKbWithPrefix_ReturnsUppercased()
+        {
+            var result = InvokeSanitizeKbId("KB1234567");
+            Assert.That(result, Is.EqualTo("KB1234567"));
+        }
+
+        [Test]
+        public void SanitizeKbId_ValidKbLowercase_ReturnsUppercased()
+        {
+            var result = InvokeSanitizeKbId("kb1234567");
+            Assert.That(result, Is.EqualTo("KB1234567"));
+        }
+
+        [Test]
+        public void SanitizeKbId_ValidKbMixedCase_ReturnsUppercased()
+        {
+            var result = InvokeSanitizeKbId("Kb1234567");
+            Assert.That(result, Is.EqualTo("KB1234567"));
+        }
+
+        [Test]
+        public void SanitizeKbId_ValidNumberOnly_ReturnsWithKbPrefix()
+        {
+            var result = InvokeSanitizeKbId("1234567");
+            Assert.That(result, Is.EqualTo("KB1234567"));
+        }
+
+        [Test]
+        public void SanitizeKbId_WithWhitespace_ReturnsTrimmedAndUppercased()
+        {
+            var result = InvokeSanitizeKbId("  KB1234567  ");
+            Assert.That(result, Is.EqualTo("KB1234567"));
+        }
+
+        [Test]
+        public void SanitizeKbId_SqlInjectionAttempt_ReturnsEmpty()
+        {
+            var result = InvokeSanitizeKbId("KB1234' OR '1'='1");
+            Assert.That(result, Is.Empty);
+        }
+
+        [Test]
+        public void SanitizeKbId_WqlInjectionWithSemicolon_ReturnsEmpty()
+        {
+            var result = InvokeSanitizeKbId("KB1234; DROP TABLE");
+            Assert.That(result, Is.Empty);
+        }
+
+        [Test]
+        public void SanitizeKbId_WithSpecialCharacters_ReturnsEmpty()
+        {
+            var result = InvokeSanitizeKbId("KB1234@#$");
+            Assert.That(result, Is.Empty);
+        }
+
+        [Test]
+        public void SanitizeKbId_NullInput_ReturnsEmpty()
+        {
+            var result = InvokeSanitizeKbId(null);
+            Assert.That(result, Is.Empty);
+        }
+
+        [Test]
+        public void SanitizeKbId_EmptyString_ReturnsEmpty()
+        {
+            var result = InvokeSanitizeKbId("");
+            Assert.That(result, Is.Empty);
+        }
+
+        [Test]
+        public void SanitizeKbId_WhitespaceOnly_ReturnsEmpty()
+        {
+            var result = InvokeSanitizeKbId("   ");
+            Assert.That(result, Is.Empty);
+        }
+
+        [Test]
+        public void SanitizeKbId_OnlyKbPrefix_ReturnsEmpty()
+        {
+            var result = InvokeSanitizeKbId("KB");
+            Assert.That(result, Is.Empty);
+        }
+
+        [Test]
+        public void SanitizeKbId_WithDashes_ReturnsEmpty()
+        {
+            var result = InvokeSanitizeKbId("KB-1234567");
+            Assert.That(result, Is.Empty);
+        }
+
+        [Test]
+        public void SanitizeKbId_WithUnderscores_ReturnsEmpty()
+        {
+            var result = InvokeSanitizeKbId("KB_1234567");
+            Assert.That(result, Is.Empty);
+        }
+
+        #endregion
+
+        #region BuildWhereClauseFromKbList Tests
+
+        [Test]
+        public void BuildWhereClause_SingleValidKb_ReturnsCorrectClause()
+        {
+            var result = InvokeBuildWhereClause(new[] { "KB1234567" });
+            Assert.That(result, Is.EqualTo("HotFixID='KB1234567'"));
+        }
+
+        [Test]
+        public void BuildWhereClause_MultipleValidKbs_ReturnsOrClause()
+        {
+            var result = InvokeBuildWhereClause(new[] { "KB1234567", "KB7654321" });
+            Assert.That(result, Is.EqualTo("HotFixID='KB1234567' OR HotFixID='KB7654321'"));
+        }
+
+        [Test]
+        public void BuildWhereClause_InvalidKb_SkipsInvalid()
+        {
+            var result = InvokeBuildWhereClause(new[] { "KB1234567", "KB1234'; DROP--", "KB7654321" });
+            Assert.That(result, Is.EqualTo("HotFixID='KB1234567' OR HotFixID='KB7654321'"));
+        }
+
+        [Test]
+        public void BuildWhereClause_AllInvalidKbs_ReturnsEmpty()
+        {
+            var result = InvokeBuildWhereClause(new[] { "'; DROP TABLE", "OR 1=1--" });
+            Assert.That(result, Is.Empty);
+        }
+
+        [Test]
+        public void BuildWhereClause_EmptyList_ReturnsEmpty()
+        {
+            var result = InvokeBuildWhereClause(Array.Empty<string>());
+            Assert.That(result, Is.Empty);
+        }
+
+        [Test]
+        public void BuildWhereClause_NullValues_SkipsNulls()
+        {
+            var result = InvokeBuildWhereClause(new[] { "KB1234567", null, "KB7654321" });
+            Assert.That(result, Is.EqualTo("HotFixID='KB1234567' OR HotFixID='KB7654321'"));
+        }
+
+        [Test]
+        public void BuildWhereClause_MixedCaseKbs_NormalizesToUppercase()
+        {
+            var result = InvokeBuildWhereClause(new[] { "kb1234567", "KB7654321" });
+            Assert.That(result, Is.EqualTo("HotFixID='KB1234567' OR HotFixID='KB7654321'"));
+        }
+
+        [Test]
+        public void BuildWhereClause_DigitOnlyKb_AddsKbPrefix()
+        {
+            var result = InvokeBuildWhereClause(new[] { "1234567" });
+            Assert.That(result, Is.EqualTo("HotFixID='KB1234567'"));
+        }
+
+        [Test]
+        public void BuildWhereClause_MixedPrefixedAndDigitOnly_NormalizesAll()
+        {
+            var result = InvokeBuildWhereClause(new[] { "1234567", "KB7654321", "kb9999999" });
+            Assert.That(result, Is.EqualTo("HotFixID='KB1234567' OR HotFixID='KB7654321' OR HotFixID='KB9999999'"));
+        }
+
+        #endregion
+
+        #region Helper Methods
+
+        private string InvokeSanitizeKbId(string input)
+        {
+            return (string)_sanitizeKbIdMethod.Invoke(_checker, new object[] { input });
+        }
+
+        private string InvokeBuildWhereClause(string[] kbList)
+        {
+            return (string)_buildWhereClauseMethod.Invoke(_checker, new object[] { kbList });
+        }
+
+        #endregion
+    }
+}

mRemoteNGTests/mRemoteNGTests.csproj

@@ -69,6 +69,7 @@
   </ItemGroup>
   <ItemGroup>
     <ProjectReference Include="..\mRemoteNG\mRemoteNG.csproj" />
+    <ProjectReference Include="..\mRemoteNGInstaller\CustomActions\CustomActions.csproj" />
   </ItemGroup>
   <ItemGroup>
     <Compile Update="Properties\Resources.Designer.cs">