rapid7 / ruby_smb

A native Ruby implementation of the SMB Protocol Family
Other
80 stars 80 forks source link

Close the opened directory after listing it's contents #257

Closed dwelch-r7 closed 7 months ago

dwelch-r7 commented 8 months ago

Noticed an issue when working on https://github.com/rapid7/metasploit-framework/pull/18539 where when listing the contents of a directory we don't close the handle to it resulting in weirdness on windows like not being able to rename any files located in that directory

To replicate add:

require 'pry-byebug'; binding.pry
puts 'done'

to the end of examples/list_directory to pause execution of the script so we don't disconnect run this command bundle exec ruby list_directory.rb <ip> <user> <pass> <share> example: bundle exec ruby list_directory.rb 172.16.158.159 vagrant vagrant foo

Then try to rename a file in the share you're listing and you'll get an error like this:

image

Check out this PR and run through the same steps and you should no longer hit the issue

adfoster-r7 commented 8 months ago

Does this mean there's potentially other resources being opened that aren't closed correctly? 🤔

dwelch-r7 commented 8 months ago

Does this mean there's potentially other resources being opened that aren't closed correctly? 🤔

Potentially sure, but none that I've ran into so far but I only interacted with it via the smb session types which is only uses a subset of ruby smbs features

adfoster-r7 commented 8 months ago

This seems legit to me; Will await an eyeball from @cdelafuente-r7 / @zeroSteiner incase there's a cleaner close API available here that should be used

cdelafuente-r7 commented 7 months ago

It looks good to me.

A another way would be to refactor this and move everything related to directory to a separate RubySMB::SMB2::Directory class or under the existing RubySMB::SMB2::File class. It looks like open_directory is not consistent with the other open_file and open_pipe methods. This one returns a response and the two others return a proper object (RubySMB::SMB2::File and RubySMB::SMB2::Pipe). The list method should also not be part of the RubySMB::SMB2::Tree class, IMHO.

That said, it is out-of-scope and this proposed fix is good to me.