-
Notifications
You must be signed in to change notification settings - Fork 8.3k
Trusted Publisher Checks for Azure Trusted Signing #25824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jborean93
wants to merge
4
commits into
PowerShell:master
Choose a base branch
from
jborean93:trusted-publisher-az-ts
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+417
−16
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,15 @@ | |
|
|
||
| using System; | ||
| using System.Collections.ObjectModel; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.IO; | ||
| using System.Management.Automation; | ||
| using System.Management.Automation.Host; | ||
| using System.Management.Automation.Internal; | ||
| using System.Management.Automation.Language; | ||
| using System.Management.Automation.Security; | ||
| using System.Security; | ||
| using System.Security.Cryptography; | ||
| using System.Security.Cryptography.X509Certificates; | ||
| using System.Text; | ||
|
|
||
|
|
@@ -65,6 +67,22 @@ internal enum RunPromptDecision | |
|
|
||
| #region constructor | ||
|
|
||
| /// <summary> | ||
| /// The EKU OID that identifies a certificate is from Azure Trusted Signing. | ||
| /// </summary> | ||
| private const string _azureTrustedSigningIdentifier = "1.3.6.1.4.1.311.97.1.0"; | ||
|
|
||
| /// <summary> | ||
| /// The OID prefix that uniquely identifies a certificate issued by Azure Trusted Signing. | ||
| /// </summary> | ||
| private const string _azureTrustedSigningIdPrefix = "1.3.6.1.4.1.311.97."; | ||
|
|
||
| [TraceSource("SecurityManager", "Security Manager Script Trust Checks.")] | ||
| private static readonly PSTraceSource s_tracer = PSTraceSource.GetTracer( | ||
| "SecurityManager", | ||
| "Security Manager Script Trust Checks.", | ||
| false); | ||
|
|
||
| // execution policy that dictates what can run in msh | ||
| private ExecutionPolicy _executionPolicy; | ||
|
|
||
|
|
@@ -217,7 +235,7 @@ private bool CheckPolicy(ExternalScriptInfo script, PSHost host, out Exception r | |
| if (signature.Status == SignatureStatus.Valid) | ||
| { | ||
| // The file is signed by a trusted publisher | ||
| if (IsTrustedPublisher(signature, path)) | ||
| if (IsTrustedPublisher(signature)) | ||
| { | ||
| policyCheckPassed = true; | ||
| } | ||
|
|
@@ -287,7 +305,7 @@ private bool CheckPolicy(ExternalScriptInfo script, PSHost host, out Exception r | |
| if (signature.Status == SignatureStatus.Valid) | ||
| { | ||
| // The file is signed by a trusted publisher | ||
| if (IsTrustedPublisher(signature, path)) | ||
| if (IsTrustedPublisher(signature)) | ||
| { | ||
| policyCheckPassed = true; | ||
| } | ||
|
|
@@ -350,7 +368,7 @@ private bool CheckPolicy(ExternalScriptInfo script, PSHost host, out Exception r | |
| // The file is signed by a trusted publisher | ||
| if (signature.Status == SignatureStatus.Valid) | ||
| { | ||
| if (IsTrustedPublisher(signature, path)) | ||
| if (IsTrustedPublisher(signature)) | ||
| { | ||
| policyCheckPassed = true; | ||
| } | ||
|
|
@@ -431,51 +449,173 @@ private static bool IsLocalFile(string filename) | |
| #endif | ||
| } | ||
|
|
||
| // Checks that a publisher is trusted by the system or is one of | ||
| // the signed product binaries | ||
| private static bool IsTrustedPublisher(Signature signature, string file) | ||
| #nullable enable | ||
| /// <summary> | ||
| /// Checks if the publisher is trusted by checking whether the | ||
| /// certificate thumbprint is in the "Trusted Publishers" store or | ||
| /// the Azure Trusted Signer Publisher ID is present in the | ||
| /// "Trusted Publishers" store. | ||
| /// </summary> | ||
| /// <param name="signature">The signature to check.</param> | ||
| /// <returns>True if the publisher is trusted.</returns> | ||
| private static bool IsTrustedPublisher(Signature signature) | ||
| { | ||
| // Get the thumbprint of the current signature | ||
| X509Certificate2 signerCertificate = signature.SignerCertificate; | ||
| string thumbprint = signerCertificate.Thumbprint; | ||
| s_tracer.WriteLine("Checking if publisher with thumbprint {0} is trusted.", thumbprint); | ||
|
|
||
| TryGetAzureTrustedSignerPublisherId(signerCertificate, out string? azurePublisherId); | ||
|
|
||
| // See if it matches any in the list of trusted publishers | ||
| X509Store trustedPublishers = new X509Store(StoreName.TrustedPublisher, StoreLocation.CurrentUser); | ||
| trustedPublishers.Open(OpenFlags.ReadOnly); | ||
|
|
||
| bool isTrusted = false; | ||
| foreach (X509Certificate2 trustedCertificate in trustedPublishers.Certificates) | ||
| { | ||
| s_tracer.WriteLine("Checking publisher against certificate '{0}' and thumbprint {1}.", | ||
| trustedCertificate.FriendlyName, | ||
| trustedCertificate.Thumbprint); | ||
|
|
||
| if (string.Equals(trustedCertificate.Thumbprint, thumbprint, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| if (!IsUntrustedPublisher(signature, file)) | ||
| { | ||
| return true; | ||
| } | ||
| isTrusted = true; | ||
| } | ||
| else if (azurePublisherId is not null && | ||
| TryGetAzureTrustedSignerPublisherId(trustedCertificate, out string? trustedIdentifier) && | ||
| azurePublisherId == trustedIdentifier) | ||
| { | ||
| isTrusted = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Do a final check to verify that the certificate has not been | ||
| // explicitly added to the "Disallowed" store. | ||
| if (isTrusted && !IsUntrustedPublisher(signerCertificate)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd expect IsUntrustedPublisher() on top of the method - it makes no sense to check trust if certificate is blocked. |
||
| { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static bool IsUntrustedPublisher(Signature signature, string file) | ||
| /// <summary> | ||
| /// Checks if the publisher is untrusted by checking whether the same | ||
| /// certificate thumbprint is in the "Disallowed" store. | ||
| /// </summary> | ||
| /// <param name="signerCertificate">The certificate to check by thumbprint.</param> | ||
| /// <returns>True when the publisher is untrusted.</returns> | ||
| private static bool IsUntrustedPublisher(X509Certificate2 signerCertificate) | ||
| { | ||
| // Get the thumbprint of the current signature | ||
| X509Certificate2 signerCertificate = signature.SignerCertificate; | ||
| string thumbprint = signerCertificate.Thumbprint; | ||
| s_tracer.WriteLine("Checking if certificate {0} is untrusted.", | ||
| thumbprint); | ||
|
|
||
| // See if it matches any in the list of trusted publishers | ||
| X509Store trustedPublishers = new X509Store(StoreName.Disallowed, StoreLocation.CurrentUser); | ||
| trustedPublishers.Open(OpenFlags.ReadOnly); | ||
| X509Store untrustedPublishers = new X509Store(StoreName.Disallowed, StoreLocation.CurrentUser); | ||
| untrustedPublishers.Open(OpenFlags.ReadOnly); | ||
|
|
||
| foreach (X509Certificate2 trustedCertificate in trustedPublishers.Certificates) | ||
| foreach (X509Certificate2 untrustedCertificate in untrustedPublishers.Certificates) | ||
| { | ||
| if (string.Equals(trustedCertificate.Thumbprint, thumbprint, StringComparison.OrdinalIgnoreCase)) | ||
| s_tracer.WriteLine("Checking publisher against untrusted certificate '{0}' and thumbprint {1}.", | ||
| untrustedCertificate.FriendlyName, | ||
| untrustedCertificate.Thumbprint); | ||
|
|
||
| if (string.Equals(untrustedCertificate.Thumbprint, thumbprint, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks if the certificate has the Azure Trusted Signer Publisher ID | ||
| /// EKU present and sets publisherId to that unique identifier. | ||
| /// </summary> | ||
| /// <param name="certificate">The certificate to check.</param> | ||
| /// <param name="publisherId">An opaque blob that uniquely identifies the publisher if present.</param> | ||
| /// <returns>True when the certificate has the Azure Trusted Signer Publisher ID EKU.</returns> | ||
| private static bool TryGetAzureTrustedSignerPublisherId( | ||
| X509Certificate2 certificate, | ||
| [NotNullWhen(true)] out string? publisherId) | ||
| { | ||
| bool containsAzTSIdentifier = false; | ||
| string? azurePubOid = null; | ||
|
|
||
| foreach (X509Extension ext in certificate.Extensions) | ||
| { | ||
| if (ext is X509EnhancedKeyUsageExtension ekuExt) | ||
| { | ||
| // The EKU OIDs need to contain the Azure Trusted Signing Identifier | ||
| // and have one that starts with the Azure Trusted Signing ID Prefix. | ||
| foreach (Oid oid in ekuExt.EnhancedKeyUsages) | ||
| { | ||
| if (oid.Value == _azureTrustedSigningIdentifier) | ||
| { | ||
| containsAzTSIdentifier = true; | ||
| } | ||
| else if (oid.Value?.StartsWith(_azureTrustedSigningIdPrefix) == true) | ||
| { | ||
| azurePubOid = oid.Value; | ||
| } | ||
| } | ||
|
|
||
| break; // No need to check other extensions. | ||
| } | ||
| } | ||
|
|
||
| string? caThumbprint = null; | ||
| if (containsAzTSIdentifier && azurePubOid is not null) | ||
| { | ||
| s_tracer.WriteLine("Certificate {0} has Azure Trusted Signer EKU OID {1}.", | ||
| certificate.Thumbprint, | ||
| azurePubOid); | ||
|
|
||
| // To avoid matching on certs that have the same EKU OID added | ||
| // we add the thumbprint of the root CA to the unique | ||
| // identifier. This means someone can't manually create a | ||
| // cert with the same OID as one already trusted as it needs to | ||
| // come from the same CA. We don't do a revocation check as we | ||
| // aren't checking the validity of the certificate, just getting | ||
| // the thumbprint of the root CA. | ||
| using X509Chain chain = new X509Chain(); | ||
| chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; | ||
| if (chain.Build(certificate)) | ||
| { | ||
| // Remarks state that the last element in the chain in the | ||
| // root CA on all platforms. | ||
| caThumbprint = chain.ChainElements[^1].Certificate.Thumbprint; | ||
| } | ||
| else | ||
| { | ||
| s_tracer.WriteLine("Failed to find root CA for certificate {0}: {1}", | ||
| certificate.Thumbprint, | ||
| chain.ChainStatus[0].StatusInformation); | ||
| } | ||
| } | ||
|
|
||
| if (caThumbprint is not null) | ||
| { | ||
| publisherId = $"{azurePubOid}.{caThumbprint}"; | ||
|
|
||
| s_tracer.WriteLine("Publisher ID for certificate {0} is {1}.", | ||
| certificate.Thumbprint, | ||
| publisherId); | ||
| return true; | ||
| } | ||
| else | ||
| { | ||
| publisherId = null; | ||
| return false; | ||
| } | ||
| } | ||
| #nullable disable | ||
|
|
||
| /// <summary> | ||
| /// Trust a publisher by adding it to the "Trusted Publishers" store. | ||
| /// </summary> | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The break statement exits the loop early when an Azure publisher ID match is found, but the thumbprint check continues through all certificates. For consistency and performance, consider adding a break after setting isTrusted = true for the thumbprint match as well.