From f39e05a73c5b5394caf493ee1205af9b64024a47 Mon Sep 17 00:00:00 2001 From: Juan Sebastian Hoyos Ayala Date: Mon, 12 Jun 2023 17:47:29 +0000 Subject: [PATCH 1/2] [release/7.0] Change NamedPipe creation flags --- src/native/eventpipe/ds-ipc-pal-namedpipe.c | 117 ++++++++++++++++++-- src/native/eventpipe/ds-ipc-pal-socket.c | 5 +- src/native/eventpipe/ds-ipc-pal.h | 2 +- src/native/eventpipe/ds-server.c | 2 +- 4 files changed, 115 insertions(+), 11 deletions(-) diff --git a/src/native/eventpipe/ds-ipc-pal-namedpipe.c b/src/native/eventpipe/ds-ipc-pal-namedpipe.c index 81a0499e56f948..1bd3a202277204 100644 --- a/src/native/eventpipe/ds-ipc-pal-namedpipe.c +++ b/src/native/eventpipe/ds-ipc-pal-namedpipe.c @@ -57,6 +57,8 @@ ep_rt_object_free (void *ptr) } #endif /* !FEATURE_PERFTRACING_STANDALONE_PAL */ +static HANDLE _ipc_listen_ownership_handle = INVALID_HANDLE_VALUE; + /* * Forward declares of all static functions. */ @@ -91,6 +93,18 @@ static bool ipc_stream_close_func (void *object); +static +void +ipc_close_ownership_handle ( + ds_ipc_error_callback_func callback); + +static +bool +ipc_createpipe_helper ( + DiagnosticsIpc *ipc, + bool ensure_pipe_creation, + ds_ipc_error_callback_func callback); + static DiagnosticsIpcStream * ipc_stream_alloc ( @@ -108,8 +122,9 @@ ds_ipc_pal_init (void) } bool -ds_ipc_pal_shutdown (void) +ds_ipc_pal_shutdown (ds_ipc_error_callback_func callback) { + ipc_close_ownership_handle(callback); return true; } @@ -330,9 +345,11 @@ ds_ipc_poll ( ep_exit_error_handler (); } +static bool -ds_ipc_listen ( +ipc_createpipe_helper ( DiagnosticsIpc *ipc, + bool ensure_pipe_creation, ds_ipc_error_callback_func callback) { bool result = false; @@ -348,16 +365,41 @@ ds_ipc_listen ( if (ipc->is_listening) return true; - EP_ASSERT (ipc->pipe == INVALID_HANDLE_VALUE); + if (!ensure_pipe_creation && _ipc_listen_ownership_handle == INVALID_HANDLE_VALUE) + { + if (callback) + callback ("Can't ensure we have ownership of the pipe. Disallowing creation.", -1); + return false; + } + + if (ensure_pipe_creation && _ipc_listen_ownership_handle != INVALID_HANDLE_VALUE) + { + if (callback) + callback ("Inconsistent state - pipe sentinel already in use for listen.", -1); + return false; + } + + EP_ASSERT (ipc->pipe == INVALID_HANDLE_VALUE); const uint32_t in_buffer_size = 16 * 1024; const uint32_t out_buffer_size = 16 * 1024; + DWORD creationFlags = PIPE_ACCESS_DUPLEX // read/write access + | FILE_FLAG_OVERLAPPED; // async listening. + + if (ensure_pipe_creation) + { + // Fail if we can't own pipe. This is the only way to ensure + // ownership of the pipe, and by extension the default DACL. + // Otherwise, Windows treats this as a FIFO queue get-or-create + // request and we might end up with DACLs set by other creators. + creationFlags |= FILE_FLAG_FIRST_PIPE_INSTANCE; + } + DS_ENTER_BLOCKING_PAL_SECTION; ipc->pipe = CreateNamedPipeA ( ipc->pipe_name, // pipe name - PIPE_ACCESS_DUPLEX | // read/write access - FILE_FLAG_OVERLAPPED, // async listening + creationFlags, PIPE_TYPE_BYTE | PIPE_WAIT | PIPE_REJECT_REMOTE_CLIENTS, // message type pipe, message-read and blocking mode PIPE_UNLIMITED_INSTANCES, // max. instances out_buffer_size, // output buffer size @@ -372,6 +414,28 @@ ds_ipc_listen ( ep_raise_error (); } + if (ensure_pipe_creation) + { + EP_ASSERT (_ipc_listen_ownership_handle == INVALID_HANDLE_VALUE); + + // The dupe and leak of the handle to ensure listen EP ownership for process duration. + bool createdSentinel = DuplicateHandle( + GetCurrentProcess(), + ipc->pipe, + GetCurrentProcess(), + &_ipc_listen_ownership_handle, + 0, + FALSE, + DUPLICATE_SAME_ACCESS); + + if (!createdSentinel) + { + if (callback) + callback ("Failed to ownership sentinel.", GetLastError()); + ep_raise_error(); + } + } + EP_ASSERT (ipc->overlap.hEvent == INVALID_HANDLE_VALUE); ipc->overlap.hEvent = CreateEventW (NULL, true, false, NULL); @@ -407,10 +471,23 @@ ds_ipc_listen ( ep_on_error: ds_ipc_close (ipc, false, callback); + if (ensure_pipe_creation) + ipc_close_ownership_handle(callback); result = false; ep_exit_error_handler (); } +bool +ds_ipc_listen ( + DiagnosticsIpc *ipc, + ds_ipc_error_callback_func callback) +{ + // This is the first time that this listening channel is created + // from the perspective of the runtime. Request we ensure that we create + // the pipe. + return ipc_createpipe_helper(ipc, true, callback); +} + DiagnosticsIpcStream * ds_ipc_accept ( DiagnosticsIpc *ipc, @@ -459,7 +536,10 @@ ds_ipc_accept ( memset(&ipc->overlap, 0, sizeof(OVERLAPPED)); // clear the overlapped objects state ipc->overlap.hEvent = INVALID_HANDLE_VALUE; - ep_raise_error_if_nok (ds_ipc_listen (ipc, callback)); + // At this point we have at least one open connection with this pipe, + // so this listen pipe won't recreate the named pipe and thus inherit + // all the necessary DACLs from the original listen call. + ep_raise_error_if_nok (ipc_createpipe_helper (ipc, false, callback)); ep_on_exit: return stream; @@ -526,6 +606,27 @@ ds_ipc_connect ( ep_exit_error_handler (); } +void +ipc_close_ownership_handle ( + ds_ipc_error_callback_func callback) +{ + if (_ipc_listen_ownership_handle == INVALID_HANDLE_VALUE) + return; + + const BOOL success_close_pipe = CloseHandle(_ipc_listen_ownership_handle); + if (success_close_pipe != TRUE) + { + if (callback) + callback ("Failed to IPC ownership sentinel handle", GetLastError()); + // Explicitly don't reset it. Failing to close and setting it to an invalid handle + // leaks the handle in a way we can't diagnose anything. Leaving it rooted helps us + // assert state consistency. + return; + } + + _ipc_listen_ownership_handle = INVALID_HANDLE_VALUE; +} + void ds_ipc_close ( DiagnosticsIpc *ipc, @@ -535,7 +636,9 @@ ds_ipc_close ( EP_ASSERT (ipc != NULL); // don't attempt cleanup on shutdown and let the OS handle it - if (is_shutdown) { + // except in the case of listen pipes - if they leak the process + // will fail to reinitialize the pipe for embedding scenarios. + if (is_shutdown && ipc->mode != DS_IPC_CONNECTION_MODE_LISTEN) { if (callback) callback ("Closing without cleaning underlying handles", 100); return; diff --git a/src/native/eventpipe/ds-ipc-pal-socket.c b/src/native/eventpipe/ds-ipc-pal-socket.c index d7aa4d0ea4eb78..c53b8fe54dcf59 100644 --- a/src/native/eventpipe/ds-ipc-pal-socket.c +++ b/src/native/eventpipe/ds-ipc-pal-socket.c @@ -1009,11 +1009,12 @@ ds_ipc_pal_init (void) } bool -ds_ipc_pal_shutdown (void) +ds_ipc_pal_shutdown (ds_ipc_error_callback_func callback) { #ifdef HOST_WIN32 if (_ipc_pal_socket_init) - WSACleanup (); + if (WSACleanup() == SOCKET_ERROR && callback) + callback ("Failed to cleanup Winsock", WSAGetLastError()); #endif _ipc_pal_socket_init = false; return true; diff --git a/src/native/eventpipe/ds-ipc-pal.h b/src/native/eventpipe/ds-ipc-pal.h index 98e0fba180e69b..8ffd202b6b5185 100644 --- a/src/native/eventpipe/ds-ipc-pal.h +++ b/src/native/eventpipe/ds-ipc-pal.h @@ -21,7 +21,7 @@ bool ds_ipc_pal_init (void); bool -ds_ipc_pal_shutdown (void); +ds_ipc_pal_shutdown (ds_ipc_error_callback_func callback); int32_t ds_ipc_get_handle_int32_t (DiagnosticsIpc *ipc); diff --git a/src/native/eventpipe/ds-server.c b/src/native/eventpipe/ds-server.c index 5185bc09f49f54..c94d172f07101c 100644 --- a/src/native/eventpipe/ds-server.c +++ b/src/native/eventpipe/ds-server.c @@ -247,7 +247,7 @@ ds_server_shutdown (void) ds_ipc_stream_factory_shutdown (server_error_callback_close); ds_ipc_stream_factory_fini (); - ds_ipc_pal_shutdown (); + ds_ipc_pal_shutdown (server_error_callback_close); return true; } From f0bc95760e18c725dcddee3227b47db50806d5d4 Mon Sep 17 00:00:00 2001 From: Jeff Handley Date: Tue, 20 Jun 2023 18:28:40 +0000 Subject: [PATCH 2/2] Merged PR 32088: Merged PR 32017: [7.0] Fix regression loading null-password encrypted PFX certificates When decrypting the payload with empty string and null passwords, try reading the payload with the Asn reader to ensure the header matches the expected format. If that succeeds, then proceed with the iteration counting. This guards against a false-positive match that previously caused our iteration count work to throw/abort, thus preventing some null-password encrypted payloads from being loaded. --- .../Cryptography/Asn1/Pkcs12/PfxAsn.manual.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/libraries/Common/src/System/Security/Cryptography/Asn1/Pkcs12/PfxAsn.manual.cs b/src/libraries/Common/src/System/Security/Cryptography/Asn1/Pkcs12/PfxAsn.manual.cs index b20fa4e016073c..b934884323724d 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/Asn1/Pkcs12/PfxAsn.manual.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/Asn1/Pkcs12/PfxAsn.manual.cs @@ -249,6 +249,12 @@ private static ArraySegment DecryptContentInfo(ContentInfoAsn contentInfo, default, encryptedData.EncryptedContentInfo.EncryptedContent.Value.Span, destination); + + // When padding happens to be as expected (false-positive), we can detect gibberish and prevent unexpected failures later + // This extra check makes it so it's very unlikely we'll end up with false positive. + AsnValueReader outerSafeBag = new AsnValueReader(destination.AsSpan(0, written), AsnEncodingRules.BER); + AsnValueReader safeBagReader = outerSafeBag.ReadSequence(); + outerSafeBag.ThrowIfNotEmpty(); } catch { @@ -259,6 +265,12 @@ private static ArraySegment DecryptContentInfo(ContentInfoAsn contentInfo, default, encryptedData.EncryptedContentInfo.EncryptedContent.Value.Span, destination); + + // When padding happens to be as expected (false-positive), we can detect gibberish and prevent unexpected failures later + // This extra check makes it so it's very unlikely we'll end up with false positive. + AsnValueReader outerSafeBag = new AsnValueReader(destination.AsSpan(0, written), AsnEncodingRules.BER); + AsnValueReader safeBagReader = outerSafeBag.ReadSequence(); + outerSafeBag.ThrowIfNotEmpty(); } } finally