-
Notifications
You must be signed in to change notification settings - Fork 53
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
Microsoft Office add-in detection #966
base: main
Are you sure you want to change the base?
Conversation
16bdf78
to
805ab29
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #966 +/- ##
==========================================
+ Coverage 77.72% 77.85% +0.13%
==========================================
Files 326 328 +2
Lines 28571 28863 +292
==========================================
+ Hits 22206 22472 +266
- Misses 6365 6391 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
95ab63d
to
d55a0c4
Compare
dissect/target/helpers/regutil.py
Outdated
@@ -172,6 +172,18 @@ def value(self, value: str) -> RegistryValue: | |||
""" | |||
raise NotImplementedError() | |||
|
|||
def value_or_default(self, value: str, default: ValueType = None) -> RegistryValue: |
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.
Can you roll this into the value
method so it's more in line with standard library Python code?
@internal | ||
def value(self, key: str, value: str) -> ValueCollection: | ||
"""Convenience method for accessing a specific value.""" | ||
return self.key(key).value(value) | ||
|
||
@internal | ||
def value_or_empty(self, key: str, value: str) -> ValueCollection: |
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.
Same here.
@@ -282,11 +286,31 @@ def key(self, key: Optional[str] = None) -> KeyCollection: | |||
|
|||
return res | |||
|
|||
@internal | |||
def key_or_empty(self, key: Optional[str] = None) -> KeyCollection: |
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.
Same with this one.
6338cec
to
755f6cf
Compare
@@ -29,17 +29,20 @@ def findall(buf: bytes, needle: bytes) -> Iterator[int]: | |||
T = TypeVar("T") | |||
|
|||
|
|||
def to_list(value: T | list[T]) -> list[T]: | |||
"""Convert a single value or a list of values to a list. | |||
def to_list(value: T | list[T] | None) -> list[T]: |
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.
To Miauwkeru: For our usecases, your implementation won in the end.
375faea
to
951e4e3
Compare
@@ -299,10 +303,10 @@ def iterkeys(self, keys: Union[str, list[str]]) -> Iterator[KeyCollection]: | |||
yield key | |||
|
|||
@internal | |||
def keys(self, keys: Union[str, list[str]]) -> Iterator[KeyCollection]: | |||
def keys(self, keys: Union[str, list[str]]) -> Iterator[RegistryKey]: |
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.
def keys(self, keys: Union[str, list[str]]) -> Iterator[RegistryKey]: | |
def keys(self, keys: str | list[str]) -> Iterator[RegistryKey]: |
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.
bump
@@ -316,6 +320,19 @@ def keys(self, keys: Union[str, list[str]]) -> Iterator[KeyCollection]: | |||
except HiveUnavailableError: | |||
pass | |||
|
|||
@internal | |||
def values(self, keys: Union[str, list[str]], value: str) -> Iterator[RegistryValue]: |
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.
def values(self, keys: Union[str, list[str]], value: str) -> Iterator[RegistryValue]: | |
def values(self, keys: str | list[str], value: str) -> Iterator[RegistryValue]: |
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.
bump
- https://learn.microsoft.com/en-us/office/dev/add-ins/overview/office-add-ins | ||
- https://learn.microsoft.com/en-us/visualstudio/vsto/registry-entries-for-vsto-add-ins | ||
|
||
Yields a OfficeNativeAddinRecord with fields: |
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.
Yields a OfficeNativeAddinRecord with fields: | |
Yields a ``OfficeNativeAddinRecord`` with fields: |
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.
bump
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 think this one was already processed?
dissect/target/helpers/regutil.py
Outdated
except RegistryValueNotFoundError: | ||
if default is self.__marker: | ||
raise | ||
return VirtualValue(VirtualHive(), value, default) |
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.
Wouldn't we still want the actual hive to be linked?
dissect/target/loaders/cb.py
Outdated
@@ -161,7 +161,7 @@ def subkey(self, subkey: str) -> CbRegistryKey: | |||
def subkeys(self) -> list[CbRegistryKey]: | |||
return list(map(self.subkey, self.data["sub_keys"])) | |||
|
|||
def value(self, value: str) -> str: | |||
def _value(self, value: str) -> str: |
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.
Maybe update these return types while we're at 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.
I love types, so happy to do it.
But is there light at the end of the tunnel?
Unfortunately, a lot of the type annotations are incorrect. As I am used to inferring the behavior of a function by its type signature, this has put me on the wrong foot on multiple occasions. The type annotations are incorrect because the code is not type checked, either in CI or locally. I think one of the problems with enforcing type checking is that core dependencies such as flow record and cstruct use dynamic typing.
I believe @Miauwkeru mentioned a solution for somehow adding type information to cstruct. Or we could use code generation at "compile-time" instead of during run-time.
Happy to help out with this (also in my own time). However, I also think a principled decision needs to be made to stop relying on techniques which use dynamic types.
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.
The .pyi
generation for cstruct was indeed my idea, of which Miauwkeru made a POC. Happy to hear other suggestions of yours.
However, 95% of code in dissect.target
does not interact with flow.record
or dissect.cstruct
, and we still want accurate type information there. I agree that it's best if we can check that in CI or locally. So I think the best way forward for the time being is to figure out if there's some way to do that and ignore all flow.record
and dissect.cstruct
types while we work separately on a solution for those dynamic types.
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 take it the return type can be covariant?)
dissect/target/loaders/smb.py
Outdated
@@ -308,7 +308,7 @@ def subkeys(self) -> list[SmbRegistryKey]: | |||
|
|||
return subkeys | |||
|
|||
def value(self, value: str) -> str: | |||
def _value(self, value: str) -> str: |
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.
Maybe update these return types while we're at it?
7439492
to
97a57f4
Compare
dissect/target/helpers/utils.py
Outdated
if not isinstance(value, list): | ||
if value is None: | ||
return [] | ||
elif not isinstance(value, list): |
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.
elif not isinstance(value, list): | |
if not isinstance(value, list): |
nativePluginStatus = self._parse_plugin_status(addin) | ||
yield OfficeNativeAddinRecord( | ||
name=addin.value("FriendlyName", None).value, | ||
modification_time=addin.timestamp, | ||
loaded=nativePluginStatus.loaded if nativePluginStatus else None, | ||
load_behavior=nativePluginStatus.load_behavior.name if nativePluginStatus else None, | ||
type=addin_type, | ||
manifest=windows_path(manifest_path_str) if manifest_path_str else None, | ||
codebases=executables, | ||
) |
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.
Any reason this is not snake cased?
nativePluginStatus = self._parse_plugin_status(addin) | |
yield OfficeNativeAddinRecord( | |
name=addin.value("FriendlyName", None).value, | |
modification_time=addin.timestamp, | |
loaded=nativePluginStatus.loaded if nativePluginStatus else None, | |
load_behavior=nativePluginStatus.load_behavior.name if nativePluginStatus else None, | |
type=addin_type, | |
manifest=windows_path(manifest_path_str) if manifest_path_str else None, | |
codebases=executables, | |
) | |
native_plugin_status = self._parse_plugin_status(addin) | |
yield OfficeNativeAddinRecord( | |
name=addin.value("FriendlyName", None).value, | |
modification_time=addin.timestamp, | |
loaded=native_plugin_status.loaded if nativePluginStatus else None, | |
load_behavior=native_plugin_status.load_behavior.name if native_plugin_status else None, | |
type=addin_type, | |
manifest=windows_path(manifest_path_str) if manifest_path_str else None, | |
codebases=executables, | |
) |
- fix regression in web plugin name
- cache office component root dirs
7888515
to
d3ec8e4
Compare
Detects:
Limitations:
Bonus: