dotnet-issue-labeler[bot]
commented
2 years ago I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.
Closed lindexi closed 2 years ago
dotnet-issue-labeler[bot]
commented
2 years ago I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.
ghost
commented
2 years ago Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones See info in area-owners.md if you want to be subscribed.
| Author: | lindexi |
|---|---|
| Assignees: | - |
| Labels: | `area-System.Security`, `untriaged` |
| Milestone: | - |
lindexi
commented
2 years ago I guess this is the problem handled by the caller. As we can see, the ChainPal.BuildChain will return null when the CertGetCertificateChain return false.
But the CertGetCertificateChain requires calling CertFreeCertificateChain to release the resource, as the document says. See https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certgetcertificatechain
We can not call the CertFreeCertificateChain by ChainPal.ReleaseSafeX509ChainHandle when the ChainPal.BuildChain will return null.
It means that the CertGetCertificateChain may make the application memory leak.
Should we call the CertFreeCertificateChain when CertGetCertificateChain return false?
lindexi
commented
2 years ago Should we call the CertFreeCertificateChain when CertGetCertificateChain return false?
I read the code from mozilla. And I find mozilla do not call the CertFreeCertificateChain when CertGetCertificateChain return false.
See https://hg.mozilla.org/releases/mozilla-release/rev/d9659c22b3c5#l3.347
bartonjs
commented
2 years ago A couple of points:
chain.Dispose() before the return null; to make it more deterministic.GC.Collect(); GC.WaitForPendingFinalizers(); to your program before determining if there was a leak.My instincts say it's just a finalization latency problem.
lindexi
commented
2 years ago Thank you @bartonjs
If you aren't already, you should add GC.Collect(); GC.WaitForPendingFinalizers(); to your program before determining if there was a leak.
The bad news is that no any .NET objects have been leaked. And all the .NET objects be gc.
Even if CertGetCertificateChain returns false, we're tracking the PCCERT_CHAIN_CONTEXT via a SafeHandle
But the chain was not released by CertFreeCertificateChain.
Windows 7 is mostly out of support.
The bad news is there are still many users using Windows 7. Unless .NET abandons the Windows desktop developers.
lindexi
commented
2 years ago I wrote a Chinese blog about it.
bartonjs
commented
2 years ago Given that we're tracking the native resources with a SafeHandle, the release will get called eventually (even if CertGetCertificateChain returns false). It doesn't sound like the leak is coming from .NET.
Windows 7 is long out of main support and Extended Support ends soon. .NET 8 will not support running on Windows 7.
Without demonstration of the problem on a more modern OS there's nothing we can really do here.
lindexi
commented
2 years ago @bartonjs Will Win7 be supported until 2026 ?
aallidina-cb
commented
2 years ago I'm debugging something similar on Windows 2003 server at the moment (different context, but similar leak).
Notice the leaking function is always: ExtractAuthRootAutoUpdateCtlFromCab
That's not directly related to the certificate chain - it looks like the code is trying to grab the latest root certificates. These days, that function will fail on older operating systems because the auto certificate updates are signed with SHA256 and older operating systems don't understand this. That appears to trigger an edge case in the code, resulting in a leak.
As a test, could you install the SHA2 patch on Windows 7 and see if the problem resolves?
Here's the KB article: https://support.microsoft.com/en-us/topic/sha-2-code-signing-support-update-for-windows-server-2008-r2-windows-7-and-windows-server-2008-september-23-2019-84a8aad5-d8d9-2d5c-6d78-34f9aa5f8339
The article points to this download site: https://www.catalog.update.microsoft.com/search.aspx?q=kb4474419
aallidina-cb
commented
2 years ago In my case, when I disabled Automatic Root Certificate Updates on my 2003 Server machine, the leak went away. You could also try that on your Windows 7 box.
Description
I find my application memory leak in Windows 7 system which install by
cn_windows_7_ultimate_with_sp1_x64_dvd_u_677408.isoI debug it with WinDbg and gflags.exe tools. And the memory leak call stack is:
And we use the
CRYPT32!CertGetCertificateChaininChainPal.BuildChain, see:https://github.com/dotnet/runtime/blob/c12bea880a2f1290d16adf97ec1000aa63631da2/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/ChainPal.Windows.BuildChain.cs#L66
WinDbg step:
The application get some https urls.
You can find the size of
00420000ranges from 48768 to 81152 .And then I continue run the application. And the application get some https urls...
You can see the
00420000uses more memory.You can see that there are more blocks of size 27994
The alloc call stack as follow:
After I use Fiddler to capture the https, the application no longer has memory leaks.
The dump file is too large for me to upload. And I can share you dump file in private.
Reproduction Steps
cn_windows_7_ultimate_with_sp1_x64_dvd_u_677408.iso.Expected behavior
The application can work well without memory leak.
Actual behavior
The application will memory leak by
CRYPT32!CertGetCertificateChain.Regression?
It work can work well in .NET Core 3.1 and memory leak in .NET 6.
Known Workarounds
No response
Configuration
.NET 6 6.0.4 (also in 6.0.1)
Windows 7 (cn_windows_7_ultimate_with_sp1_x64_dvd_u_677408.iso)
Other information
See IE crashes due to SSL certificate check - Problem with MSVCR80.dll, - Microsoft Community