Skip to content

Commit 741fdc0

Browse files
smcintyre-r7smcintyre-r7
authored
Merge pull request #803 from rapid7/6.5-pre5-resolve-windows-bug
Fix channel closing race condition bug
2 parents 1a8dfb4 + 697345a commit 741fdc0

4 files changed

Lines changed: 86 additions & 21 deletions

File tree

‎c/meterpreter/source/extensions/stdapi/server/net/socket/tcp.c‎

Lines changed: 76 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -108,19 +108,47 @@ DWORD tcp_channel_client_close(Channel *channel, Packet *request, LPVOID context
108108
{
109109
TcpClientContext *ctx = (TcpClientContext *)context;
110110

111-
dprintf( "[TCP] tcp_channel_client_close. channel=0x%08X, ctx=0x%08X", channel, ctx );
111+
dprintf("[TCP] tcp_channel_client_close. channel=%p id=%u ctx=%p", (void*)channel, met_api->channel.get_id(channel), (void*)ctx);
112112

113113
if (ctx)
114114
{
115-
// Set the context channel to NULL so we don't try to close the
116-
// channel (since it's already being closed)
117-
ctx->channel = NULL;
118-
119-
// Free the context
120-
free_tcp_client_context(ctx);
121-
122-
// Set the native channel operations context to NULL
115+
// The framework has told us to close this channel. There are two
116+
// possibilities:
117+
// 1. Only this path is running (no local peer close in flight).
118+
// We must fully tear down the context here.
119+
// 2. tcp_channel_client_local_notify is also running because the
120+
// local peer closed at roughly the same time. In that case we
121+
// must NOT double-free the context: the local-notify thread
122+
// owns cleanup and will observe the closed fd/NULL channel.
123+
//
124+
// Atomically claim ownership of ctx->channel. If we swap a non-NULL
125+
// value, no other thread has started teardown; we own it.
126+
// If we swap NULL, the local-notify path already claimed cleanup;
127+
// we must leave ctx alone.
128+
PVOID prev = InterlockedExchangePointer((PVOID*)&ctx->channel, NULL);
129+
130+
// Detach the native context from the channel regardless, so that
131+
// channel_destroy's subsequent free of `channel` cannot be observed
132+
// via ops->context from any other path.
123133
met_api->channel.set_native_io_context(channel, NULL);
134+
135+
if (prev != NULL)
136+
{
137+
// We won the race: no local-notify teardown is in flight. Free
138+
// the context ourselves.
139+
free_tcp_client_context(ctx);
140+
}
141+
else
142+
{
143+
// The local-notify thread is (or was) handling teardown. Close
144+
// the socket to wake it up in case it's still blocked on recv,
145+
// but do NOT free ctx - that thread owns it.
146+
if (ctx->fd)
147+
{
148+
closesocket(ctx->fd);
149+
ctx->fd = 0;
150+
}
151+
}
124152
}
125153

126154
return ERROR_SUCCESS;
@@ -185,15 +213,44 @@ DWORD tcp_channel_client_local_notify(Remote * remote, TcpClientContext * ctx)
185213

186214
if (dwBytesRead == 0)
187215
{
188-
dprintf("[TCP] tcp_channel_client_local_notify. [closed] channel=0x%08X read=0x%.8x", ctx->channel, dwBytesRead);
189-
190-
// Set the native channel operations context to NULL
191-
met_api->channel.set_native_io_context(ctx->channel, NULL);
216+
// The local peer closed the connection, OR tcp_channel_client_close
217+
// (running on the packet dispatcher thread in response to a framework
218+
// core_channel_close) closed our fd out from under us to wake us up.
219+
//
220+
// Atomically claim ownership of ctx->channel. If we swap a non-NULL
221+
// value the framework hasn't started closing this channel yet, so we
222+
// must notify it. If we swap NULL, the framework is (or was) already
223+
// closing this channel and its tcp_channel_client_close ran; that
224+
// path has left cleanup of ctx to us but is no longer using the
225+
// Channel struct.
226+
Channel *chan = (Channel *)InterlockedExchangePointer((PVOID *)&ctx->channel, NULL);
227+
228+
dprintf("[TCP] tcp_channel_client_local_notify. [closed] chan=%p fd=%llu read=0x%.8x",
229+
(void*)chan, (unsigned long long)ctx->fd, dwBytesRead);
230+
231+
if (chan != NULL)
232+
{
233+
// We won the race. Detach `chan` from ctx first so that a
234+
// core_channel_close request arriving between now and the
235+
// channel_close send below will see ops->context == NULL and
236+
// short-circuit tcp_channel_client_close.
237+
//
238+
// After set_native_io_context, `chan` is still valid: only
239+
// channel_destroy frees the Channel struct, and channel_destroy
240+
// is invoked from a request handler that runs on the packet
241+
// dispatcher thread. That thread has not yet processed our
242+
// upcoming close notification, so `chan` cannot be freed until
243+
// we return from this function and the packet is round-tripped.
244+
met_api->channel.set_native_io_context(chan, NULL);
245+
met_api->channel.close(chan, remote, NULL, 0, NULL);
246+
}
192247

193-
// Sleep for a quarter second
248+
// Sleep briefly to let the framework drain buffered channel data
249+
// before we tear down the underlying socket.
194250
Sleep(250);
195251

196-
// Free the context
252+
// We own cleanup of ctx now; free_socket_context will skip the
253+
// channel.close branch because ctx->channel is NULL.
197254
free_tcp_client_context(ctx);
198255

199256
// Stop processing
@@ -418,6 +475,9 @@ VOID free_socket_context(SocketContext *ctx)
418475
{
419476
dprintf("[TCP] free_socket_context. ctx=0x%08X", ctx);
420477

478+
// Capture fd before closing so it can be logged accurately below
479+
SOCKET originalFd = ctx->fd;
480+
421481
// Close the socket and notification handle
422482
if (ctx->fd)
423483
{
@@ -427,6 +487,7 @@ VOID free_socket_context(SocketContext *ctx)
427487

428488
if (ctx->channel)
429489
{
490+
dprintf("[TCP] free_socket_context. closing channel=%p id=%u fd=%llu", (void*)ctx->channel, met_api->channel.get_id(ctx->channel), (unsigned long long)originalFd);
430491
met_api->channel.close(ctx->channel, ctx->remote, NULL, 0, NULL);
431492
ctx->channel = NULL;
432493
}

‎c/meterpreter/source/extensions/stdapi/server/net/socket/tcp_server.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ VOID free_tcp_server_context(TcpServerContext * ctx)
9090
break;
9191
}
9292

93-
dprintf("[TCP-SERVER] free_tcp_server_context. ctx=0x%08X", ctx);
93+
dprintf("[TCP-SERVER] free_tcp_server_context. ctx=0x%08X channel=0x%08X id=%u fd=%u", ctx, ctx->channel, ctx->channel ? met_api->channel.get_id(ctx->channel) : 0, (DWORD)ctx->fd);
9494

9595
if (ctx->fd)
9696
{
@@ -129,7 +129,7 @@ DWORD tcp_channel_server_close(Channel * channel, Packet * request, LPVOID conte
129129

130130
do
131131
{
132-
dprintf("[TCP-SERVER] tcp_channel_server_close. channel=0x%08X, ctx=0x%08X", channel, ctx);
132+
dprintf("[TCP-SERVER] tcp_channel_server_close. channel=0x%08X id=%u ctx=0x%08X", channel, met_api->channel.get_id(channel), ctx);
133133

134134
if (!ctx)
135135
{

‎c/meterpreter/source/metsrv/channel.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -599,7 +599,7 @@ DWORD channel_write(Channel *channel, Remote *remote, Tlv *addend, DWORD addendL
599599
packet_add_tlv_raw(request, TLV_TYPE_CHANNEL_DATA, buffer, length);
600600
}
601601

602-
packet_add_tlv_uint(request, TLV_TYPE_LENGTH, channel_get_id(channel));
602+
packet_add_tlv_uint(request, TLV_TYPE_LENGTH, length);
603603

604604
// Initialize the packet completion routine
605605
if (completionRoutine)
@@ -628,6 +628,8 @@ DWORD channel_close(Channel *channel, Remote *remote, Tlv *addend,
628628
DWORD res = ERROR_SUCCESS;
629629
Tlv commandIdTlv;
630630

631+
dprintf("[CHANNEL] channel_close ENTER. channel=%p id=%u", (void*)channel, channel ? channel_get_id(channel) : 0);
632+
631633
do
632634
{
633635
Packet *request = packet_create(PACKET_TLV_TYPE_REQUEST, 0);
@@ -658,7 +660,7 @@ DWORD channel_close(Channel *channel, Remote *remote, Tlv *addend,
658660
realRequestCompletion = &requestCompletion;
659661
}
660662

661-
dprintf("[CHANNEL] channel_close. channel=0x%08X completion=0x%.8x", channel, completionRoutine);
663+
dprintf("[CHANNEL] channel_close. channel=0x%08X id=%u completion=0x%.8x", channel, channel ? channel_get_id(channel) : 0, completionRoutine);
662664

663665
// Transmit the packet with the supplied completion routine, if any.
664666
res = packet_transmit(remote, request, realRequestCompletion);

‎gem/Rakefile‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,15 @@ end
9595

9696
task :win_compile do
9797
Dir.chdir(c_source) do
98-
system('cmd.exe /c make.bat')
98+
success = system('cmd.exe /c make.bat')
99+
abort('win_compile failed: make.bat could not be executed or exited with a non-zero status. Check the build output for errors.') unless success
99100
end
100101
end
101102

102103
task :java_compile do
103104
Dir.chdir(java_source) do
104-
system('mvn package -Ddeploy.path=output -Dandroid.sdk.path=$ANDROID_HOME -Dandroid.ndk.path=$ANDROID_NDK_HOME -Dandroid.release=true -q -P deploy')
105+
success = system('mvn package -Ddeploy.path=output -Dandroid.sdk.path=$ANDROID_HOME -Dandroid.ndk.path=$ANDROID_NDK_HOME -Dandroid.release=true -q -P deploy')
106+
abort('java_compile failed: mvn could not be executed or exited with a non-zero status. Check the build output for errors.') unless success
105107
end
106108
end
107109

0 commit comments

Comments
 (0)