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

Modify Post read_file to check if file is readable #11342

Merged
merged 1 commit into from Feb 7, 2019

Conversation

Projects
None yet
2 participants
@bcoles
Copy link
Contributor

bcoles commented Feb 1, 2019

This changes the behavior for Post read_file on command shell sessions to first check if a file is readable before attempting read.

This fixes #9854 by ensuring that nil is returned in the event a file is not readable.

Prior to this PR, invoking read_file on a command shell session, for a file which was not readable (didn't exist or insufficient permissions) would sometimes return an error message from the system (which could vary in value based on system locale) depending on whether stderr was routed over the socket.

Advantages

It ensures consistency. read_file on command shell sessions will now always return nil if the file cannot be read, regardless of session type or remote system environment.

It should be stealthier in logs, in that it won't trigger permission denied errors.

Disadvantages

It adds an extra cmd_exec call for every file read, and requires test to be in $PATH.

TOCTOU. If this is a concern, instead of executing the test in a separate cmd_exec call, test -r /path/to/file && could be prepended in a single cmd_exec call.

Command Shell Session

msf5 exploit(linux/local/test) > check 

[!] SESSION may not be compatible with this module.
cat: /etc/shadow: Permission denied
cat: /tmp/doesnotexist: No such file or directory
[-] Check failed: The state could not be determined.
msf5 exploit(linux/local/test) > edit lib/msf/core/post/file.rb
[*] Reloading /pentest/exploit/metasploit-framework/lib/msf/core/post/file.rb
msf5 exploit(linux/local/test) > git diff lib/msf/core/post/file.rb
[*] exec: git diff lib/msf/core/post/file.rb

diff --git a/lib/msf/core/post/file.rb b/lib/msf/core/post/file.rb
index e256169..06a1452 100644
--- a/lib/msf/core/post/file.rb
+++ b/lib/msf/core/post/file.rb
@@ -167,6 +167,19 @@ module Msf::Post::File
   end
 
   #
+  # See if +path+ on the remote system exists and is readable
+  #
+  # @param path [String] Remote path to check
+  #
+  # @return [Boolean] true if +path+ exists and is readable
+  #
+  def readable?(path)
+    raise "`readable?' method does not support Windows systems" if session.platform == 'windows'
+
+    cmd_exec("test -r '#{path}' && echo true").to_s.include? 'true'
+  end
+
+  #
   # Check for existence of +path+ on the remote file system
   #
   # @param path [String] Remote filename to check
@@ -318,6 +331,8 @@ module Msf::Post::File
       return session.shell_command_token("type \"#{file_name}\"")
     end
 
+    return nil unless readable?(file_name)
+
     if command_exists?('cat')
       return session.shell_command_token("cat \"#{file_name}\"")
     end
msf5 exploit(linux/local/test) > check 

[!] SESSION may not be compatible with this module.


[-] Check failed: The state could not be determined.
msf5 exploit(linux/local/test) > 

Test Code

  def check
    puts read_file '/tmp/doesnotexist'
    return
  end

@busterb busterb self-assigned this Feb 7, 2019

@busterb

This comment has been minimized.

Copy link
Contributor

busterb commented Feb 7, 2019

This does improve things, even if there's a window for a file to be deleted. Seems to be about the best you can do on a shell session.

@busterb busterb merged commit 47a13ea into rapid7:master Feb 7, 2019

3 checks passed

Metasploit Automation - Sanity Test Execution Successfully completed all tests.
Details
Metasploit Automation - Test Execution Successfully completed all tests.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

busterb added a commit that referenced this pull request Feb 7, 2019

@busterb

This comment has been minimized.

Copy link
Contributor

busterb commented Feb 7, 2019

Release Notes

This adds a run-time check if a file exists in a shell session before trying to read that file, eliminating some types of errors.

@bcoles bcoles deleted the bcoles:lib-post-file-readable branch Feb 7, 2019

jmartin-r7 added a commit that referenced this pull request Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment