[PATCH 1/2] bcrypt: Synchronize symmetric key access for encrypt and decrypt.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- Forza Horizon 4 crashes during entering multiplayer mode due to BCryptEncrypt and BCryptDecrypt being used from different threads with the same key using MODE_ID_GCM. Most of the time that is either glinc free() assertion or segfault inside gnutls function. While bcrypt functions are probably not thread safe in general on Windows, I suppose the native implementation does not modify any key data in encrypt and decrypt in this mode. dlls/bcrypt/bcrypt_internal.h | 1 + dlls/bcrypt/bcrypt_main.c | 16 ++++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/dlls/bcrypt/bcrypt_internal.h b/dlls/bcrypt/bcrypt_internal.h index eb136111509..efa1a463489 100644 --- a/dlls/bcrypt/bcrypt_internal.h +++ b/dlls/bcrypt/bcrypt_internal.h @@ -164,6 +164,7 @@ struct key_symmetric ULONG vector_len; UCHAR *secret; ULONG secret_len; + CRITICAL_SECTION cs; }; #define KEY_FLAG_LEGACY_DSA_V2 0x00000001 diff --git a/dlls/bcrypt/bcrypt_main.c b/dlls/bcrypt/bcrypt_main.c index a1423dcd836..2ef26e81291 100644 --- a/dlls/bcrypt/bcrypt_main.c +++ b/dlls/bcrypt/bcrypt_main.c @@ -1480,6 +1480,7 @@ NTSTATUS WINAPI BCryptGenerateSymmetricKey( BCRYPT_ALG_HANDLE algorithm, BCRYPT_ if (!(block_size = get_block_size( alg ))) return STATUS_INVALID_PARAMETER; if (!(key = heap_alloc_zero( sizeof(*key) ))) return STATUS_NO_MEMORY; + InitializeCriticalSection( &key->u.s.cs ); key->hdr.magic = MAGIC_KEY; key->alg_id = alg->id; key->u.s.mode = alg->mode; @@ -1588,6 +1589,8 @@ static NTSTATUS key_duplicate( struct key *key_orig, struct key *key_copy ) key_copy->u.s.block_size = key_orig->u.s.block_size; key_copy->u.s.secret = buffer; key_copy->u.s.secret_len = key_orig->u.s.secret_len; + + InitializeCriticalSection( &key_copy->u.s.cs ); } else { @@ -1613,6 +1616,7 @@ static void key_destroy( struct key *key ) key_funcs->key_symmetric_destroy( key ); heap_free( key->u.s.vector ); heap_free( key->u.s.secret ); + DeleteCriticalSection( &key->u.s.cs ); } else { @@ -1714,6 +1718,7 @@ NTSTATUS WINAPI BCryptEncrypt( BCRYPT_KEY_HANDLE handle, UCHAR *input, ULONG inp ULONG iv_len, UCHAR *output, ULONG output_len, ULONG *ret_len, ULONG flags ) { struct key *key = handle; + NTSTATUS ret; TRACE( "%p, %p, %u, %p, %p, %u, %p, %u, %p, %08x\n", handle, input, input_len, padding, iv, iv_len, output, output_len, ret_len, flags ); @@ -1730,13 +1735,17 @@ NTSTATUS WINAPI BCryptEncrypt( BCRYPT_KEY_HANDLE handle, UCHAR *input, ULONG inp return STATUS_NOT_IMPLEMENTED; } - return key_encrypt( key, input, input_len, padding, iv, iv_len, output, output_len, ret_len, flags ); + EnterCriticalSection( &key->u.s.cs ); + ret = key_encrypt( key, input, input_len, padding, iv, iv_len, output, output_len, ret_len, flags ); + LeaveCriticalSection( &key->u.s.cs ); + return ret; } NTSTATUS WINAPI BCryptDecrypt( BCRYPT_KEY_HANDLE handle, UCHAR *input, ULONG input_len, void *padding, UCHAR *iv, ULONG iv_len, UCHAR *output, ULONG output_len, ULONG *ret_len, ULONG flags ) { struct key *key = handle; + NTSTATUS ret; TRACE( "%p, %p, %u, %p, %p, %u, %p, %u, %p, %08x\n", handle, input, input_len, padding, iv, iv_len, output, output_len, ret_len, flags ); @@ -1753,7 +1762,10 @@ NTSTATUS WINAPI BCryptDecrypt( BCRYPT_KEY_HANDLE handle, UCHAR *input, ULONG inp return STATUS_NOT_IMPLEMENTED; } - return key_decrypt( key, input, input_len, padding, iv, iv_len, output, output_len, ret_len, flags ); + EnterCriticalSection( &key->u.s.cs ); + ret = key_decrypt( key, input, input_len, padding, iv, iv_len, output, output_len, ret_len, flags ); + LeaveCriticalSection( &key->u.s.cs ); + return ret; } NTSTATUS WINAPI BCryptSetProperty( BCRYPT_HANDLE handle, const WCHAR *prop, UCHAR *value, ULONG size, ULONG flags ) -- 2.30.2
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/bcrypt/tests/bcrypt.c | 87 +++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-) diff --git a/dlls/bcrypt/tests/bcrypt.c b/dlls/bcrypt/tests/bcrypt.c index 456727d04a9..fb5ac03b039 100644 --- a/dlls/bcrypt/tests/bcrypt.c +++ b/dlls/bcrypt/tests/bcrypt.c @@ -862,6 +862,64 @@ static void test_BCryptGenerateSymmetricKey(void) ok(ret == STATUS_SUCCESS, "got %08x\n", ret); } +#define RACE_TEST_COUNT 200 +static LONG encrypt_race_start_barrier; + +static DWORD WINAPI encrypt_race_thread(void *parameter) +{ + static UCHAR nonce[] = + {0x11,0x20,0x30,0x40,0x50,0x60,0x10,0x20,0x30,0x40,0x50,0x60}; + static UCHAR auth_data[] = + {0x61,0x50,0x40,0x30,0x20,0x10,0x60,0x50,0x40,0x30,0x20,0x10}; + static UCHAR data2[] = + {0x00,0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b,0x0c,0x0d,0x0e,0x0f, + 0x10,0x10,0x10,0x10,0x10,0x10,0x10,0x10,0x10,0x10,0x10,0x10,0x10,0x10,0x10,0x10}; + static UCHAR expected4[] = + {0xb2,0x27,0x19,0x09,0xc7,0x89,0xdc,0x52,0x24,0x83,0x3a,0x55,0x34,0x76,0x2c,0xbf, + 0x15,0xa1,0xcb,0x40,0x78,0x11,0xba,0xbc,0xa4,0x76,0x69,0x7c,0x75,0x4f,0x11,0xba}; + static UCHAR expected_tag3[] = + {0xef,0xee,0x75,0x99,0xb8,0x12,0xe9,0xf0,0xb4,0xcc,0x65,0x11,0x67,0x60,0x2d,0xe6}; + + BCRYPT_AUTHENTICATED_CIPHER_MODE_INFO auth_info; + BCRYPT_KEY_HANDLE key = parameter; + UCHAR ciphertext[48], tag[16]; + unsigned int i, test; + NTSTATUS ret; + ULONG size; + + memset(&auth_info, 0, sizeof(auth_info)); + auth_info.cbSize = sizeof(auth_info); + auth_info.dwInfoVersion = 1; + auth_info.pbNonce = nonce; + auth_info.cbNonce = sizeof(nonce); + auth_info.pbTag = tag; + auth_info.cbTag = sizeof(tag); + auth_info.pbAuthData = auth_data; + auth_info.cbAuthData = sizeof(auth_data); + + InterlockedIncrement(&encrypt_race_start_barrier); + while (InterlockedCompareExchange(&encrypt_race_start_barrier, 3, 2) != 2) + ; + + for (test = 0; test < RACE_TEST_COUNT; ++test) + { + size = 0; + memset(ciphertext, 0xff, sizeof(ciphertext)); + memset(tag, 0xff, sizeof(tag)); + ret = pBCryptEncrypt(key, data2, 32, &auth_info, NULL, 0, ciphertext, 32, &size, 0); + ok(ret == STATUS_SUCCESS, "got %08x\n", ret); + ok(size == 32, "got %u\n", size); + ok(!memcmp(ciphertext, expected4, sizeof(expected4)), "wrong data\n"); + ok(!memcmp(tag, expected_tag3, sizeof(expected_tag3)), "wrong tag\n"); + for (i = 0; i < 32; i++) + ok(ciphertext[i] == expected4[i], "%u: %02x != %02x\n", i, ciphertext[i], expected4[i]); + for (i = 0; i < 16; i++) + ok(tag[i] == expected_tag3[i], "%u: %02x != %02x\n", i, tag[i], expected_tag3[i]); + } + + return 0; +} + static void test_BCryptEncrypt(void) { static UCHAR nonce[] = @@ -921,9 +979,10 @@ static void test_BCryptEncrypt(void) BCRYPT_AUTHENTICATED_CIPHER_MODE_INFO auth_info; UCHAR *buf, ciphertext[48], ivbuf[16], tag[16]; BCRYPT_AUTH_TAG_LENGTHS_STRUCT tag_length; + ULONG size, len, i, test; BCRYPT_ALG_HANDLE aes; BCRYPT_KEY_HANDLE key; - ULONG size, len, i; + HANDLE hthread; NTSTATUS ret; ret = pBCryptOpenAlgorithmProvider(&aes, BCRYPT_AES_ALGORITHM, NULL, 0); @@ -1215,6 +1274,32 @@ static void test_BCryptEncrypt(void) ret = pBCryptEncrypt(key, data2, 32, &auth_info, ivbuf, 16, ciphertext, 48, &size, BCRYPT_BLOCK_PADDING); ok(ret == STATUS_INVALID_PARAMETER, "got %08x\n", ret); + /* race test */ + + encrypt_race_start_barrier = 0; + hthread = CreateThread(NULL, 0, encrypt_race_thread, key, 0, NULL); + + while (InterlockedCompareExchange(&encrypt_race_start_barrier, 2, 1) != 1) + ; + + for (test = 0; test < RACE_TEST_COUNT; ++test) + { + size = 0; + memset(ciphertext, 0xff, sizeof(ciphertext)); + memset(tag, 0xff, sizeof(tag)); + ret = pBCryptEncrypt(key, data2, 32, &auth_info, NULL, 0, ciphertext, 32, &size, 0); + ok(ret == STATUS_SUCCESS, "got %08x\n", ret); + ok(size == 32, "got %u\n", size); + ok(!memcmp(ciphertext, expected4, sizeof(expected4)), "wrong data\n"); + ok(!memcmp(tag, expected_tag3, sizeof(expected_tag2)), "wrong tag\n"); + for (i = 0; i < 32; i++) + ok(ciphertext[i] == expected4[i], "%u: %02x != %02x\n", i, ciphertext[i], expected4[i]); + for (i = 0; i < 16; i++) + ok(tag[i] == expected_tag3[i], "%u: %02x != %02x\n", i, tag[i], expected_tag3[i]); + } + + WaitForSingleObject(hthread, INFINITE); + ret = pBCryptDestroyKey(key); ok(ret == STATUS_SUCCESS, "got %08x\n", ret); HeapFree(GetProcessHeap(), 0, buf); -- 2.30.2
On Tue, 2021-04-20 at 13:48 +0300, Paul Gofman wrote:
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- Forza Horizon 4 crashes during entering multiplayer mode due to BCryptEncrypt and BCryptDecrypt being used from different threads with the same key using MODE_ID_GCM. Most of the time that is either glinc free() assertion or segfault inside gnutls function.
While bcrypt functions are probably not thread safe in general on Windows, I suppose the native implementation does not modify any key data in encrypt and decrypt in this mode.
Are you referring to the late initialization of the GnuTLS cipher or does this happen entirely within GnuTLS code?
On 4/20/21 16:13, Hans Leidekker wrote:
On Tue, 2021-04-20 at 13:48 +0300, Paul Gofman wrote:
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- Forza Horizon 4 crashes during entering multiplayer mode due to BCryptEncrypt and BCryptDecrypt being used from different threads with the same key using MODE_ID_GCM. Most of the time that is either glinc free() assertion or segfault inside gnutls function.
While bcrypt functions are probably not thread safe in general on Windows, I suppose the native implementation does not modify any key data in encrypt and decrypt in this mode. Are you referring to the late initialization of the GnuTLS cipher or does this happen entirely within GnuTLS code?
The exact race manifestation might be different, wgat I observed in the real game happens from key_symmetric_set_vector(). Where is can be either working with key->u.s.vector in key_symmetric_set_vector itself (I think that is what is most readily reproduce by my test in the next patch, which always gets the wrong results and only sometimes crashes), or race in gnutls_cipher_deinit() called from key_symmetric_vector_reset (exhibiting itself as a segfault inside gnutls code or assertion from glibc's free). But I think the whole path with the (temporary) key update is racy, including key_symmetric_set_auth_data().
On Tue, 2021-04-20 at 16:19 +0300, Paul Gofman wrote:
On 4/20/21 16:13, Hans Leidekker wrote:
On Tue, 2021-04-20 at 13:48 +0300, Paul Gofman wrote:
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- Forza Horizon 4 crashes during entering multiplayer mode due to BCryptEncrypt and BCryptDecrypt being used from different threads with the same key using MODE_ID_GCM. Most of the time that is either glinc free() assertion or segfault inside gnutls function.
While bcrypt functions are probably not thread safe in general on Windows, I suppose the native implementation does not modify any key data in encrypt and decrypt in this mode. Are you referring to the late initialization of the GnuTLS cipher or does this happen entirely within GnuTLS code?
The exact race manifestation might be different, wgat I observed in the real game happens from key_symmetric_set_vector(). Where is can be either working with key->u.s.vector in key_symmetric_set_vector itself (I think that is what is most readily reproduce by my test in the next patch, which always gets the wrong results and only sometimes crashes), or race in gnutls_cipher_deinit() called from key_symmetric_vector_reset (exhibiting itself as a segfault inside gnutls code or assertion from glibc's free). But I think the whole path with the (temporary) key update is racy, including key_symmetric_set_auth_data().
Unfortunately we can't avoid reinitializing the cipher. It turns out there's support for decryption with asymmetric keys but we failed to remove the FIXME and error return from BCryptDecrypt. I fixed that and rebased your patch.
On 4/20/21 17:54, Hans Leidekker wrote:
On Tue, 2021-04-20 at 16:19 +0300, Paul Gofman wrote:
On 4/20/21 16:13, Hans Leidekker wrote:
On Tue, 2021-04-20 at 13:48 +0300, Paul Gofman wrote:
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- Forza Horizon 4 crashes during entering multiplayer mode due to BCryptEncrypt and BCryptDecrypt being used from different threads with the same key using MODE_ID_GCM. Most of the time that is either glinc free() assertion or segfault inside gnutls function.
While bcrypt functions are probably not thread safe in general on Windows, I suppose the native implementation does not modify any key data in encrypt and decrypt in this mode. Are you referring to the late initialization of the GnuTLS cipher or does this happen entirely within GnuTLS code?
The exact race manifestation might be different, wgat I observed in the real game happens from key_symmetric_set_vector(). Where is can be either working with key->u.s.vector in key_symmetric_set_vector itself (I think that is what is most readily reproduce by my test in the next patch, which always gets the wrong results and only sometimes crashes), or race in gnutls_cipher_deinit() called from key_symmetric_vector_reset (exhibiting itself as a segfault inside gnutls code or assertion from glibc's free). But I think the whole path with the (temporary) key update is racy, including key_symmetric_set_auth_data(). Unfortunately we can't avoid reinitializing the cipher.
That's what I assumed and thus suggested to just sync the access in such scenario.
It turns out there's support for decryption with asymmetric keys but we failed to remove the FIXME and error return from BCryptDecrypt. I fixed that and rebased your patch.
Thanks.
participants (2)
-
Hans Leidekker -
Paul Gofman