Land rapid7#4858, RPC client true/truthy fix

* Misc ruby cleanup and fixing the issue that caused MSP-12235, rolling back the
full rollback of PR 4823

Author: shuckins-r7

lib/msf/core/rpc/v10/client.rb

@@ -31,7 +31,7 @@ def initialize(info={})
 
   def login(user,pass)
     res = self.call("auth.login", user, pass)
-    if(not (res and res['result'] == "success"))
+    unless (res && res['result'] == "success")
       raise RuntimeError, "authentication failed"
     end
     self.token = res['token']
@@ -41,16 +41,16 @@ def login(user,pass)
   # Prepend the authentication token as the first parameter
   # of every call except auth.login. Requires the
   def call(meth, *args)
-    if(meth != "auth.login")
-      if(not self.token)
+    unless meth == "auth.login"
+      unless self.token
         raise RuntimeError, "client not authenticated"
       end
       args.unshift(self.token)
     end
 
     args.unshift(meth)
 
-    if not @cli
+    unless @cli
       @cli = Rex::Proto::Http::Client.new(info[:host], info[:port], info[:context], info[:ssl], info[:ssl_version])
       @cli.set_config(
         :vhost => info[:host],
@@ -69,10 +69,12 @@ def call(meth, *args)
     res = @cli.send_recv(req)
     @cli.close
 
-    if res and [200, 401, 403, 500].include?(res.code)
+    if res && [200, 401, 403, 500].include?(res.code)
       resp = MessagePack.unpack(res.body)
 
-      if resp and resp.kind_of?(::Hash) and resp['error'] == true
+      # Boolean true versus truthy check required here;
+      # RPC responses such as { "error" => "Here I am" } and { "error" => "" } must be accommodated.
+      if resp && resp.kind_of?(::Hash) && resp['error'] == true
         raise Msf::RPC::ServerException.new(resp['error_code'] || res.code, resp['error_message'] || resp['error_string'], resp['error_class'], resp['error_backtrace'])
       end
 
@@ -83,7 +85,7 @@ def call(meth, *args)
   end
 
   def close
-    if @cli and @cli.conn?
+    if @cli && @cli.conn?
       @cli.close
     end
     @cli = nil

lib/msf/core/rpc/v10/service.rb

@@ -112,21 +112,21 @@ def process(req)
         end
       end
 
-      if not (req.headers["Content-Type"] and req.headers["Content-Type"] == "binary/message-pack")
+      unless (req.headers["Content-Type"] && req.headers["Content-Type"] == "binary/message-pack")
         raise ArgumentError, "Invalid Content Type"
       end
 
       msg = MessagePack.unpack(req.body)
 
-      if not (msg and msg.kind_of?(::Array) and msg.length > 0)
+      unless (msg && msg.kind_of?(::Array) && msg.length > 0)
         raise ArgumentError, "Invalid Message Format"
       end
 
       msg.map { |a| a.respond_to?(:force_encoding) ? a.force_encoding(self.str_encoding) : a }
 
       group, funct = msg.shift.split(".", 2)
 
-      if not self.handlers[group]
+      unless self.handlers[group]
         raise ArgumentError, "Unknown API Group: '#{group.inspect}'"
       end
 
@@ -138,13 +138,13 @@ def process(req)
         mname << '_noauth'
       end
 
-      if not self.handlers[group].respond_to?(mname)
+      unless self.handlers[group].respond_to?(mname)
         raise ArgumentError, "Unknown API Call: '#{mname.inspect}'"
       end
 
       if doauth
         token = msg.shift
-        if not authenticate(token)
+        unless authenticate(token)
           raise ::Msf::RPC::Exception.new(401, "Invalid Authentication Token")
         end
       end
@@ -203,7 +203,7 @@ def authenticate(token)
     stale = []
 
 
-    if not (token and token.kind_of?(::String))
+    unless (token && token.kind_of?(::String))
       return false
     end
 
@@ -212,17 +212,17 @@ def authenticate(token)
 
     self.tokens.each_key do |t|
       user,ctime,mtime,perm = self.tokens[t]
-      if ! perm and mtime + self.token_timeout < Time.now.to_i
+      if !perm && mtime + self.token_timeout < Time.now.to_i
         stale << t
       end
     end
 
     stale.each { |t| self.tokens.delete(t) }
 
-    if not self.tokens[token]
+    unless self.tokens[token]
 
       begin
-        if framework.db.active and ::Mdm::ApiKey.find_by_token(token)
+        if framework.db.active && ::Mdm::ApiKey.find_by_token(token)
           return true
         end
       rescue ::Exception => e

msfrpc

@@ -41,7 +41,7 @@ opts = {
 }
 
 # Parse command line arguments.
-arguments.parse(ARGV) { |opt, idx, val|
+arguments.parse(ARGV) do |opt, idx, val|
   case opt
     when "-a"
       opts['ServerHost'] = val
@@ -57,16 +57,16 @@ arguments.parse(ARGV) { |opt, idx, val|
       print("\nUsage: #{File.basename(__FILE__)} <options>\n" +	arguments.usage)
       exit
   end
-}
+end
 
 
-if(not opts['ServerHost'])
+unless opts['ServerHost']
   $stderr.puts "[-] Error: a server IP must be specified (-a)"
   $stderr.puts arguments.usage
   exit(0)
 end
 
-if(not opts['Pass'])
+unless opts['Pass']
   $stderr.puts "[-] Error: a password must be specified (-P)"
   $stderr.puts arguments.usage
   exit(0)
@@ -83,10 +83,11 @@ rpc = Msf::RPC::Client.new(
   :ssl  => opts['SSL']
 )
 
-res = rpc.login(opts['User'], opts['Pass'])
+rpc.login(opts['User'], opts['Pass'])
 
 puts "[*] The 'rpc' object holds the RPC client interface"
-puts ""
+puts "[*] Use rpc.call('group.command') to make RPC calls"
+puts ''
 
 while(ARGV.shift)
 end

msfrpcd

@@ -72,7 +72,7 @@ arguments.parse(ARGV) { |opt, idx, val|
   end
 }
 
-if(not opts['Pass'])
+unless opts['Pass']
   $stderr.puts "[-] Error: a password must be specified (-P)"
   exit(0)
 end
@@ -83,7 +83,7 @@ rpctype = 'MSG'
 
 $stderr.puts "[*] #{rpctype}RPC starting on #{opts['ServerHost']}:#{opts['ServerPort']} (#{opts['SSL'] ? "SSL" : "NO SSL"}):#{opts['ServerType']}..."
 
-$stderr.puts "[*] URI: #{opts['URI']}" if(opts['URI'])
+$stderr.puts "[*] URI: #{opts['URI']}" if opts['URI']
 
 require 'msf/base'
 require 'msf/ui'

plugins/msgrpc.rb

@@ -1,15 +1,11 @@
 #!/usr/bin/env ruby
 #
-# $Id$
-#
 # This plugin provides an msf daemon interface that spawns a listener on a
 # defined port (default 55552) and gives each connecting client its own
 # console interface.  These consoles all share the same framework instance.
 # Be aware that the console instance that spawns on the port is entirely
 # unauthenticated, so realize that you have been warned.
 #
-# $Revision$
-#
 
 require "msf/core/rpc/v10/service"
 require "fileutils"
@@ -43,7 +39,7 @@ def initialize(framework, opts)
 
     host = opts['ServerHost'] || DefaultHost
     port = opts['ServerPort'] || DefaultPort
-    ssl  = (opts['SSL'] and opts['SSL'].to_s =~ /^[ty]/i) ? true : false
+    ssl  = (opts['SSL'] && opts['SSL'].to_s =~ /^[ty]/i) ? true : false
     cert = opts['SSLCert']
 
     user = opts['User'] || "msf"
@@ -67,7 +63,7 @@ def initialize(framework, opts)
 
     # If the run in foreground flag is not specified, then go ahead and fire
     # it off in a worker thread.
-    if (opts['RunInForeground'] != true)
+    unless opts['RunInForeground'] == true
       # Store a handle to the thread so we can kill it during
       # cleanup when we get unloaded.
       self.thread = Thread.new { run }