Make external module read loop more robust

Changes from a "hope we get at most one message at a time" model to
something beginning to resemble a state machine. Also logs error output
and fails the MSF module when the external module fails.

lib/msf/core/module/external.rb

@@ -2,6 +2,7 @@ include Msf::Auxiliary::Report
 
 module Msf::Module::External
   def wait_status(mod)
+    begin
       while mod.running
         m = mod.get_status
         if m
@@ -16,6 +17,10 @@ module Msf::Module::External
           end
         end
       end
+    rescue Exception => e #Msf::Modules::External::Bridge::Error => e
+      elog e.backtrace.join("\n")
+      fail_with Failure::UNKNOWN, e.message
+    end
   end
 
   def log_output(m)

lib/msf/core/modules/external/bridge.rb

@@ -43,12 +43,13 @@ class Msf::Modules::External::Bridge
     self.path = module_path
     self.cmd = [self.path, self.path]
     self.messages = Queue.new
+    self.buf = ''
   end
 
   protected
 
   attr_writer :path, :running
-  attr_accessor :cmd, :env, :ios, :messages
+  attr_accessor :cmd, :env, :ios, :buf, :messages, :wait_thread
 
   def describe
     resp = send_receive(Msf::Modules::External::Message.new(:describe))
@@ -64,8 +65,9 @@ class Msf::Modules::External::Bridge
   end
 
   def send(message)
-    input, output, status = ::Open3.popen3(self.env, self.cmd)
+    input, output, err, status = ::Open3.popen3(self.env, self.cmd)
-    self.ios = [input, output, status]
+    self.ios = [input, output, err]
+    self.wait_thread = status
     # We would call Rex::Threadsafe directly, but that would require rex for standalone use
     case select(nil, [input], nil, 0.1)
     when nil
@@ -91,33 +93,55 @@ class Msf::Modules::External::Bridge
   end
 
   def recv(filter_id=nil, timeout=600)
-    _, fd, _ = self.ios
+    _, out, err = self.ios
+    message = ''
 
     # Multiple messages can come over the wire all at once, and since yajl
     # doesn't play nice with windows, we have to emulate a state machine to
     # read just enough off the wire to get one request at a time. Since
     # Windows cannot do a nonblocking read on a pipe, we are forced to do a
-    # whole lot of `select` syscalls :(
+    # whole lot of `select` syscalls and keep a buffer ourselves :(
-    buf = ""
     begin
       loop do
+        # This is so we don't end up calling JSON.parse on every char and
+        # catch an exception. Windows can't do nonblock on pipes, so we
+        # still have to do the select if we are not at the end of object
+        # and don't have any buffer left
+        parts = self.buf.split '}', 2
+        if parts.length == 2 # [part, rest]
+          message << parts[0] << '}'
+          self.buf = parts[1]
+          break
+        elsif parts.length == 1 # [part]
+          if self.buf[-1] == '}'
+            message << parts[0] << '}'
+            self.buf = ''
+            break
+          else
+            message << parts[0]
+            self.buf = ''
+          end
+        end
+
         # We would call Rex::Threadsafe directly, but that would require Rex for standalone use
-        case select([fd], nil, nil, timeout)
+        res = select([out, err], nil, nil, timeout)
-        when nil
+        if res == nil
           # This is what we would have gotten without Rex and what `readpartial` can also raise
           raise EOFError.new
-        when [[fd], [], []]
+        else
-          c = fd.readpartial(1)
+          fds = res[0]
-          buf << c
+          # Preferentially drain and log stderr
-
+          if fds.include? err
-          # This is so we don't end up calling JSON.parse on every char and
+            errbuf = err.readpartial(4096)
-          # having to catch an exception. Windows can't do nonblock on pipes,
+            elog "Unexpected output running #{self.path}:\n#{errbuf}"
-          # so we still have to do the select each time.
+          end
-          break if c == '}'
+          if fds.include? out
+            self.buf << out.readpartial(4096)
+          end
         end
       end
 
-      m = Msf::Modules::External::Message.from_module(JSON.parse(buf))
+      m = Msf::Modules::External::Message.from_module(JSON.parse(message))
       if filter_id && m.id != filter_id
         # We are filtering for a response to a particular message, but we got
         # something else, store the message and try again
@@ -128,16 +152,16 @@ class Msf::Modules::External::Bridge
         m
       end
     rescue JSON::ParserError
-      # Probably an incomplete response, but no way to really tell
+      # Probably an incomplete response, but no way to really tell. Keep trying
+      # until EOF
       retry
     rescue EOFError => e
-      {}
+      nil
     end
   end
 
   def close_ios
-    input, output, status = self.ios
+    self.ios.each {|fd| fd.close rescue nil} # Yeah, yeah. I know.
-    [input, output].each {|fd| fd.close rescue nil} # Yeah, yeah. I know.
   end
 end
 

modules/exploits/linux/smtp/haraka.py

@@ -72,6 +72,7 @@ def send_mail(to, mailserver, cmd, mfrom, port):
     except smtplib.SMTPDataError as err:
       if err[0] == 450:
         module.log("Triggered bug in target server (%s)"%err[1], 'good')
+        s.close()
         return(True)
     module.log("Bug not triggered in target server", 'error')
     module.log("it may not be vulnerable or have the attachment plugin activated", 'error')