-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Use AMSI for archive malware scanning#4910
Use AMSI for archive malware scanning #4910pl4nty wants to merge 3 commits intomicrosoft:master from
Conversation
* Remove `#include <pure.h>` and add necessary includes for AMSI in `src/AppInstallerCommonCore/Archive.cpp` * Initialize AMSI, create a session, scan the file, and handle results in `ScanZipFile` function * Add tests for new archive formats in `src/AppInstallerCLITests/Archive.cpp` - Add test cases for 7z, Rar, TarGz, and TarBz2 archive formats - Verify extraction and scanning of these new archive formats
This comment has been minimized.
This comment has been minimized.
@florelis
florelis
left a comment
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 AMSI was a suitable replacement to PureLib, I'd be all for changing it just to have one less external dependency.
But I don't think it is a good replacement based on the documentation. I only see mentions of AMSI supporting scripts or .NET binaries, while PureLib is for scanning ZIP files specifically. For example, it's not clear to me that AMSI would catch a zip bomb.
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.
This may be me being paranoid but I would be slightly concerned about having a binary checked without knowing what it is. Would be great if you could share how it gas generated so that we can validate that it's that (assuming it's a deterministic process)
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.
Huh, I wonder why we don't have the other dependencies here. I think they are listed in the projects that produce the final binaries, like WindowsPackageManager.vcxproj. Maybe we could avoid some duplication by having the dependencies at the point they are introduced...
(Not a comment for this PR)
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'm not sure if we need it in pch.h if it is only used in Archive.cpp. But I do see the TODO comment from before about moving 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.
IF we do decide to go with AMSI instead of PureLib, there are a bunch other places we have to remove it from. We have the source code at src/PureLib/, all the references in AdditionalIncludeDirectories and AdditionalDependencies, and removing the project from the solution. There's also the some housekeeping to do with the cgmanifest and the notice file
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.
These should use one of the WIL magic RAII wrappers to make the destruction easier and ensure it happens. I don't see any specific for AMSI, so it would probably be a wil::unique_any
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.
This is missing
For example, it's not clear to me that AMSI would catch a zip bomb.
Most antivirus software should be able to detect zip bomb. I'm using ESET and just tried out AMSI with this code:
https://gist.github.com/SpecterShell/c2b5507ba9e5c531faead38c7d0b877e
And ESET passed me a notification for detecting the zip bomb.
EDIT: Microsoft Defender doesn't report on the zip bomb though.
Uh oh!
There was an error while loading. Please reload this page.
Replace pure with AMSI for archive malware scanning, to allow archive types other than zip. Added tests for
tgzwitharchiveExtractionMethod tarand a stub for testing a large file. I tested with a 1GB compressed 7z and scanning took slightly under two minutes on a 4 core VM.This was partially an experiment to see how Copilot Workspace handles C++/Windows, no worries if this feature isn't useful.
For more details, open the Copilot Workspace session.
Microsoft Reviewers: Open in CodeFlow