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

Add Defender Quarantine Recovery #96

Merged
merged 12 commits into from Jan 2, 2023

Conversation

MaxGroot
Copy link
Contributor

@MaxGroot MaxGroot commented Dec 2, 2022

Initial version, requires additional testing.

(DIS-1573)

Initial version, requires additional testing.

(DIS-1573)
@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Merging #96 (4d31688) into main (f227175) will increase coverage by 0.32%.
The diff coverage is 92.50%.

@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
+ Coverage   58.54%   58.86%   +0.32%     
==========================================
  Files         189      189              
  Lines       14754    14900     +146     
==========================================
+ Hits         8637     8771     +134     
- Misses       6117     6129      +12     
Flag Coverage Δ
unittests 58.86% <92.50%> (+0.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dissect/target/plugins/os/windows/defender.py 93.37% <92.50%> (-6.63%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Bugfix. Sometimes The filename is not the complete resource_id.
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
tests/test_plugins_os_windows_defender.py Show resolved Hide resolved
tests/test_plugins_os_windows_defender.py Outdated Show resolved Hide resolved
tests/test_plugins_os_windows_defender.py Outdated Show resolved Hide resolved
MaxGroot and others added 4 commits December 16, 2022 11:26
Co-authored-by: Erik Schamper <1254028+Schamper@users.noreply.github.com>
Refactor parsing into quarantine entry (resource) classes

Move several constants around for better reading order

Add doc strings
Per code review suggestions
@MaxGroot
Copy link
Contributor Author

@Schamper I implemented your suggested changes, thanks :) While I like the approach of refactoring most of the parsing stuff into the QuarantineEntry and QuarantineEntryResource classes, I have a little problem with the hierarchy.

QuarantineEntry now has a property resources, which is a list of QuarantineEntryResource instances. However, for both plugin functions, in the end the plugin yields results based on all available instances of QuarantineEntryResource. This means I have to loop over all QuarantineEntry objects, and then within that for loop, loop over the QuarantineEntryResource objects that said entry contains. This means that both plugin functions are now nested one level deeper, which hurts readability a little. If you have any ideas in that regard, that would be great (:

dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
Small docstring changes
Variable renames
Typehints
@MaxGroot
Copy link
Contributor Author

@Schamper suggestions implemented :)

Due to a bug, we encountered a situation where only a part of the
resource_id was found in the acquired evidence.
This led to the invalid assumption on my end that sometimes
defender will only use a part of the resource id for the filename of
a ResourceData file.

Now, we assume the a quarantine entry's resource_id will be the
filename found in the ResourceData folder.
This also aligns with what we saw when investigating mpengine.dll.
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
dissect/target/plugins/os/windows/defender.py Outdated Show resolved Hide resolved
tests/test_plugins_os_windows_defender.py Outdated Show resolved Hide resolved
Comment on lines 4 to 10
from flow.record import Record

import dissect.util.ts as ts
from dissect.cstruct import Structure, cstruct
from dissect.target import plugin
from dissect.target.helpers.record import TargetRecordDescriptor
from flow.record import Record
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you run isort? You can optionally add the following configuration to your .vscode/settings.json:

    "editor.codeActionsOnSave": {
        "source.organizeImports": true
    }

@Schamper Schamper merged commit 669aca6 into fox-it:main Jan 2, 2023
Poeloe pushed a commit that referenced this pull request Feb 29, 2024
@MaxGroot MaxGroot deleted the feature/defender_quarantine branch March 9, 2024 19:49
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.

None yet

2 participants