Improve shellbags plugin #470
Conversation
A target system can contain non utf-8 characters in the shellbag volume_name. This would break the shellbags plugin and is now fixed.
| @@ -617,7 +617,7 @@ def __init__(self, buf): | |||
| if self.type == 0x2E: | |||
| self.identifier = uuid.UUID(bytes_le=buf[4:20].tobytes()) | |||
| else: | |||
| self.volume_name = self.fh.read(20).rstrip(b"\x00").decode() | |||
| self.volume_name = self.fh.read(20).rstrip(b"\x00").decode(errors="backslashreplace") | |||
There was a problem hiding this comment.
Would a surrogateescape not be preferred?
There was a problem hiding this comment.
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.
We use surrogateescape everywhere else and it's also the default error mode for filesystems. I'm not aware of any limitations with surrogateescape, 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.
Using surrogateescape here would not repair the shellbags plugin. You would still get the following error UnicodeDecodeError: '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.
TIL, thanks for the explanation.
There was a problem hiding this comment.
What is the status of this review? Do you propose a fix in flow.record?
There was a problem hiding this comment.
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.
Could this be merged in the meantime?
There was a problem hiding this comment.
I'd rather not, as it would deviated from all other places that use errors="surrogateescape"
There was a problem hiding this comment.
When you compare the two in the dissect.target codebase I think it would be incorrect to state that surrogateescape 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 the shellbags plugin is broken as no encoding error handler is used when dealing with foreign volume names.
Using surrogateescape would still break the shellbags plugin as it is currently incompatible with flow.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 in flow.record.
A target system can contain non-utf-8 characters in a shellbag volume
volume_name. This would break theshellbagsplugin and is now fixed by escaping usingbackslashreplace.