Improve TLS 1.3/1.2 Key Capture & Scylla ATL-Removal Build Cleanup#132
Improve TLS 1.3/1.2 Key Capture & Scylla ATL-Removal Build Cleanup#132wmetcalf wants to merge 5 commits into
Conversation
Completes the TLS 1.3 traffic-secret extraction that was scaffolded in 84d5f71 but left unimplemented, adds in-process TLS 1.2/1.3 coverage for WinHTTP/.NET samples that use Schannel-CNG directly (bypassing ncryptsslp.dll), and fixes a concurrent-write race in LogTls where FILE_SHARE_READ-only sharing mode prevented multi-process key capture.
There was a problem hiding this comment.
Code Review
This pull request introduces support for TLS 1.3 and improves TLS 1.2 secret capturing in modern Windows environments by adding hooks for BCryptHashData, BCryptKeyDerivation, and BCryptDeriveKey. It also replaces ATL-based string conversion with manual implementations using Windows APIs. Feedback highlights several critical thread-safety issues, including the use of a global buffer for logging, race conditions in file handle initialization, and non-thread-safe lookup table operations. Additionally, a logic error was identified in the new string conversion functions regarding UTF-8 buffer size calculations.
| memset(SecretsLine, 0, BUFFER_SIZE); | ||
| _snprintf_s(SecretsLine, BUFFER_SIZE, _TRUNCATE, "%s %s %s\n", Label, ClientRandomRepr, SecretRepr); | ||
| WriteFile(TlsLog, SecretsLine, (DWORD)strlen(SecretsLine), (LPDWORD)&LastWriteLength, NULL); |
There was a problem hiding this comment.
The use of the global buffer SecretsLine is not thread-safe. In a multi-threaded environment, concurrent calls to LogTls or LogTls13 from different threads will lead to data corruption in this shared buffer. This is particularly critical when monitoring modern browsers or servers that utilize multiple threads for TLS operations. This buffer should be moved to the stack within each logging function to ensure thread isolation.
if (TlsLog != INVALID_HANDLE_VALUE) {
char SecretsLine[BUFFER_SIZE];
memset(SecretsLine, 0, BUFFER_SIZE);
_snprintf_s(SecretsLine, BUFFER_SIZE, _TRUNCATE, "%s %s %s\n", Label, ClientRandomRepr, SecretRepr);
WriteFile(TlsLog, SecretsLine, (DWORD)strlen(SecretsLine), (LPDWORD)&LastWriteLength, NULL);
}| if (!TlsLog) | ||
| TlsLog = CreateFile(FullPathName, FILE_APPEND_DATA, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); |
There was a problem hiding this comment.
The initialization of the global TlsLog handle is subject to a race condition. Multiple threads entering this block simultaneously could each call CreateFile, resulting in handle leaks and potential file access conflicts. While the use of FILE_APPEND_DATA helps with atomic writes, the handle management itself should be synchronized (e.g., using InterlockedCompareExchangePointer or a critical section).
| stage = *pStage; | ||
| *pStage = stage + 1; | ||
| } else { | ||
| pStage = (int*)lookup_add(&ThreadTLS13Stage, tid, sizeof(int)); |
There was a problem hiding this comment.
The lookup_add function and the underlying lookup_t system are not thread-safe. Since ThreadTLS13Stage (and other lookup tables like ThreadTLS13ClientRandom) are global objects accessed from hooks across multiple threads, concurrent calls to lookup_add will result in race conditions when updating the linked list root. This can lead to memory leaks, lost entries, or even crashes due to corrupted pointers. Access to these shared lookup tables must be protected by a synchronization primitive.
| while (str[len] && len < bufsize - 1) len++; | ||
|
|
||
| int written = WideCharToMultiByte(CP_UTF8, 0, str, (int)len, buf, (int)bufsize - 1, NULL, NULL); |
There was a problem hiding this comment.
The logic for calculating len and calling WideCharToMultiByte is problematic for non-ASCII characters. By limiting the input length len to bufsize - 1 based on the number of wchar_t, the code assumes a 1:1 mapping between UTF-16 and UTF-8. However, a single UTF-16 character can result in up to 3 bytes in UTF-8 (or 4 bytes for surrogate pairs). If the converted sequence exceeds the destination buffer size, WideCharToMultiByte will fail and return 0, causing the function to return an empty string. This is a regression from the previous ATL-based implementation which correctly handled truncation. Consider using a larger intermediate buffer or calling the API in a way that allows for graceful truncation.
…F-16 boundary-aware graceful truncation
Replace the per-thread g_stage_root / g_client_random_root / g_tls13_client_random_root linked lists (and their SRWLOCKs and get/add/del helpers introduced in 0c6e76e) with plain __declspec(thread) variables. Same per-thread semantics, no manual lookup or locking, no leaked entries on thread exit (the OS reclaims TLS automatically). - t_stage replaces ThreadTLS13Stage / tls_stage_* - t_client_random (+ t_client_random_valid) replaces ThreadClientRandom / tls_client_random_* - t_tls13_client_random (+ t_tls13_client_random_valid) replaces ThreadTLS13ClientRandom / tls13_client_random_* Preserves upstream semantics: - SslHashHandshake (TLS 1.2) stashes client_random only if one has not already been captured for the thread (was `if (R == NULL)`), and resets t_stage to 0 on every ClientHello. - BCryptHashData (TLS 1.3) remains unchanged behaviorally: always HexEncodes the current ClientHello's client_random (was unconditional HexEncode after get-or-create). Drops the never-written ServerRandomRepr field from ThreadRandom. Validated live on a CAPE host: TLS 1.2 master_secret and the full TLS 1.3 secret set (CLIENT/SERVER_HANDSHAKE_TRAFFIC_SECRET, CLIENT/SERVER_TRAFFIC_SECRET_0, EXPORTER_SECRET) are captured identically to the lookup-table version against the same baseline samples. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Overview
This pull request brings robust support for TLS 1.3 traffic secret capture, implements TLS 1.2 master secret extraction for modern Windows Schannel CNG bypass paths, and removes the Visual Studio build environment's dependency on the Active Template Library (ATL).
Detailed Changes
1. In-Process TLS 1.3 Key Capture (
hook_tls.c)Adds complete, bitness-aware support for extracting TLS 1.3 handshake and application secrets from in-process NCrypt structure hierarchies (mapping pointers across
BDDD->3lss->RUUU->YKSMlayouts for both x86 and x64):BCryptHashDataand stashes it.BCryptKeyDerivationto intercept serialized HKDF labels (HkdfLabelper RFC 8446 §7.1) during HKDF-Expand-Label calls.CLIENT_HANDSHAKE_TRAFFIC_SECRET,SERVER_HANDSHAKE_TRAFFIC_SECRET,CLIENT_TRAFFIC_SECRET_0,SERVER_TRAFFIC_SECRET_0, andEXPORTER_SECRET).SSLKEYLOGFILEformat totlsdump.logso Wireshark/editcap can cleanly decrypt traffic.2. In-Process TLS 1.2 PRF Extraction (
hook_tls.c)Hooks
BCryptDeriveKeyto intercept "TLS_PRF" master secret derivations. This adds coverage for modern Windows 10/11 WinHTTP and newer .NET HttpClient implementations that leverage Schannel-CNG directly and bypassncryptsslp.dll.3. Scylla ATL Dependency Removal (
CAPE/Scylla/StringConversion.cpp)Removes references to
atlbase.handatlconv.h(ATL::CW2AandATL::CA2W) in Scylla'sStringConversion. It implements standard character conversions using native Win32MultiByteToWideCharandWideCharToMultiByteAPIs withCP_UTF8. This ensurescapemoncompiles out-of-the-box on clean developer machines without requiring the ATL component in Visual Studio.Verification
Release\capemon.dllx64\Release\capemon_x64.dll