Skip to content
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

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

Merged

Conversation

dwelch-r7
Copy link
Contributor

Noticed an issue when working on rapid7/metasploit-framework#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
Copy link
Contributor

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

@dwelch-r7
Copy link
Contributor Author

dwelch-r7 commented Dec 4, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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.

@adfoster-r7 adfoster-r7 merged commit f1cd79a into rapid7:master Dec 11, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants