Skip to content
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.

Sign up for GitHub

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

Jump to bottom

dotnet OpenSSL double free with Ubuntu 24.04 #109689

Open
pepone opened this issue Nov 10, 2024 · 9 comments · May be fixed by #113124
Open

dotnet OpenSSL double free with Ubuntu 24.04 #109689

pepone opened this issue Nov 10, 2024 · 9 comments · May be fixed by #113124
Assignees
@rzikm
Labels
area-System.Net.Security in-pr There is an active PR which will close this issue when it is merged
Milestone
10.0.0

Comments

@pepone
Copy link
Contributor

pepone commented Nov 10, 2024

We are experiencing intermittent test failures involving SSL with zeroc-ice/ice for .NET. After investigating, I traced the issue to crashes in the .NET process, and I managed to collect a core dump:

#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007f9495a4526e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007f9495a2898f in __GI_abort () at ./stdlib/abort.c:100
#5  0x00007f9495a297b6 in __libc_message_impl (fmt=fmt@entry=0x7f9495bce8d7 "%s\n") at ../sysdeps/posix/libc_fatal.c:132
#6  0x00007f9495aa8fe5 in malloc_printerr (str=str@entry=0x7f9495bd1bf0 "free(): double free detected in tcache 2") at ./malloc/malloc.c:5772
#7  0x00007f9495aab54f in _int_free (av=0x7f530c000030, p=<optimized out>, have_lock=0) at ./malloc/malloc.c:4541
#8  0x00007f9495aadd9e in __GI___libc_free (mem=0x7f530c04d960) at ./malloc/malloc.c:3398
#9  0x00007f94048d5685 in ossl_asn1_string_embed_free (a=0x7f530c036ba8, embed=4096) at ../crypto/asn1/asn1_lib.c:364
#10 0x00007f94048e210f in ossl_asn1_primitive_free (pval=<optimized out>, it=<optimized out>, embed=<optimized out>) at ../crypto/asn1/tasn_fre.c:204
#11 0x00007f94048e2572 in ossl_asn1_template_free (pval=0x7f94073febe8, tt=tt@entry=0x7f9404cfd830 <X509_seq_tt+80>) at ../crypto/asn1/tasn_fre.c:142
#12 0x00007f94048e22a9 in ossl_asn1_item_embed_free (pval=pval@entry=0x7f94073fec98, it=0x7f9404cd7c40 <local_it>, embed=embed@entry=0) at ../crypto/asn1/tasn_fre.c:110
#13 0x00007f94048e247b in ASN1_item_free (val=<optimized out>, it=<optimized out>) at ../crypto/asn1/tasn_fre.c:20
#14 0x00007f9404b4d629 in OPENSSL_sk_pop_free (st=0x7f530c04d690, func=0x7f9404b8e660 <X509_free>) at ../crypto/stack/stack.c:426
#15 0x00007f9404b84f0e in X509_STORE_CTX_cleanup (ctx=0x7f530c0509e0) at ../crypto/x509/x509_vfy.c:2486
#16 0x00007f9493ccbe75 in CryptoNative_X509StoreCtxReset () from /home/vscode/.dotnet/shared/Microsoft.NETCore.App/8.0.10/libSystem.Security.Cryptography.Native.OpenSsl.so
#17 0x00007f941790fc38 in ?? ()
#18 0x00007f94073fed40 in ?? ()
#19 0x00000000012f2668 in ?? ()
#20 0x00007f94958723c8 in ?? () from /home/vscode/.dotnet/shared/Microsoft.NETCore.App/8.0.10/libcoreclr.so
#21 0x00007f94073ffcc0 in ?? ()
#22 0x0000000000000000 in ?? ()
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Nov 10, 2024
@pepone pepone mentioned this issue Nov 11, 2024
Open
@janvorli
Copy link
Member

cc: @rzikm

@rzikm
Copy link
Member

rzikm commented Nov 11, 2024

@pepone Would it be possible to share one of the dumps or create a minimal reproducible application which we can run locally to investigate the issue?

@rzikm rzikm added the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 11, 2024
@pepone
Copy link
Contributor Author

pepone commented Nov 11, 2024

@rzikm

Would it be possible to share one of the dumps

I can upload the core, it is 247 MB just let me know where to upload.

or create a minimal reproducible application which we can run locally to investigate the issue?

That might be more difficult. For a non minimal test case you can use our repository and the provided dev container.

It is just a matter of running make in the csharp subdirectory and then run python allTests.py IceSSL/configuration --loop it will eventually crash.

@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 11, 2024
@rzikm
Copy link
Member

rzikm commented Nov 13, 2024 •
edited
Loading

@pepone

It is just a matter of running make in the csharp subdirectory and then run python allTests.py IceSSL/configuration --loop it will eventually crash.

It is currently running at 700+ iterations without crash, so getting a dump from you might speed up the process.

I can upload the core, it is 247 MB just let me know where to upload.

If there is no sensitive data involved, you can upload it to a cloud storage of your choice and send me a link to my work email (radekzikmund (at) microsoft.com). Or reach out to me and I will send you a one drive link where you can upload the dump.

@pepone
Copy link
Contributor Author

pepone commented Nov 13, 2024

@rzikm attached a core dump to this issue zeroc-ice/ice#1745 (comment)

@jeffhandley
Copy link
Member

Assigned to @krwq to finish triaging this.

@krwq
Copy link
Member

krwq commented Jan 16, 2025

Talked with @rzikm offline. We'll put this in System.Net.Security for now so he can look at this further

@krwq krwq assigned rzikm and unassigned krwq Jan 16, 2025
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@wfurt wfurt removed the untriaged New issue has not been triaged by the area owner label Jan 16, 2025
@wfurt wfurt added this to the 10.0.0 milestone Jan 16, 2025
@rzikm rzikm linked a pull request Mar 4, 2025 that will close this issue
Open
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Mar 4, 2025
@rzikm
Copy link
Member

rzikm commented Mar 4, 2025 •
edited
Loading

Sorry, I was busy with other commitments, finally got time to figure this out.

Your test code disposes SslStream sometime during handshake, or immediately afterwards, so at some point it may read garbage data here

runtime/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs

Lines 57 to 76 in 4a8a95f

using (SafeSharedX509StackHandle chainStack =
Interop.OpenSsl.GetPeerCertificateChain((SafeSslHandle)securityContext))
{
if (!chainStack.IsInvalid)
{
int count = Interop.Crypto.GetX509StackFieldCount(chainStack);
for (int i = 0; i < count; i++)
{
IntPtr certPtr = Interop.Crypto.GetX509StackField(chainStack, i);
if (certPtr != IntPtr.Zero)
{
// X509Certificate2(IntPtr) calls X509_dup, so the reference is appropriately tracked.
X509Certificate2 chainCert = new X509Certificate2(certPtr);
chain.ChainPolicy.ExtraStore.Add(chainCert);
}
}
}
}

The chainStack safe handle does not keep the underlying data alive, as it is an interior pointer to containing SSL structure being kept alive by handle securityContext (which get's closed on Dispose()). Disposing the SslStream instance while within this block will cause the crash (sooner or later).

While SslStream should not be used concurrently from multiple threads, we also should ensure that such usage does not crash applications.

The fix is to link the chainStack to parent handle to make sure we don't free the memory too early (as per existing interior safe handle pattern).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rzikm rzikm

Labels
area-System.Net.Security in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Milestone
10.0.0
Development

Successfully merging a pull request may close this issue.

Link peer's X509 stack handle to parent SSL safe handle rzikm/dotnet-runtime
6 participants
@pepone @krwq @jeffhandley @janvorli @wfurt @rzikm