-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
Replace IO read on binary files with File binread #16325
Conversation
d14316a
to
c7da195
Compare
# bug fix for: data = ::IO.read(datastore['PEXEC']) | ||
# the above does not return the entire contents |
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.
👀 👀 👀
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.
lol. perhaps we should add a rubocop rule for this?
Looks like the issue exists with
As well as calls to |
Not 100% sure this fixes all the instances of this problem. Looking at https://github.com/rapid7/metasploit-framework/search?q=%22IO.read%22, I see a couple of cases where we have library functions that take in a parameter named Examples can be seen in lib/msf/core/exploit/remote/java/rmi/util.rb particularly at metasploit-framework/lib/msf/core/exploit/remote/java/rmi/util.rb Lines 49 to 143 in 04e8752
IO object and all of which call the .read() method on that object later on in the code.
To truely fix all instances of this issue I believe we may need to do another run through the code and take this into consideration. There also appears to be some cases where |
After some digging through the code, it turns out that these calls are using metasploit-framework/lib/msf/core/exploit/remote/java/rmi/client/jmx/server/parser.rb Lines 25 to 27 in df81a48
metasploit-framework/lib/msf/core/exploit/remote/java/rmi/client/registry/parser.rb Lines 23 to 25 in df81a48
|
It appears I haven't tried hard enough to break assumptions 😈 Lets see if I kind find any other potential edge cases, but nice job spotting that isn't an affected change 👍 |
Potentially missed files: metasploit-framework/modules/auxiliary/gather/flash_rosetta_jsonp_url_disclosure.rb Lines 64 to 72 in 585b470
metasploit-framework/docs/build.rb Lines 1097 to 1119 in 953a4cb
|
I think for the suggested files above, they don't need to be changed as they are either not interacting with the filesystem or reading from a process, therefore they aren't impacted. Correct me if I'm wrong. 💯 |
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.
Proposed changes look good but will await updates to see if we need to add additional files.
ebf7dc1
to
13c7989
Compare
Syncing up with @sjanusz-r7 - we're keeping the scope of this PR to fixing the uses of IO.read that are obviously reading binary files |
Before the change on a Windows host (Linux hosts work fine):
|
@@ -117,7 +117,7 @@ def ef_payload(pdf_name,payload_exe,obj_num) | |||
print_status("Using '#{datastore['EXENAME']}' as payload...") | |||
|
|||
file_size = File.size(payload_exe) | |||
stream = Rex::Text.zlib_deflate(IO.read(payload_exe)) | |||
stream = Rex::Text.zlib_deflate(File.binread(payload_exe)) | |||
md5 = Rex::Text.md5(File.read(payload_exe)) |
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 seems to be failing for me which leads me to ask, should this not also be File.binread
in the call to Rex::Text.md5
?
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.
Good God, are they putting encoding on #read
? i'm guessing there's a fair amount of that going on 😕
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.
Probably, I wouldn't be surprised if similar typos were made just cause people didn't know binread
existed.
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.
Git blame will not be kind to me on this one. Since we embed binary encoding enforcement in the Ruby file level, i thought this was already addressed. Should we change all occurrences then? The encoding nightmares of 1.9 days are an old ghost - hoping there's a consistent way to do this all around (and should we update Rex underpinnings?).
This is still failing for me:
I think this may be related to comment above about using Edit: Its still failing for me, not sure what is going on here... |
06f3937
to
96aff11
Compare
96aff11
to
bbf9e31
Compare
@gwillcox-r7 I believe that issue should be resolved now 🤞 For visibility; We expanded on the scope of this PR a bit from just updating the IO.read calls. It now focuse on fixing the most obvious scenarios were uses of File.read and IO.read should have been using File.binread or specifying mode We've made the world a better place, but there's more than likely still gremlins, but it should be shippable in its current state. A potential gremlin is the scenario of uploading user specified files to a remote target, whether line endings should be normalised or not, and the impact that has on uploading binary files. Other tools solve this by letting the user specify ascii/binary modes, i.e. in the case of bsd's ftp client, or just always uploading in binary mode, i.e. samba's smbclient. |
Looks like I made a mistake and the module in question needed the changes made to the files
Will do one last inspection over code and then get this landed, thanks for making these changes! |
This should be good to land, unfortunately GitHub had some hicups earlier with their API and similar so I wasn't sure if I should land or not, but this should be good to go. |
Release NotesThis PR replaces |
This PR replaces
IO.read
withFile.binread
, in scenarios where it's obvious that we're reading from binaries, to prevent an issue where not all of the file has been read correctly due to an additional EOL<->CRLF conversion that happens on Windows. Specifying the mode onFile.read
orIO.read
to includeb
would also fix the issue.Example:
Relevant issue: #16285
Cross-referencing a PR that also takes care of file reads: #16174
Bug - Not reading full file contents
When trying to read a file without specifying the mode with
File
orIO
:Verification
msfconsole
use exploit/windows/fileformat/adobe_pdf_embedded_exe
set exename {...}
run
run
command fails without these changes.Before (using
exploit/windows/fileformat/adobe_pdf_embedded_exe
)After (using
exploit/windows/fileformat/adobe_pdf_embedded_exe
)