@nickzman can you see if you can make anything from this?

Copy that; I'll see if I can reproduce this.

I can't reproduce the problem with the example provided; it properly exits without finding the certificate. I'm scanning the code, and nothing really looks like it would obviously cause the problem. Can you still reproduce the problem with the latest code?

Thanks for taking a look @nickzman !

Here's a more "plug and play" sample which reproduces the crash on for me. You should only have to run repro.sh, and it will fetch the latest versions of curl and nghttp2, build the sample program and run it.

repro-curl-9194.zip

(So to answer your question, I can reproduce using libcurl 7.84.0 and nghttp2 1.48.0)

Here's a video of me running it on my machine :

I ran the script, but still could not reproduce the problem on my M1 Mac running the latest macOS release.

What version of Clang are you using? I'm using Apple Clang 13.1.6, which comes with Xcode 13.4.1.

@nickzman

❯ xcrun clang --version
Apple clang version 13.1.6 (clang-1316.0.21.2.5)
Target: arm64-apple-darwin21.5.0
Thread model: posix
InstalledDir: /Applications/Xcode-13.4.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Also here's the full log of my repro.sh script running on my machine :

out.log

Ok so I have 2 coworkers (Intel machines) who are crashing like me, and a coworker (Apple Silicon) not crashing and getting the error message like you are.

I'll compare our configs / build logs and report back.

Some news :

I could not find any meaningful differences between my coworker's build logs and mine.

After a bit more debugging in lldb, it appears the bug is caused by SecCertificateCopyCommonName() (sectransp.c:1174) returning a code -26276, which I could not find a mention of in the official doc, but is said to be errSecInternal "An internal error occured in the Security framework." by osstatus.com.

Apparently in that case common_name will be set (kept) to NULL, and thus the subsequent call to CFRelease(common_name) will crash.

That doesn't looks too good to me. However that doesn't explain why it works on some machines on not others (mine 😭).

It's starting to look more and more like an Apple bug, though, so I'm not sure if I should continue to bother you guys about it. If you have any pointer on how I could possibly debug this further I'd appreciate it, but either way thank you already for the time you have spent on this issue 👍🏻

I'll probably file a radar if I can reduce the scope of the issue further (not having Apple engineers build nghttp2 and curl to reproduce would be nice), but I think I already know how this will end...

Could this be worked around in curl by checking if common_name is NULL, and if it is, skipping the CFRelease call?

Does this still happen with a current curl?

@bagder yes, I just reproduced the crash with the latest version of curl (8.2.1) and nghttp2 (1.55.1) targeting macOS 13.

Here's and updated version of my repro script : repro-curl-9194-20230728.zip

This actually seems to be a security vulnerability (denial of service) in either libcurl or Secure Transport.

Does this simple patch fix it?

diff --git a/lib/vtls/sectransp.c b/lib/vtls/sectransp.c
index 348bbe202..1c72ed626 100644
--- a/lib/vtls/sectransp.c
+++ b/lib/vtls/sectransp.c
@@ -1147,11 +1147,12 @@ static OSStatus CopyIdentityWithLabel(char *label,
             CFRetain(identity);
             *out_cert_and_key = identity;
             status = noErr;
             break;
           }
-          CFRelease(common_name);
+          if(common_name)
+            CFRelease(common_name);
         }
         CFRelease(cert);
       }
     }
 

@bagder with the patch it's better in some configurations, but I'm still crashing with --disable-debug --enable-optimize (which is the configuration I'd like to use) :

--disable-debug
--enable-optimize

--enable-debug
--enable-optimize

--disable-debug
--disable-optimize

--disable-debug
--enable-optimize

--enable-debug
--enable-optimize

--disable-debug
--disable-optimize

When configuring with --disable-debug --enable-optimize (and with the patch), the crash I'm seeing is :

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x20)
    frame #0: 0x00000001a06662a0 libobjc.A.dylib`lookUpImpOrForward + 80
    frame #1: 0x00000001a0665f64 libobjc.A.dylib`_objc_msgSend_uncached + 68
  * frame #2: 0x00000001000383a4 a.out`sectransp_connect_common + 5400
    frame #3: 0x000000010003aee0 a.out`ssl_cf_connect + 584
    frame #4: 0x0000000100009740 a.out`cf_setup_connect + 92
    frame #5: 0x000000010000404c a.out`cf_hc_connect + 748
    frame #6: 0x0000000100007264 a.out`Curl_conn_connect + 76
    frame #7: 0x0000000100026430 a.out`multi_runsingle + 956
    frame #8: 0x0000000100025fe4 a.out`curl_multi_perform + 92
    frame #9: 0x00000001000107ac a.out`curl_easy_perform + 260
    frame #10: 0x00000001000010a8 a.out`main at sample.c:27:20
    frame #11: 0x00000001a06a7f28 dyld`start + 2236

But because I don't have the debug symbols in this case, I'm struggling to understand which part of the code causes the segfault. My lldb debugging skills are a bit rusty ; a .dSYM file is generated and seems to be loaded by lldb, but it looks like the crash is not fully symbolicated since I don't get the files and lines numbers? Any advice?

Frame 2 says it is now inside sectransp_connect_common. Can you add some fprintf() calls or something in there to help us spot where exactly it goes wrong?

@bagder after digging, it seems like the lldb backtrace was incomplete, and the problem is still at the same place (on the CFRelease(common_name); line).

I think the problem comes from the fact that when testing whether common_name is NULL before releasing it, the pointer is not NULL but some garbage value. The pointer is never initialized (it's simply declared as CFStringRef common_name;).

I'm guessing that when curl is compiled with debug flags the compiler will not optimize things (?) and will take the time to initialize common_name to NULL. So we're not releasing it when SecCertificateCopyCommonName fails.
When using --disable-debug --enable-optimize however, the pointer keeps the garbage value that was stored there before, and because SecCertificateCopyCommonName seems not to touch our pointer either, we're releasing this memory garbage (and crashing).

So the crash is fixed with :

diff --git a/lib/vtls/sectransp.c b/lib/vtls/sectransp.c
index 32bb3a5..0391baf 100644
--- a/lib/vtls/sectransp.c
+++ b/lib/vtls/sectransp.c
@@ -1086,7 +1086,7 @@ static OSStatus CopyIdentityWithLabel(char *label,
   CFArrayRef keys_list;
   CFIndex keys_list_count;
   CFIndex i;
-  CFStringRef common_name;
+  CFStringRef common_name = NULL;

   /* SecItemCopyMatching() was introduced in iOS and Snow Leopard.
      kSecClassIdentity was introduced in Lion. If both exist, let's use them
@@ -1149,7 +1149,8 @@ static OSStatus CopyIdentityWithLabel(char *label,
             status = noErr;
             break;
           }
-          CFRelease(common_name);
+          if(common_name)
+            CFRelease(common_name);
         }
         CFRelease(cert);
       }

I have updated the PR with (a slight variation of) this.