Simplify and improve signature verification for workloads on Windows #36787
Conversation
dotnet-issue-labeler
bot
added
Area-Workloads
untriaged
|
@dtivel would you mind looking at the WinVerifyTrust and X509 extension methods? I'm still working on fixing up Source Build |
src/Cli/dotnet/Installer/Windows/Security/X509CertificateExtensions.cs
Outdated
Show resolved
Hide resolved
|
|
||
| bool isTrustedOrganization = AuthentiCode.IsSignedByTrustedOrganization(msiPath, AuthentiCode.TrustedOrganizations); | ||
| // We know there's a signature, so we can perform additional checks. | ||
| X509Certificate certificate = X509Certificate.CreateFromSignedFile(msiPath); |
| CERT_CHAIN_POLICY_STATUS policyStatus = default; | ||
|
|
||
| using X509Chain chain = new(); | ||
| bool buildResult = chain.Build(new X509Certificate2(certificate)); |
There was a problem hiding this comment.
Same here. Either pass certificate to `X509Chain.Build(...) or dispose of the new certificate.
Also, why ignore buildResult? X509Chain.Build(...) could succeed (i.e.: buildResult == true) or it could fail with a variety of states (per X509Chain.ChainStatus), some of which may be recoverable, some not. For example, if the certificate has expired but you have a valid, trusted timestamp, then you may want to ignore X509ChainStatusFlags.NotTimeValid. There are a myriad of other flags you probably don't want to ignore.
There was a problem hiding this comment.
My thinking was that at this point I would have verified everything I wanted. I was mainly relying on this to create the context pointer for me so I could call into CertVerify... API. But given the issues you mentioned I'll have to restructure this a bit differently, otherwise I need to keep building the same chain and same checks in multiple places.
There was a problem hiding this comment.
As per the new iteration, we'll build the chain using the certificate handle and avoid using the X509Chain.Build() so we don't recheck policies covered by WinTrust
| trustData[0].dwUIChoice = WINTRUST_DATA_UICHOICE.WTD_UI_NONE; | ||
| trustData[0].fdwRevocationChecks = WINTRUST_DATA_REVOCATION_CHECKS.WTD_REVOKE_WHOLECHAIN; | ||
| trustData[0].dwUnionChoice = WINTRUST_DATA_UNION_CHOICE.WTD_CHOICE_FILE; | ||
| trustData[0].dwStateAction = WINTRUST_DATA_STATE_ACTION.WTD_STATEACTION_VERIFY; |
There was a problem hiding this comment.
Docs say:
| Value | Meaning |
|---|---|
| WTD_STATEACTION_IGNORE 0x00000000 | Ignore the hWVTStateData member. |
| WTD_STATEACTION_VERIFY 0x00000001 | Verify the trust of the object (typically a file) that is specified by the dwUnionChoice member. The hWVTStateData member will receive a handle to the state data. This handle must be freed by specifying the WTD_STATEACTION_CLOSE action in a subsequent call. |
| WTD_STATEACTION_CLOSE 0x00000002 | Free the hWVTStateData member previously allocated with the WTD_STATEACTION_VERIFY action. This action must be specified for every use of the WTD_STATEACTION_VERIFY action. |
This sample shows proper use of WTD_STATEACTION_VERIFY + WTD_STATEACTION_CLOSE.
I don't see a call with WTD_STATEACTION_CLOSE anywhere. Should you use WTD_STATEACTION_IGNORE instead?
There was a problem hiding this comment.
No, I think I need to just make the release call to free up the handle.
| /// <param name="path">The path of the file to verify.</param> | ||
| /// <param name="cacheOnlyRevocationChecks">Prevents revocation checks over the network when set to <see langword="true"/>.</param> | ||
| /// <returns>0 if successful.</returns> | ||
| public static unsafe int IsSigned(string path, bool cacheOnlyRevocationChecks = false) |
There was a problem hiding this comment.
cacheOnlyRevocationChecks is poorly named, as it implies that only revocation checks will be cached when true. (Windows may cache other things, like intermediate certificate authority certificates.) How about allowOnlineRevocationChecks?
There was a problem hiding this comment.
Agree, the suggested name is more clear about the intent.
There was a problem hiding this comment.
Renamed the policy key to AllowOnlineRevocationChecks
| var certificates = AuthentiCode.GetCertificates(Assembly.GetExecutingAssembly().Location); | ||
|
|
||
| Assert.Empty(certificates); | ||
| X509Certificate certificate = X509Certificate.CreateFromSignedFile(Path.Combine(s_testDataPath, file)); |
There was a problem hiding this comment.
Most X509 pieces are removed in the new iteration, but I did add usings for the few that remain
|
I'm still waiting on updated runtime-asset packages for the tests. Locally they pass, but until the new package is available in the feed, they'll fail in the PR build. |
| throw new CryptographicException(string.Format(LocalizableStrings.UnableToCheckCertificateChainPolicy, nameof(CERT_CHAIN_POLICY_MICROSOFT_ROOT))); | ||
| } | ||
|
|
||
| CertFreeCertificateChain(pChainContext); |
There was a problem hiding this comment.
CertFreeCertificateChain has three manual calls in this routine. I recommend wrapping lines 43-64 in a try/finally and just making the call once, in the finally.
There was a problem hiding this comment.
Fixed in the latest commit
| trustData[0].dwUIContext = 0; | ||
| trustData[0].Anonymous.pFile = fileInfo; |
There was a problem hiding this comment.
I'd move these up to be with the rest of the unconditional assignments.
| internal static unsafe int HasMicrosoftTrustedRoot(string path) | ||
| { | ||
| // Create an X509Certificate2 instance so we can access the certificate context and create a chain context. | ||
| using X509Certificate2 certificate = new(path); |
There was a problem hiding this comment.
I feel like WinVerifyTrust already exposes back the certificate, and this has to replicate that work. I advise checking to see if you can already get this from trustData->pCert. If so, then between the action call to WinVerifyTrust and the cleanup call, do PCCERT_CONTEXT pCertContext = CertDuplicateCertificateContext(trustData->pCert->psCertContext);, and pass the PCCERT_CONTEXT (or CERT_CONTEXT*, I suppose) into this method instead of string path.
It's both more performant, and would avoid a theoretical ToC/ToU problem if the file isn't locked during your call.
(Don't forget to CertFreeCertificateContext the value after 😄)
There was a problem hiding this comment.
I don't think I have access to it. Since I'm verifying a file, the union struct is a WINTRUT_FILE_INFO* and not a WINTRUST_CERT_INFO*. In this case, the WinVerifyTrust call creates the cert context from the file, but won't expose that back to me.
I suppose I could do it the other way around, extract the cert context from the file, then let WinVerifyTrust verify the cert context, but I'll need to check if the OS does anything else when asked to verify a file. Then I could do dwUnionChoice = WINTRUST_DATA_UNION_CHOICE.WTD_CHOICE_CERT instead of dwUnionChoice = WINTRUST_DATA_UNION_CHOICE.WTD_CHOICE_FILE
There was a problem hiding this comment.
Ah, I thought that some of the other data got populated by calling into WinVerifyTrust; and had glossed over it being a union type.
So, disregard.
|
What will this feature do in airgapped environments? Are we assuming that I'm assuming that this feature will create a gap with Linux. What's the thinking on that? |
This is specific to Windows only as it's only used for verifying the signatures for the MSI packages. The policy flag will force the checks to use the cached certificate revocation list in the machine trust store. |
|
@richlander for some context, on non-Windows platforms we use the so-called 'file based' workload installation, which essentially just downloads a NuGet package and extracts its contents to a specific folder structure. In these scenarios we should be making use of NuGet package signing when that is enabled on each platform. This MSI signature verification is something we can do for MSI-based workload installations, which are Windows-only. When we implement package-manager-based workloads for e.g. Linux platforms, we could potentially implement similar signature checks. |
PR feedback Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
PR feedback Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
|
/backport to release/8.0.1xx |
|
Started backporting to release/8.0.1xx: https://github.com/dotnet/sdk/actions/runs/7415512861 |
|
@joeloff backporting to release/8.0.1xx failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Preliminary code change
.git/rebase-apply/patch:401: trailing whitespace.
return WinVerifyTrust((HWND)IntPtr.Zero, ref policyGuid, trustData);
.git/rebase-apply/patch:404: trailing whitespace.
}
.git/rebase-apply/patch:1109: trailing whitespace.
/// Determines whether the certificate is intended for code signing.
warning: 3 lines add whitespace errors.
Using index info to reconstruct a base tree...
M eng/Versions.props
M src/Cli/dotnet/Installer/Windows/MsiPackageCache.cs
M src/Cli/dotnet/Installer/Windows/Security/NativeMethods.cs
M src/Cli/dotnet/Installer/Windows/Security/WinTrustData.cs
M src/Cli/dotnet/commands/dotnet-workload/SignCheck.cs
M src/Cli/dotnet/commands/dotnet-workload/install/NetSdkMsiInstallerClient.cs
M src/Cli/dotnet/dotnet.csproj
Falling back to patching base and 3-way merge...
Auto-merging src/Cli/dotnet/dotnet.csproj
Auto-merging src/Cli/dotnet/commands/dotnet-workload/install/NetSdkMsiInstallerClient.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/SignCheck.cs
Removing src/Cli/dotnet/Installer/Windows/Security/WinTrustFileInfo.cs
CONFLICT (modify/delete): src/Cli/dotnet/Installer/Windows/Security/WinTrustData.cs deleted in Preliminary code change and modified in HEAD. Version HEAD of src/Cli/dotnet/Installer/Windows/Security/WinTrustData.cs left in tree.
Removing src/Cli/dotnet/Installer/Windows/Security/UnionChoice.cs
Removing src/Cli/dotnet/Installer/Windows/Security/UIChoice.cs
Removing src/Cli/dotnet/Installer/Windows/Security/StateAction.cs
Removing src/Cli/dotnet/Installer/Windows/Security/RevocationChecks.cs
Removing src/Cli/dotnet/Installer/Windows/Security/ProviderSettings.cs
CONFLICT (modify/delete): src/Cli/dotnet/Installer/Windows/Security/NativeMethods.cs deleted in Preliminary code change and modified in HEAD. Version HEAD of src/Cli/dotnet/Installer/Windows/Security/NativeMethods.cs left in tree.
Removing src/Cli/dotnet/Installer/Windows/Security/CryptQueryObjectType.cs
Removing src/Cli/dotnet/Installer/Windows/Security/Crypt32.cs
Removing src/Cli/dotnet/Installer/Windows/Security/CertQueryFormatFlags.cs
Removing src/Cli/dotnet/Installer/Windows/Security/CertQueryContentFlags.cs
Auto-merging src/Cli/dotnet/Installer/Windows/MsiPackageCache.cs
Auto-merging eng/Versions.props
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Preliminary code change
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@joeloff an error occurred while backporting to release/8.0.1xx, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
Description
Improve code signing verification on Windows for workloads (MSI based installs).
WinVerifyTrust. Revocation checks default to online, except if theAllowOnlineRevocationCheckspolicy key is set to 0. The policy setting can be used to perform revocation checks when network access is restricted.This change also introduces the use of
Microsoft.Windows.CsWin32to generate all the native structs and API calls. As a result, we can safely remove a chunk of code and rely on the generated source code instead.