-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix metasploit-framework#15968 (Multiple Socket Abstraction Bugs) #20
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,9 @@ def initialize_abstraction | |
# | ||
def cleanup_abstraction | ||
lsock.close if lsock and !lsock.closed? | ||
|
||
monitor_thread.join if monitor_thread&.alive? | ||
|
||
rsock.close if rsock and !rsock.closed? | ||
|
||
self.lsock = nil | ||
|
@@ -111,19 +114,36 @@ def localinfo | |
# | ||
attr_reader :rsock | ||
|
||
module MonitoredRSock | ||
def close | ||
@close_requested = true | ||
@monitor_thread.join | ||
nil | ||
end | ||
|
||
def sysclose | ||
self.class.instance_method(:close).bind(self).call | ||
end | ||
|
||
attr_reader :close_requested | ||
attr_writer :monitor_thread | ||
end | ||
|
||
protected | ||
|
||
def monitor_rsock(threadname = 'SocketMonitorRemote') | ||
self.monitor_thread = Rex::ThreadFactory.spawn(threadname, false) do | ||
rsock.extend(MonitoredRSock) | ||
rsock.monitor_thread = self.monitor_thread = Rex::ThreadFactory.spawn(threadname, false) do | ||
loop do | ||
closed = false | ||
buf = nil | ||
closed = rsock.nil? || rsock.close_requested | ||
|
||
unless rsock | ||
wlog('monitor_rsock: the remote socket is nil, exiting loop') | ||
if closed | ||
wlog('monitor_rsock: the remote socket has been closed, exiting loop') | ||
break | ||
end | ||
|
||
buf = nil | ||
|
||
begin | ||
s = Rex::ThreadSafe.select([rsock], nil, nil, 0.2) | ||
next if s.nil? || s[0].nil? | ||
|
@@ -159,10 +179,10 @@ def monitor_rsock(threadname = 'SocketMonitorRemote') | |
# Using syswrite() breaks SSL streams. | ||
sent = write(data) | ||
|
||
# sf: Only remove the data off the queue is write was successfull. | ||
# This way we naturally perform a resend if a failure occured. | ||
# sf: Only remove the data off the queue is write was successful. | ||
# This way we naturally perform a resend if a failure occurred. | ||
# Catches an edge case with meterpreter TCP channels where remote send | ||
# failes gracefully and a resend is required. | ||
# fails gracefully and a resend is required. | ||
if sent.nil? | ||
closed = true | ||
wlog('monitor_rsock: failed writing, socket must be dead') | ||
|
@@ -182,14 +202,18 @@ def monitor_rsock(threadname = 'SocketMonitorRemote') | |
|
||
begin | ||
close_write if respond_to?('close_write') | ||
rescue IOError | ||
rescue StandardError | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgot to mention, the Meterpreter error raised when calling |
||
end | ||
|
||
break | ||
end | ||
|
||
rsock.sysclose | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now the one and only location where a monitored |
||
end | ||
end | ||
|
||
attr_accessor :monitor_thread | ||
attr_writer :lsock, :rsock | ||
end | ||
end; end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,7 +141,7 @@ def monitor_listener | |
begin | ||
cli = accept | ||
unless cli | ||
elog("The accept() returned nil in stream server listener monitor: #{fd.inspect}") | ||
elog('The accept() returned nil in stream server listener monitor') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropped the |
||
::IO.select(nil, nil, nil, 0.10) | ||
next | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only ever goes from true to false so I don't think there's a synchronization problem here but if another solution is preferred happy to go a different direction.