-
Notifications
You must be signed in to change notification settings - Fork 44
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
Improve shellbags plugin #470
Open
JSCU-CNI
wants to merge
11
commits into
fox-it:main
Choose a base branch
from
JSCU-CNI:fix/windows-shellbags-volume-name-decode
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
f039266
improve shellbags volume_name decode
JSCU-CNI fba32e3
Merge branch 'fox-it:main' into fix/windows-shellbags-volume-name-decode
JSCU-CNI 291ec1a
Merge branch 'fox-it:main' into fix/windows-shellbags-volume-name-decode
JSCU-CNI b7abda4
Merge branch 'main' into fix/windows-shellbags-volume-name-decode
JSCU-CNI a0379cd
Merge branch 'main' into fix/windows-shellbags-volume-name-decode
JSCU-CNI e7a059a
Merge branch 'main' into fix/windows-shellbags-volume-name-decode
JSCU-CNI 4a3fb19
Merge branch 'main' into fix/windows-shellbags-volume-name-decode
JSCU-CNI 3e9c219
Merge branch 'main' into fix/windows-shellbags-volume-name-decode
JSCU-CNI 8fd0dd1
Merge branch 'main' into fix/windows-shellbags-volume-name-decode
JSCU-CNI c8eca4c
Merge branch 'main' into fix/windows-shellbags-volume-name-decode
JSCU-CNI 059a2fe
Merge branch 'main' into fix/windows-shellbags-volume-name-decode
JSCU-CNI File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Would a
surrogateescape
not be preferred?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.
If I remember correctly
surrogateescape
does not work with ASCII incompatible encodings. Why would you prefer surrogates in this instance?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.
We use
surrogateescape
everywhere else and it's also the default error mode for filesystems. I'm not aware of any limitations withsurrogateescape
, do you have an example of that?I'm definitely not an expert on decoding error modes, so I'm mostly going by with what I know works and what we use elsewhere.
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.
Using
surrogateescape
here would not repair the shellbags plugin. You would still get the following errorUnicodeDecodeError: 'utf-8' codec can't encode characters in position x-y: surrogates not allowed
when encoding the record for the record stream. (https://stackoverflow.com/a/31898719)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.
TIL, thanks for the explanation.
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.
What is the status of this review? Do you propose a fix in
flow.record
?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.
Yes, this should be fixed in flow.record. Unfortunately, there is no easy single spot there to fix it.
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.
Could this be merged in the meantime?
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.
I'd rather not, as it would deviated from all other places that use
errors="surrogateescape"
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.
When you compare the two in the
dissect.target
codebase I think it would be incorrect to state thatsurrogateescape
is the de facto 'default'.https://github.com/search?q=repo%3Afox-it%2Fdissect.target+surrogateescape&type=code
https://github.com/search?q=repo%3Afox-it%2Fdissect.target%20%22backslashreplace%22&type=code
I agree it would be nice if this gets a proper fix in
flow.record
eventually. Our current situation is that theshellbags
plugin is broken as no encoding error handler is used when dealing with foreign volume names.Using
surrogateescape
would still break theshellbags
plugin as it is currently incompatible withflow.record
. Not using any error handling keeps this plugin broken for us.In my opinion using
errors=backslashreplace
is better than using no error handler at all. Please consider merging this as a temporary solution while we wait for a better fix inflow.record
.