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

Fix FileDropper to properly clone string variables before storing them #18403

Merged

Conversation

cdelafuente-r7
Copy link
Contributor

This fix an issue I spotted while testing this PR.

The way register_files_for_cleanup and register_dirs_for_cleanup store the strings in @dropped_files and @dropped_dirs can cause issues when the array passed as argument contains references to a string variable.

Please see this comment for a detailed example of what could happen.

Verification

A basic example would be:

dir = 'C:\\one'
mkdir(dir)
dir << '\\two'
mkdir(dir)
dir << '\\three'
mkdir(dir)

mkdir takes care of calling register_dirs_for_cleanup and will end up having @dropped_dirs with three identical entries:

[1] pry(main)> @dropped_dirs
=> ["C:\\one\\two\\three",
 "C:\\one\\two\\three",
 "C:\\one\\two\\three"]

After this fix, @dropped_dirs should contains the expected entries:

[1] pry(main)> @dropped_dirs
=> ["C:\\one",
 "C:\\one\\two",
 "C:\\one\\two\\three"]

Maybe the easiest way to test this is to follow the steps in the PR I mentioned earlier and make sure the expected directories are correctly cleaned up.

@cdelafuente-r7 cdelafuente-r7 changed the title Properly clone the path strings before storing them Fix FileDropper to properly clone string variables before storing them Sep 26, 2023
@adfoster-r7
Copy link
Contributor

I worry that cloning all strings here by default will be unnecessary overhead that isn't needed for most modules. I wonder if we froze the strings instead, that might be cleaner / more efficient long-term - as it would be up to the caller to not mutate the strings out of band. I just don't have a gut feel for how many modules would be doing something wrong here to explore that path, but I imagine it wouldn't be too many modules? 🤔

@bwatters-r7
Copy link
Contributor

I agree it is unnecessary for most modules, but I think that it is the best way to have this feature act as expected. Delayed action on a variable passed by reference (especially a delayed rm action) is a recipe for disaster.
Expecting every module author to know that after they create a directory they can never change the string they used to create the directory is probably not a realistic thing.

@adfoster-r7 adfoster-r7 merged commit 93645c2 into rapid7:master Oct 25, 2023
55 checks passed
@adfoster-r7 adfoster-r7 added the rn-fix release notes fix label Oct 25, 2023
@adfoster-r7
Copy link
Contributor

Release Notes

Fixes a potential bug with modules that register files to cleanup after a session opens. Previously modules could accidentally mutate registered file names to delete, causing the intended files to be left on the remote system still.

@adfoster-r7
Copy link
Contributor

Thanks for the PR, it's a shame there's so much unintended mutation in some of our modules - but I guess this is a nicer implementation detail to guard against potential issues - even at the expense of memory costs. Hopefully this doesn't break modules that relied on the old behavior 😄

Feel free to reword the release notes if they're not quite cromulent 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug library rn-fix release notes fix
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants