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 URL-encoded filesystem entries in the Velociraptor loader #700

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Zawadidone
Copy link
Contributor

@Zawadidone Zawadidone commented May 5, 2024

Closes #707 and #708

This occurs with some Velociraptor collections.
It is currently unknown why this issue occurs.
The names of the files and directories collected by Velociraptor are
URL-encoded, which are decoded by the ZIP loader in order to
prevent errors when executing plugins.
This mainly impacts Unix targets because files like `%2Ebash_history`
-> `.bash_history` are used by the plugins.
Loading a directory is removed, because using the ZIP as a
target is the preferred and enforced method.
@Schamper
Copy link
Member

Schamper commented May 6, 2024

Wouldn't this also be an issue on non-zip (directory) Velociraptor targets?

@Zawadidone
Copy link
Contributor Author

@Schamper yes it will, I would like to enforce the usage of a ZIP file as target, because extracting a ZIP file causes all kinds of errors. For example, the only option to extract a ZIP file in an automated manner is with unzip -o [...], which enables overwriting existing files without prompting, which possible removes valuable traces.

Should this explicitly be documented in the comments of the loader or should the loader also support a non-zip directory Velociraptor target?

@Schamper
Copy link
Member

Schamper commented May 7, 2024

We don't use Velociraptor so I can't really speak for it, but I do think I've heard people say they sometimes work with unzipped collections. If there are no other reasons to drop support for it, I'd suggest to let it in.

I'm not a huge fan of the explicit URL decoding under a generic decode_name flag, but I don't immediately have a better suggestion. I was thinking maybe to pass a callback function, but I'm not sure I like that either 😄. I'll also throw it up for discussion in the team to see if anyone has a nice idea.

@Schamper
Copy link
Member

After a short discussion we decided it may be best to tweak the ZipFilesystem and DirectoryFilesystem implementations slightly to facilitate customization by subclasses a little better. You can then subclass these filesystems within your loader and provide the customized behaviour. This subclass can live within the same file as the loader as far as I'm concerned (we also do this in other cases where we e.g. subclass the registry plugin to provide a custom implementation).

I've created #707 and #708 to implement this in the respective filesystems. Would you be up for making these changes?

@Zawadidone
Copy link
Contributor Author

Yes but I don't understand how to fix the issues. I will add comments to the issues.

@OlafHaalstra
Copy link
Contributor

Would be nice to keep the support for a non-zipped folder since we noticed a huge hit on performance when working with zip archives rather than unzipped directories.

@Zawadidone wondering if you do not have the same problem with zipped vs unzipped

@OlafHaalstra
Copy link
Contributor

To add to the need for this.

Example, the following path is not picked up by target-*:

uploads/auto/C%3A/Users/Test/%2Essh/known_hosts 

This totally makes sense since the openssh module is looking for the .ssh folder:

Can we include the fix not only for the zip loader, but also for the folder loader?

@OlafHaalstra
Copy link
Contributor

@Schamper yes it will, I would like to enforce the usage of a ZIP file as target, because extracting a ZIP file causes all kinds of errors. For example, the only option to extract a ZIP file in an automated manner is with unzip -o [...], which enables overwriting existing files without prompting, which possible removes valuable traces.

Should this explicitly be documented in the comments of the loader or should the loader also support a non-zip directory Velociraptor target?

@Zawadidone Under which circumstances do you run into files that are overwritten?

@Zawadidone
Copy link
Contributor Author

Would be nice to keep the support for a non-zipped folder since we noticed a huge hit on performance when working with zip archives rather than unzipped directories.

@Zawadidone wondering if you do not have the same problem with zipped vs unzipped

Yes we have the same performance hit when working with a collection stored in a ZIP file, but only when executing the MFT plugin. The other plugins are 'fast' but that's only because they return less records compared to the MFT plugin.

I haven't had the time for it, but I want to look into solving the following issues:

@Zawadidone
Copy link
Contributor Author

To add to the need for this.

Example, the following path is not picked up by target-*:

uploads/auto/C%3A/Users/Test/%2Essh/known_hosts 

This totally makes sense since the openssh module is looking for the .ssh folder:

Can we include the fix not only for the zip loader, but also for the folder loader?

Yes this PR will include a fix for both the ZIP as well as the DIR loader.

@Zawadidone
Copy link
Contributor Author

@Schamper yes it will, I would like to enforce the usage of a ZIP file as target, because extracting a ZIP file causes all kinds of errors. For example, the only option to extract a ZIP file in an automated manner is with unzip -o [...], which enables overwriting existing files without prompting, which possible removes valuable traces.
Should this explicitly be documented in the comments of the loader or should the loader also support a non-zip directory Velociraptor target?

@Zawadidone Under which circumstances do you run into files that are overwritten?

On Linux systems that use symlinks see #699, but this could occur on all kind of systems that use symlinks.

@Zawadidone Zawadidone marked this pull request as draft July 8, 2024 21:57
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.

Separate mapping of filesystem entries in ZipFilesystem into another method
3 participants