diff --git a/src/ProjectTemplates/Shared/DevelopmentCertificate.cs b/src/ProjectTemplates/Shared/DevelopmentCertificate.cs index e9c8ea6fa169..080f35c3b3f8 100644 --- a/src/ProjectTemplates/Shared/DevelopmentCertificate.cs +++ b/src/ProjectTemplates/Shared/DevelopmentCertificate.cs @@ -35,7 +35,7 @@ private static string EnsureDevelopmentCertificates(string certificatePath, stri var manager = CertificateManager.Instance; var certificate = manager.CreateAspNetCoreHttpsDevelopmentCertificate(now, now.AddYears(1)); var certificateThumbprint = certificate.Thumbprint; - CertificateManager.ExportCertificate(certificate, path: certificatePath, includePrivateKey: true, certificatePassword, CertificateKeyExportFormat.Pfx); + manager.ExportCertificate(certificate, path: certificatePath, includePrivateKey: true, certificatePassword, CertificateKeyExportFormat.Pfx); return certificateThumbprint; } diff --git a/src/Shared/CertificateGeneration/CertificateManager.cs b/src/Shared/CertificateGeneration/CertificateManager.cs index eb16e6f51b54..96e5630c43f1 100644 --- a/src/Shared/CertificateGeneration/CertificateManager.cs +++ b/src/Shared/CertificateGeneration/CertificateManager.cs @@ -323,6 +323,14 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( { try { + // If the user specified a non-existent directory, we don't want to be responsible + // for setting the permissions appropriately, so we'll bail. + var exportDir = Path.GetDirectoryName(path); + if (!string.IsNullOrEmpty(exportDir) && !Directory.Exists(exportDir)) + { + throw new InvalidOperationException($"The directory '{exportDir}' does not exist. Choose permissions carefully when creating it."); + } + ExportCertificate(certificate, path, includePrivateKey, password, keyExportFormat); } catch (Exception e) @@ -484,7 +492,13 @@ public void CleanupHttpsCertificates() protected abstract IList GetCertificatesToRemove(StoreName storeName, StoreLocation storeLocation); - internal static void ExportCertificate(X509Certificate2 certificate, string path, bool includePrivateKey, string? password, CertificateKeyExportFormat format) + protected abstract void CreateDirectoryWithPermissions(string directoryPath); + + /// + /// Will create directories to make it possible to write to . + /// If you don't want that, check for existence before calling this method. + /// + internal void ExportCertificate(X509Certificate2 certificate, string path, bool includePrivateKey, string? password, CertificateKeyExportFormat format) { if (Log.IsEnabled()) { @@ -500,7 +514,7 @@ internal static void ExportCertificate(X509Certificate2 certificate, string path if (!string.IsNullOrEmpty(targetDirectoryPath)) { Log.CreateExportCertificateDirectory(targetDirectoryPath); - Directory.CreateDirectory(targetDirectoryPath); + CreateDirectoryWithPermissions(targetDirectoryPath); } byte[] bytes; @@ -1230,6 +1244,9 @@ public sealed class CertificateManagerEventSource : EventSource [Event(111, Level = EventLevel.LogAlways, Message = "For OpenSSL trust to take effect, '{0}' must be listed in the {2} environment variable. " + "See https://aka.ms/dev-certs-trust for more information.")] internal void UnixSuggestSettingEnvironmentVariableWithoutExample(string certDir, string envVarName) => WriteEvent(111, certDir, envVarName); + + [Event(112, Level = EventLevel.Warning, Message = "Directory '{0}' may be readable by other users.")] + internal void DirectoryPermissionsNotSecure(string directoryPath) => WriteEvent(112, directoryPath); } internal sealed class UserCancelledTrustException : Exception diff --git a/src/Shared/CertificateGeneration/MacOSCertificateManager.cs b/src/Shared/CertificateGeneration/MacOSCertificateManager.cs index b8c16b50c9a4..a38e22762190 100644 --- a/src/Shared/CertificateGeneration/MacOSCertificateManager.cs +++ b/src/Shared/CertificateGeneration/MacOSCertificateManager.cs @@ -18,6 +18,8 @@ namespace Microsoft.AspNetCore.Certificates.Generation; /// internal sealed class MacOSCertificateManager : CertificateManager { + private const UnixFileMode DirectoryPermissions = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute; + // User keychain. Guard with quotes when using in command lines since users may have set // their user profile (HOME) directory to a non-standard path that includes whitespace. private static readonly string MacOSUserKeychain = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) + "/Library/Keychains/login.keychain-db"; @@ -93,6 +95,7 @@ protected override TrustLevel TrustCertificateCore(X509Certificate2 publicCertif var tmpFile = Path.GetTempFileName(); try { + // We can't guarantee that the temp file is in a directory with sensible permissions, but we're not exporting the private key ExportCertificate(publicCertificate, tmpFile, includePrivateKey: false, password: null, CertificateKeyExportFormat.Pfx); if (Log.IsEnabled()) { @@ -134,9 +137,7 @@ internal override void CorrectCertificateState(X509Certificate2 candidate) { try { - // Ensure that the directory exists before writing to the file. - Directory.CreateDirectory(MacOSUserHttpsCertificateLocation); - + // This path is in a well-known folder, so we trust the permissions. var certificatePath = GetCertificateFilePath(candidate); ExportCertificate(candidate, certificatePath, includePrivateKey: true, null, CertificateKeyExportFormat.Pfx); } @@ -152,6 +153,7 @@ public override TrustLevel GetTrustLevel(X509Certificate2 certificate) var tmpFile = Path.GetTempFileName(); try { + // We can't guarantee that the temp file is in a directory with sensible permissions, but we're not exporting the private key ExportCertificate(certificate, tmpFile, includePrivateKey: false, password: null, CertificateKeyExportFormat.Pem); using var checkTrustProcess = Process.Start(new ProcessStartInfo( @@ -316,7 +318,7 @@ protected override X509Certificate2 SaveCertificateCore(X509Certificate2 certifi } // Ensure that the directory exists before writing to the file. - Directory.CreateDirectory(MacOSUserHttpsCertificateLocation); + CreateDirectoryWithPermissions(MacOSUserHttpsCertificateLocation); File.WriteAllBytes(GetCertificateFilePath(certificate), certBytes); } @@ -474,4 +476,22 @@ protected override void RemoveCertificateFromUserStoreCore(X509Certificate2 cert RemoveCertificateFromKeychain(MacOSUserKeychain, certificate); } } + + protected override void CreateDirectoryWithPermissions(string directoryPath) + { +#pragma warning disable CA1416 // Validate platform compatibility (not supported on Windows) + var dirInfo = new DirectoryInfo(directoryPath); + if (dirInfo.Exists) + { + if ((dirInfo.UnixFileMode & ~DirectoryPermissions) != 0) + { + Log.DirectoryPermissionsNotSecure(dirInfo.FullName); + } + } + else + { + Directory.CreateDirectory(directoryPath, DirectoryPermissions); + } +#pragma warning restore CA1416 // Validate platform compatibility + } } diff --git a/src/Shared/CertificateGeneration/UnixCertificateManager.cs b/src/Shared/CertificateGeneration/UnixCertificateManager.cs index 6fa154bb8e93..c099794df9a1 100644 --- a/src/Shared/CertificateGeneration/UnixCertificateManager.cs +++ b/src/Shared/CertificateGeneration/UnixCertificateManager.cs @@ -20,6 +20,8 @@ namespace Microsoft.AspNetCore.Certificates.Generation; /// internal sealed partial class UnixCertificateManager : CertificateManager { + private const UnixFileMode DirectoryPermissions = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute; + /// The name of an environment variable consumed by OpenSSL to locate certificates. private const string OpenSslCertificateDirectoryVariableName = "SSL_CERT_DIR"; @@ -74,7 +76,8 @@ public override TrustLevel GetTrustLevel(X509Certificate2 certificate) Log.UnixNotTrustedByDotnet(); } - var nickname = GetCertificateNickname(certificate); + // Will become the name of the file on disk and the nickname in the NSS DBs + var certificateNickname = GetCertificateNickname(certificate); var sslCertDirString = Environment.GetEnvironmentVariable(OpenSslCertificateDirectoryVariableName); if (string.IsNullOrEmpty(sslCertDirString)) @@ -88,7 +91,7 @@ public override TrustLevel GetTrustLevel(X509Certificate2 certificate) var sslCertDirs = sslCertDirString.Split(Path.PathSeparator); foreach (var sslCertDir in sslCertDirs) { - var certPath = Path.Combine(sslCertDir, nickname + ".pem"); + var certPath = Path.Combine(sslCertDir, certificateNickname + ".pem"); if (File.Exists(certPath)) { var candidate = X509CertificateLoader.LoadCertificateFromFile(certPath); @@ -125,7 +128,7 @@ public override TrustLevel GetTrustLevel(X509Certificate2 certificate) { foreach (var nssDb in nssDbs) { - if (IsCertificateInNssDb(nickname, nssDb)) + if (IsCertificateInNssDb(certificateNickname, nssDb)) { sawTrustSuccess = true; } @@ -138,6 +141,7 @@ public override TrustLevel GetTrustLevel(X509Certificate2 certificate) } } + // Success & Failure => Partial; Success => Full; Failure => None return sawTrustSuccess ? sawTrustFailure ? TrustLevel.Partial @@ -244,7 +248,7 @@ protected override TrustLevel TrustCertificateCore(X509Certificate2 certificate) if (needToExport) { // Security: we don't need the private key for trust, so we don't export it. - // Note that this will create directories as needed. + // Note that this will create directories as needed. We control `certPath`, so the permissions should be fine. ExportCertificate(certificate, certPath, includePrivateKey: false, password: null, CertificateKeyExportFormat.Pem); } @@ -449,6 +453,24 @@ protected override IList GetCertificatesToRemove(StoreName sto return ListCertificates(StoreName.My, StoreLocation.CurrentUser, isValid: false, requireExportable: false); } + protected override void CreateDirectoryWithPermissions(string directoryPath) + { +#pragma warning disable CA1416 // Validate platform compatibility (not supported on Windows) + var dirInfo = new DirectoryInfo(directoryPath); + if (dirInfo.Exists) + { + if ((dirInfo.UnixFileMode & ~DirectoryPermissions) != 0) + { + Log.DirectoryPermissionsNotSecure(dirInfo.FullName); + } + } + else + { + Directory.CreateDirectory(directoryPath, DirectoryPermissions); + } +#pragma warning restore CA1416 // Validate platform compatibility + } + private static string GetChromiumNssDb(string homeDirectory) { return Path.Combine(homeDirectory, ".pki", "nssdb"); diff --git a/src/Shared/CertificateGeneration/WindowsCertificateManager.cs b/src/Shared/CertificateGeneration/WindowsCertificateManager.cs index 058fb7fef023..85a72be37c66 100644 --- a/src/Shared/CertificateGeneration/WindowsCertificateManager.cs +++ b/src/Shared/CertificateGeneration/WindowsCertificateManager.cs @@ -5,8 +5,10 @@ using System.Collections.Generic; using System.Linq; using System.Runtime.Versioning; +using System.Security.AccessControl; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; +using System.Security.Principal; namespace Microsoft.AspNetCore.Certificates.Generation; @@ -126,4 +128,41 @@ protected override IList GetCertificatesToRemove(StoreName sto { return ListCertificates(storeName, storeLocation, isValid: false); } + + protected override void CreateDirectoryWithPermissions(string directoryPath) + { + var dirInfo = new DirectoryInfo(directoryPath); + + if (!dirInfo.Exists) + { + // We trust the default permissions on Windows enough not to apply custom ACLs. + // We'll warn below if things seem really off. + dirInfo.Create(); + } + + var currentUser = WindowsIdentity.GetCurrent(); + var currentUserSid = currentUser.User; + var systemSid = new SecurityIdentifier(WellKnownSidType.LocalSystemSid, domainSid: null); + var adminGroupSid = new SecurityIdentifier(WellKnownSidType.BuiltinAdministratorsSid, domainSid: null); + + var dirSecurity = dirInfo.GetAccessControl(); + var accessRules = dirSecurity.GetAccessRules(true, true, typeof(SecurityIdentifier)); + + foreach (FileSystemAccessRule rule in accessRules) + { + var idRef = rule.IdentityReference; + if (rule.AccessControlType == AccessControlType.Allow && + !idRef.Equals(currentUserSid) && + !idRef.Equals(systemSid) && + !idRef.Equals(adminGroupSid)) + { + // This is just a heuristic - determining whether the cumulative effect of the rules + // is to allow access to anyone other than the current user, system, or administrators + // is very complicated. We're not going to do anything but log, so an approximation + // is fine. + Log.DirectoryPermissionsNotSecure(dirInfo.FullName); + break; + } + } + } } diff --git a/src/Tools/FirstRunCertGenerator/test/CertificateManagerTests.cs b/src/Tools/FirstRunCertGenerator/test/CertificateManagerTests.cs index e6c81b006a56..09a32825e50e 100644 --- a/src/Tools/FirstRunCertGenerator/test/CertificateManagerTests.cs +++ b/src/Tools/FirstRunCertGenerator/test/CertificateManagerTests.cs @@ -352,6 +352,30 @@ public void EnsureCreateHttpsCertificate_CanExportTheCertInPemFormat_WithoutPass Assert.Equal(httpsCertificate.GetCertHashString(), exportedCertificate.GetCertHashString()); } + [ConditionalFact] + [SkipOnHelix("https://github.com/dotnet/aspnetcore/issues/6720", Queues = "All.OSX")] + public void EnsureCreateHttpsCertificate_CannotExportToNonExistentDirectory() + { + // Arrange + const string CertificateName = nameof(EnsureCreateHttpsCertificate_CannotExportToNonExistentDirectory) + ".pem"; + + _fixture.CleanupCertificates(); + + var now = DateTimeOffset.UtcNow; + now = new DateTimeOffset(now.Year, now.Month, now.Day, now.Hour, now.Minute, now.Second, 0, now.Offset); + var creation = _manager.EnsureAspNetCoreHttpsDevelopmentCertificate(now, now.AddYears(1), path: null, trust: false, isInteractive: false); + Output.WriteLine(creation.ToString()); + ListCertificates(); + + // Act + // Export the certificate (same method, but this time with an output path) + var result = _manager + .EnsureAspNetCoreHttpsDevelopmentCertificate(now, now.AddYears(1), Path.Combine("NoSuchDirectory", CertificateName)); + + // Assert + Assert.Equal(EnsureCertificateResult.ErrorExportingTheCertificate, result); + } + [Fact] public void EnsureCreateHttpsCertificate_ReturnsExpiredCertificateIfVersionIsIncorrect() {