Fix certificate handling and permissions issues across multiple commands (#5433)

* Fix certificate handling and permissions issues across multiple commands

* Fix certificate handling and permissions issues in various commands

* Fix key container name retrieval and improve file permission handling in certificate operations

* Fix key container name retrieval and ensure proper disposal of RSA private keys

Author: gautamdsheth

CHANGELOG.md

@@ -27,6 +27,11 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Changed `Export-PnPFlow -AsZipPackage` and `Export-PnPPowerApp` to ask for confirmation before overwriting an existing file when `-OutPath` is omitted, as they already did when `-OutPath` is specified. Unattended scripts that rely on the previous silent overwrite need to specify `-Force`. [#5421](https://github.com/pnp/powershell/pull/5421)
 
 ### Fixed
+- Fixed `Disconnect-PnPOnline` never removing the private key container which loading a certificate leaves behind, so every certificate based connect permanently added a private key file to the profile of the user. Certificates are loaded with `X509KeyStorageFlags.UserKeySet`, which places the key container under `%APPDATA%\Microsoft\Crypto`, while only the machine wide `%ProgramData%\Microsoft\Crypto\RSA\MachineKeys` was looked at, so the removal silently found nothing to do. Both locations are now checked, `Connect-PnPOnline -CertificateBase64Encoded` takes part in the cleanup as `-CertificatePath` already did, and a removal which does not succeed is written to the debug log rather than being swallowed. [#5433](https://github.com/pnp/powershell/pull/5433)
+- Fixed `New-PnPAzureCertificate -OutPfx` and `Register-PnPEntraIDApp -OutPath` writing the PFX holding the private key using the default permissions of the process on Linux and macOS, which the usual umask leaves readable by every local account. The file is now created readable and writable by its owner only. The accompanying CER file holds no private key and keeps the default permissions. [#5433](https://github.com/pnp/powershell/pull/5433)
+- Fixed `Connect-PnPOnline -CertificatePath` keeping the certificate file open for as long as the PowerShell session lived, which prevented the file from being moved, replaced or deleted and leaked a file handle on every connect. The same read could also return fewer bytes than the file holds and have the resulting truncated certificate reported as being corrupt or password protected. [#5433](https://github.com/pnp/powershell/pull/5433)
+- Fixed `Get-PnPTenantDeletedSite` failing to fetch the additional details of a site whose url contains an ampersand, as the url was placed into the CAML query without being escaped. [#5433](https://github.com/pnp/powershell/pull/5433)
+- Fixed `Get-PnPListItemAttachment` combining the file name returned by the server with `-Path` without reducing it to its file name part first, which meant a rooted value would have silently replaced the path that was asked for. [#5433](https://github.com/pnp/powershell/pull/5433)
 - Fixed delegated and application permission metadata and documentation for `Add-PnPPlannerBucket`, `Add-PnPPlannerRoster`, `Add-PnPPlannerRosterMember`, `Add-PnPPlannerTask`, `Get-PnPPlannerBucket`, `Get-PnPPlannerPlan`, `Get-PnPPlannerRosterMember`, `Get-PnPPlannerRosterPlan`, `Get-PnPPlannerTask`, `New-PnPPlannerPlan`, `Remove-PnPPlannerBucket`, `Remove-PnPPlannerPlan`, `Remove-PnPPlannerRoster`, `Remove-PnPPlannerRosterMember`, `Remove-PnPPlannerTask`, `Set-PnPPlannerBucket`, `Set-PnPPlannerPlan`, and `Set-PnPPlannerTask`. [#5432](https://github.com/pnp/powershell/pull/5432)
 - Fixed delegated and application permission metadata and documentation for `Add-PnPTodoTaskFileAttachment`, `Get-PnPTodoList`, `Get-PnPTodoTask`, `Get-PnPTodoTaskChecklistItem`, `Get-PnPTodoTaskFileAttachment`, `Get-PnPTodoTaskLinkedResource`, `New-PnPTodoList`, `New-PnPTodoTask`, `New-PnPTodoTaskChecklistItem`, `New-PnPTodoTaskLinkedResource`, `Remove-PnPTodoList`, `Remove-PnPTodoTask`, `Remove-PnPTodoTaskChecklistItem`, `Remove-PnPTodoTaskFileAttachment`, `Remove-PnPTodoTaskLinkedResource`, `Update-PnPTodoList`, `Update-PnPTodoTask`, and `Update-PnPTodoTaskChecklistItem`. [#5432](https://github.com/pnp/powershell/pull/5432)
 - Changed Microsoft To Do cmdlets to target the user supplied through `-User` directly, avoiding an additional Microsoft Graph user lookup and correctly encoding guest user principal names. Invalid user identifiers now surface the Microsoft Graph error returned by the To Do endpoint instead of being silently ignored. [#5432](https://github.com/pnp/powershell/pull/5432)

src/Commands/Base/ConnectOnline.cs

@@ -650,7 +650,8 @@ private PnPConnection ConnectAppOnlyWithCertificate()
                 {
                     ReuseAuthenticationManager();
                 }
-                return PnPConnection.CreateWithCert(new Uri(Url), ClientId, Tenant, TenantAdminUrl, AzureEnvironment, certificate);
+                // The key container behind this certificate was created by loading the bytes above, so it is ours to remove again on disconnect
+                return PnPConnection.CreateWithCert(new Uri(Url), ClientId, Tenant, TenantAdminUrl, AzureEnvironment, certificate, true);
             }
             else if (ParameterSpecified(nameof(Thumbprint)))
             {

src/Commands/Base/NewAzureCertificate.cs

@@ -75,7 +75,7 @@ protected override void ProcessRecord()
                     OutPfx = Path.Combine(SessionState.Path.CurrentFileSystemLocation.Path, OutPfx);
                 }
                 byte[] certData = certificate.Export(X509ContentType.Pfx, CertificatePassword);
-                File.WriteAllBytes(OutPfx, certData);
+                CertificateHelper.WritePrivateKeyFile(OutPfx, certData);
             }
 
             if (!string.IsNullOrWhiteSpace(OutCert))

src/Commands/Base/PnPConnection.cs

@@ -1052,38 +1052,83 @@ internal static void CleanupCryptoMachineKey(X509Certificate2 certificate)
 
             if (Utilities.OperatingSystem.IsWindows())
             {
-                var privateKey = (certificate.GetRSAPrivateKey() as RSACng)?.Key;
-                // var privateKey = (certificate.PrivateKey as RSACng)?.Key;
-                if (privateKey == null)
+                // The private key is asked for once and then offered to both providers. Returning early when it is not a Cryptography Next Generation
+                // key would make the legacy cryptographic service provider branch below unreachable, which is what used to leave the key containers
+                // of such a certificate behind. It is a caller owned object, so it is disposed again right away rather than being left to the garbage
+                // collector, as the handle it holds on the key would otherwise still be open while the file behind it is deleted further down.
+                string uniqueKeyContainerName;
+                using (var rsaPrivateKey = certificate.GetRSAPrivateKey())
+                {
+#pragma warning disable CA1416 // Validate platform compatibility, this whole block only runs on Windows
+                    // Both of these are the name of the file the key lives in. For the legacy providers that is UniqueKeyContainerName, where
+                    // KeyContainerName is only the logical name of the container and never matches anything on disk.
+                    uniqueKeyContainerName = (rsaPrivateKey as RSACng)?.Key?.UniqueName
+                        ?? (rsaPrivateKey as RSACryptoServiceProvider)?.CspKeyContainerInfo?.UniqueKeyContainerName;
+#pragma warning restore CA1416 // Validate platform compatibility
+                }
+
+                if (string.IsNullOrEmpty(uniqueKeyContainerName))
+                {
+                    Log.Debug("PnPConnection", "Unable to remove the private key of the certificate because its key container name could not be determined.");
                     return;
+                }
 
-                string uniqueKeyContainerName = privateKey.UniqueName;
-                if (uniqueKeyContainerName == null)
+                certificate.Reset();
+
+                // Certificates are loaded using X509KeyStorageFlags.UserKeySet, which puts the key container in the profile of the current user
+                // rather than in the machine wide store. Only the machine wide store used to be looked at here, so nothing was ever removed. The
+                // machine wide path is still checked last for a certificate which was loaded with MachineKeySet instead.
+                var candidatePaths = new List<string>();
+
+                var appDataPath = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData);
+                if (!string.IsNullOrEmpty(appDataPath))
                 {
-#pragma warning disable CA1416 // Validate platform compatibilit
-                    RSACryptoServiceProvider rsaCSP = certificate.GetRSAPrivateKey() as RSACryptoServiceProvider;
-                    uniqueKeyContainerName = rsaCSP.CspKeyContainerInfo.KeyContainerName;
+                    // Where a key created through Cryptography Next Generation ends up, which is what a modern certificate uses
+                    candidatePaths.Add(Path.Combine(appDataPath, "Microsoft", "Crypto", "Keys", uniqueKeyContainerName));
+
+                    // Where a key created through the legacy cryptographic service providers ends up
+#pragma warning disable CA1416 // Validate platform compatibility, this whole block only runs on Windows
+                    var currentUserSid = System.Security.Principal.WindowsIdentity.GetCurrent()?.User?.Value;
 #pragma warning restore CA1416 // Validate platform compatibility
+                    if (!string.IsNullOrEmpty(currentUserSid))
+                    {
+                        candidatePaths.Add(Path.Combine(appDataPath, "Microsoft", "Crypto", "RSA", currentUserSid, uniqueKeyContainerName));
+                    }
                 }
-                certificate.Reset();
 
+                // A certificate loaded with X509KeyStorageFlags.MachineKeySet, which -X509KeyStorageFlags accepts and the documentation of
+                // Connect-PnPOnline shows, ends up machine wide instead of in the profile of the user
                 var programDataPath = Environment.GetEnvironmentVariable("ProgramData");
                 if (string.IsNullOrEmpty(programDataPath))
                 {
                     programDataPath = @"C:\ProgramData";
                 }
-                try
+
+                // Where a machine wide key created through Cryptography Next Generation ends up
+                candidatePaths.Add(Path.Combine(programDataPath, "Microsoft", "Crypto", "Keys", uniqueKeyContainerName));
+
+                // Where a machine wide key created through the legacy cryptographic service providers ends up
+                candidatePaths.Add(Path.Combine(programDataPath, "Microsoft", "Crypto", "RSA", "MachineKeys", uniqueKeyContainerName));
+
+                foreach (var candidatePath in candidatePaths)
                 {
-                    var temporaryCertificateFilePath = $@"{programDataPath}\Microsoft\Crypto\RSA\MachineKeys\{uniqueKeyContainerName}";
-                    if (System.IO.File.Exists(temporaryCertificateFilePath))
+                    try
+                    {
+                        if (System.IO.File.Exists(candidatePath))
+                        {
+                            System.IO.File.Delete(candidatePath);
+                            Log.Debug("PnPConnection", $"Removed the private key container of the certificate at '{candidatePath}'.");
+                            return;
+                        }
+                    }
+                    catch (Exception e)
                     {
-                        System.IO.File.Delete(temporaryCertificateFilePath);
+                        // Best effort cleanup, but no longer silent so that a failure to remove the key can be diagnosed
+                        Log.Debug("PnPConnection", $"Unable to remove the private key container of the certificate at '{candidatePath}': {e.Message}");
                     }
                 }
-                catch (Exception)
-                {
-                    // best effort cleanup
-                }
+
+                Log.Debug("PnPConnection", $"The private key container '{uniqueKeyContainerName}' of the certificate was not found in any of the known locations, so nothing was removed.");
             }
         }
 

src/Commands/EntraID/RegisterEntraIDApp.cs

@@ -516,7 +516,7 @@ private X509Certificate2 GetCertificate(PSObject record)
                     string pfxPath = Path.Combine(OutPath, $"{ApplicationName}.pfx");
                     string cerPath = Path.Combine(OutPath, $"{ApplicationName}.cer");
                     byte[] certPfxData = cert.Export(X509ContentType.Pfx, CertificatePassword);
-                    File.WriteAllBytes(pfxPath, certPfxData);
+                    CertificateHelper.WritePrivateKeyFile(pfxPath, certPfxData);
                     record.Properties.Add(new PSVariableProperty(new PSVariable("Pfx file", pfxPath)));
 
                     byte[] certCerData = cert.Export(X509ContentType.Cert);

src/Commands/Lists/GetListItemAttachment.cs

@@ -66,7 +66,9 @@ protected override void ExecuteCmdlet()
                 // Enumerate over the attachments and download them
                 foreach (var attachment in attachmentFilesCollection)
                 {
-                    string fileOut = System.IO.Path.Combine(Path, attachment.FileName);
+                    // Only the file name part of what the server returned is combined with the path, as Path.Combine silently discards the path it is
+                    // given when the second argument turns out to be rooted
+                    string fileOut = System.IO.Path.Combine(Path, System.IO.Path.GetFileName(attachment.FileName));
 
                     if (System.IO.File.Exists(fileOut) && !Force)
                     {

src/Commands/Model/SPODeletedSite.cs

@@ -169,7 +169,8 @@ internal SPODeletedSite(DeletedSiteProperties deletedSiteProperties, bool fetchA
 				var list = clientContext.Web.Lists.GetByTitle("DO_NOT_DELETE_SPLIST_TENANTADMIN_ALL_SITES_AGGREGATED_SITECOLLECTIONS");
 				CamlQuery query = new CamlQuery
 				{
-					ViewXml = $"<View><Query><Where><Eq><FieldRef Name='SiteUrl' /><Value Type='Text'>{Url}</Value></Eq></Where></Query><RowLimit>1</RowLimit></View>"
+					// The url is escaped as it goes into the CAML as element content, where an ampersand or an angle bracket would otherwise make the query malformed
+					ViewXml = $"<View><Query><Where><Eq><FieldRef Name='SiteUrl' /><Value Type='Text'>{System.Security.SecurityElement.Escape(Url)}</Value></Eq></Where></Query><RowLimit>1</RowLimit></View>"
 				};
 
 				var listItems = list.GetItems(query);

src/Commands/Utilities/CertificateHelper.cs

@@ -116,6 +116,46 @@ internal static X509Certificate2 GetCertificateFromStore(string thumbprint)
             return null;
         }
 
+        /// <summary>
+        /// Writes a file which holds private key material, ensuring that only the current user is able to read it back.
+        /// </summary>
+        /// <param name="path">Path of the file to write to.</param>
+        /// <param name="contents">Contents to write, i.e. the exported PKCS#12 bytes.</param>
+        /// <remarks>
+        /// On Linux and macOS a file is created using the default permissions of the process, which the usual umask leaves at 0644, meaning any
+        /// local account could read the private key. Windows has no equivalent notion and throws on these APIs, so there the file keeps inheriting
+        /// the permissions of the folder it is written to.
+        /// </remarks>
+        internal static void WritePrivateKeyFile(string path, byte[] contents)
+        {
+            // Deliberately the framework check rather than the PnP one, as that is what lets the platform compatibility analyzer see that the code
+            // below is unreachable on Windows, where the two Unix mode APIs throw
+            if (System.OperatingSystem.IsWindows())
+            {
+                File.WriteAllBytes(path, contents);
+                return;
+            }
+
+            var fileStreamOptions = new FileStreamOptions
+            {
+                Mode = FileMode.Create,
+                Access = FileAccess.Write,
+
+                // Applies when the file gets created, so that the key is never readable by others, not even briefly
+                UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite
+            };
+
+            using var fileStream = File.Open(path, fileStreamOptions);
+
+            // The mode above is only applied to a file which did not exist yet. Writing over a file which is already there truncates it and leaves
+            // whatever permissions it had, so the mode is set again here, while the file is still empty, to cover that case as well. This is done
+            // against the handle rather than against the path, so that replacing the path in between cannot send the mode change to another file
+            // while the key is written to this one.
+            File.SetUnixFileMode(fileStream.SafeFileHandle, UnixFileMode.UserRead | UnixFileMode.UserWrite);
+
+            fileStream.Write(contents, 0, contents.Length);
+        }
+
         /// <summary>
         /// Opens the X509Certificate2 at the provided path using the provided certificate password
         /// </summary>
@@ -136,15 +176,15 @@ internal static X509Certificate2 GetCertificateFromPath(Cmdlet cmdlet, string ce
             {
                 Log.Debug("CertificateHelper", $"Reading certificate from file '{certificatePath}'");
 
-                var certFile = System.IO.File.OpenRead(certificatePath);
-                if (certFile.Length == 0)
+                // Read through to a byte array in one go rather than holding the file open: the stream used to be left undisposed, which kept the
+                // certificate file locked for as long as the PowerShell session lived, and the partial read it performed could hand a truncated
+                // certificate to the constructor below and have it reported as a corrupt certificate
+                var certificateBytes = System.IO.File.ReadAllBytes(certificatePath);
+                if (certificateBytes.Length == 0)
                 {
                     throw new PSArgumentException($"The specified certificate path '{certificatePath}' points to an empty file");
                 }
 
-                var certificateBytes = new byte[certFile.Length];
-                certFile.Read(certificateBytes, 0, (int)certFile.Length);
-
                 Log.Debug("CertificateHelper", $"Opening certificate in file '{certificatePath}' {(certificatePassword == null ? "without" : "using")} a certificate password");
                 try
                 {