Skip to content
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

Issue #7188 resolved #8063

Closed
wants to merge 1 commit into from
Closed

Conversation

itsmeroy2012
Copy link
Contributor

Tell us what this change does. If you're fixing a bug, please mention
the github issue number.
Issue number #7188

Verification

List the steps needed to make sure this thing works

  • msfconsole

  • use exploit/multi/handler

  • set payload android/meterpreter/reverse_tcp

  • set lhost 127.0.0.1

  • set lport 4444

  • exploit

  • Verify: It will set lhost to 127.0.0.1 and will print a warning : "Please note that errors might be experienced with this choice of address for LHOST."

  • set lhost 127.0.0.54

  • set lport 4444

  • exploit

  • Verify: It will set lhost to 127.0.0.54 and will not print a warning.

@@ -82,12 +82,15 @@ def setup_handler
'MsfPayload' => self,
'MsfExploit' => assoc_exploit
})

if (ip=='127.0.0.1' || ip=='127.0.0.2' || ip=='127.0.0.3' || ip=='127.0.0.4'|| ip=='127.0.0.5'||ip=='127.0.0.6' || ip=='127.0.0.7' || ip=='127.0.0.8')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a little syntactic sugar?

localhost_ips = []

(1..8).each do |num|
  localhost_ips << "127.0.0.#{num}"
end

if localhost_ips.include?(ip)
  print_error("Please note that errors might be experienced with this choice of address for LHOST.")
end

Copy link
Contributor

@firefart firefart Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can keep it simple:

if ip =~ /^127\.0\.0\.[1-8]$/

Copy link
Member

@busterb busterb Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original suggestion from the issue was to check if this is in the loopback subnet 127.0.0.0/8, not just the first 8 addresses. The proper check function would look something like this (I'm sure the rescue could be tightened up, but I prefer test functions to really only return booleans where possible):

require 'ipaddr'

def is_loopback_addr(addr)
  begin
    a = IPAddr.new(addr.to_s)
    return true if IPAddr.new('127.0.0.1/8') === a
    return true if IPAddr.new('::1') === a
  rescue
  end
  false
end

[IPAddr.new('127.0.0.1'), '127.0.0.1', '127.255.255.255', '::1', 'dog'].each do |addr|
  puts "#{addr} is loopback: #{is_loopback_addr(addr)}"
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns the following experimental values:

127.0.0.1 is loopback: true
127.0.0.1 is loopback: true
127.255.255.255 is loopback: true
::1 is loopback: true
dog is loopback: false

@cbrnrd
Copy link
Contributor

cbrnrd commented Mar 7, 2017

Isn't this already an open PR? (#8041)

@HansHauge
Copy link

Nice catch @thecarterb

@wvu
Copy link
Contributor

wvu commented Mar 7, 2017

Yes, we're attempting to deconflict.

@busterb
Copy link
Member

busterb commented Mar 7, 2017

Close, but this isn't the right fix. This PR is also made from a master branch, which usually is easier to fix with a new PR. See https://github.com/rapid7/metasploit-framework/blob/master/CONTRIBUTING.md#code-contributions about creating topic branches for PRs.

Closing for now. See comment above as well about how to test for subnet membership for an IP address. Thanks, do try again!

@busterb busterb closed this Mar 7, 2017
@itsmeroy2012
Copy link
Contributor Author

Thanks.. I'll surely look into it...

@busterb
Copy link
Member

busterb commented Mar 7, 2017

Also, since we have an earlier PR #8041, suggest we continue discussion there. Thanks!

@itsmeroy2012
Copy link
Contributor Author

How do I issue a PR in an open discussion?

@busterb
Copy link
Member

busterb commented Mar 7, 2017

Well, I'd say make suggestions at that PR, rather than hijacking with an alternate. Let's keep the discussion in one place. You could also pull the other contributor's fork as a remote, and send him a pull request on his topic branch.

@wvu wvu mentioned this pull request Mar 16, 2017
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants