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
Add an SMB-based fetch payload for Windows #18664
Changes from all commits
a8c240f
33306fa
b590641
96316a9
c9504f9
1c36d89
b9cf7ba
3d476f4
44916e6
7e59380
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 | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -10,7 +10,11 @@ def initialize(*args) | |||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
def cleanup_handler | ||||||||||||||||||||||||||||||||||||||
cleanup_http_fetch_service(@fetch_service, @delete_resource) | ||||||||||||||||||||||||||||||||||||||
if @fetch_service | ||||||||||||||||||||||||||||||||||||||
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 change in isolation looks good to me; But this
I don't think there should be two calls to 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. Yeah the ref counting is wrong here; Verified with some extra logging to rex-core: diff --git a/lib/rex/sync/ref.rb b/lib/rex/sync/ref.rb
index 50b0146..9087747 100644
--- a/lib/rex/sync/ref.rb
+++ b/lib/rex/sync/ref.rb
@@ -45,6 +45,8 @@ module Ref
@_references = 1
@_references_mutex = Mutex.new
+ $stderr.puts "[ref-count] calling refinit: #{caller}, total references: current #{@_references}"
+
self
end
@@ -54,6 +56,7 @@ module Ref
def ref
@_references_mutex.synchronize {
@_references += 1
+ $stderr.puts "[ref-count] creating ref: #{caller}, total references: current #{@_references}"
}
self
@@ -65,11 +68,15 @@ module Ref
#
def deref
@_references_mutex.synchronize {
- if ((@_references -= 1) == 0)
+ @_references -= 1
+ $stderr.puts "[ref-count] creating deref: #{caller}, total references: #{@_references}"
+ if (@_references == 0)
+ $stderr.puts "[ref-count] calling cleanup"
cleanup
true
else
+ $stderr.puts "[ref-count] no cleanup required"
false
end
} We end up with a refcount of 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. Yeah that looks like an issue, but I copied it over from @bwatters-r7's original implementation here. I tested it again by removing everything after the 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. I always thought this was wonky; I never understood why we needed to 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. @bwatters-r7 are we alright to keep the proposed changes or are you still looking into why it needed to be done this way? 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. I started looking at the old commits and was trying to remember what tests I ran that made me think this was an issue. It is on my list for today to see if I can figure it out. 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. I found the conversation where I think I was working on this. I was unable to recreate the issue when I removed
That said, I also can not longer use the |
||||||||||||||||||||||||||||||||||||||
cleanup_http_fetch_service(@fetch_service, @delete_resource) | ||||||||||||||||||||||||||||||||||||||
@fetch_service = nil | ||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
super | ||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
@@ -20,4 +24,3 @@ def setup_handler | |||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,102 @@ | ||
module Msf::Payload::Adapter::Fetch::Server::HTTP | ||
include Msf::Payload::Adapter::Fetch::Server::Https | ||
|
||
# This mixin supports only HTTP fetch handlers but still imports the HTTPS mixin. | ||
# We just remove the HTTPS Options so the user does not see them. | ||
# | ||
# This mixin supports only HTTP fetch handlers. | ||
|
||
def initialize(*args) | ||
super | ||
deregister_options('FETCH_SSL', | ||
'FETCH_CHECK_CERT', | ||
'FetchSSLCert', | ||
'FetchSSLCompression', | ||
'FetchSSLCipher', | ||
'FetchSSLCipher', | ||
'FetchSSLVersion' | ||
register_advanced_options( | ||
[ | ||
Msf::OptString.new('FetchHttpServerName', [true, 'Fetch HTTP server name', 'Apache']) | ||
] | ||
) | ||
end | ||
|
||
def fetch_protocol | ||
'HTTP' | ||
end | ||
|
||
def srvname | ||
datastore['FetchHttpServerName'] | ||
end | ||
|
||
def add_resource(fetch_service, uri, srvexe) | ||
vprint_status("Adding resource #{uri}") | ||
if fetch_service.resources.include?(uri) | ||
# When we clean up, we need to leave resources alone, because we never added one. | ||
@delete_resource = false | ||
fail_with(Msf::Exploit::Failure::BadConfig, "Resource collision detected. Set FETCH_URIPATH to a different value to continue.") | ||
end | ||
fetch_service.add_resource(uri, | ||
'Proc' => proc do |cli, req| | ||
on_request_uri(cli, req, srvexe) | ||
end, | ||
'VirtualDirectory' => true) | ||
rescue ::Exception => e | ||
# When we clean up, we need to leave resources alone, because we never added one. | ||
@delete_resource = false | ||
fail_with(Msf::Exploit::Failure::Unknown, "Failed to add resource\n#{e}") | ||
end | ||
|
||
def cleanup_http_fetch_service(fetch_service, delete_resource) | ||
escaped_srvuri = ('/' + srvuri).gsub('//', '/') | ||
if fetch_service.resources.include?(escaped_srvuri) && delete_resource | ||
fetch_service.remove_resource(escaped_srvuri) | ||
end | ||
fetch_service.deref | ||
end | ||
|
||
def start_http_fetch_handler(srvname, srvexe, ssl=false, ssl_cert=nil, ssl_compression=nil, ssl_cipher=nil, ssl_version=nil) | ||
# this looks a bit funny because I converted it to use an instance variable so that if we crash in the | ||
# middle and don't return a value, we still have the right fetch_service to clean up. | ||
escaped_srvuri = ('/' + srvuri).gsub('//', '/') | ||
fetch_service = start_http_server(ssl, ssl_cert, ssl_compression, ssl_cipher, ssl_version) | ||
if fetch_service.nil? | ||
cleanup_handler | ||
fail_with(Msf::Exploit::Failure::BadConfig, "Fetch handler failed to start on #{fetch_bindnetloc}") | ||
end | ||
vprint_status("#{fetch_protocol} server started") | ||
fetch_service.server_name = srvname | ||
add_resource(fetch_service, escaped_srvuri, srvexe) | ||
fetch_service | ||
end | ||
|
||
def on_request_uri(cli, request, srvexe) | ||
client = cli.peerhost | ||
vprint_status("Client #{client} requested #{request.uri}") | ||
if (user_agent = request.headers['User-Agent']) | ||
client += " (#{user_agent})" | ||
end | ||
vprint_status("Sending payload to #{client}") | ||
cli.send_response(payload_response(srvexe)) | ||
end | ||
|
||
def payload_response(srvexe) | ||
res = Rex::Proto::Http::Response.new(200, 'OK', Rex::Proto::Http::DefaultProtocol) | ||
res['Content-Type'] = 'text/html' | ||
res.body = srvexe.to_s.unpack('C*').pack('C*') | ||
res | ||
end | ||
|
||
def start_http_server(ssl=false, ssl_cert=nil, ssl_compression=nil, ssl_cipher=nil, ssl_version=nil) | ||
begin | ||
fetch_service = Rex::ServiceManager.start( | ||
Rex::Proto::Http::Server, | ||
fetch_bindport, fetch_bindhost, ssl, | ||
{ | ||
'Msf' => framework, | ||
'MsfExploit' => self | ||
}, | ||
_determine_server_comm(fetch_bindhost), | ||
ssl_cert, | ||
ssl_compression, | ||
ssl_cipher, | ||
ssl_version | ||
) | ||
rescue Exception => e | ||
cleanup_handler | ||
fail_with(Msf::Exploit::Failure::BadConfig, "Fetch handler failed to start on #{fetch_bindnetloc}\n#{e}") | ||
end | ||
vprint_status("Fetch handler listening on #{fetch_bindnetloc}") | ||
fetch_service | ||
end | ||
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.
Not a blocker; This UX is a bit weird for me having to specify FETCH_SRVHOST instead of the normal LHOST convention 😄
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 makes sense to me?
If there is an
LHOST
value (reverse port-enabled payloads),FETCH_SRVHOST
is optional, and if it is blank we use theLHOST
value.If there is no
LHOST
value, we require theFETCH_SRVHOST
value.There are a lot of situations where we would like the
LHOST
andFETCH_SRVHOST
to be different, though.