-
-
Notifications
You must be signed in to change notification settings - Fork 171
poc: Try to extract an embedded .msi file #237
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
base: master
Are you sure you want to change the base?
Conversation
|
@activescott is this something you would be interested in? |
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.
In general I'm super supportive! 💪
I left a couple minor comments on the details. A couple other things I think need considered:
- tests - add a e2e test (we have a precedent, but feel free to ask if you want a pointer)
- ideally this would be in the core dll and could somehow work with the CLI and the windows GUI. If you want to keep this smaller, I'd just say move the core detection and extraction logic into the core dll and call it from the GUI - and create an issue to follow up on for you or someone else later to add it to CLI.
- What if there are multiple MSIs in an exe? Is that even a thing? I'm fine to ignore it too, just curious if you thought about it or know about it.
|
|
||
| static readonly Byte[] STORAGE_magic = new byte[]{ 0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1 }; | ||
|
|
||
| public static bool TryDetectMsiHeader(LessIO.Path filePath, out long offset) |
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 don't think you need the 4K buffer here. Since you're already in a MemoryMappedFile (good call!!), you can just use the view to scan through for the magic bits.
Minor: I might just return an offset of -1 instead of using an out param, but that's just a preference. up to you.
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.
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.
MMF is already memory (like C) so the extra buffer is probably redundant. It’s a nitpick. We can investigate later.
| { | ||
| if (eCatchAll is IOException && this.SelectedMsiFile.Extension.ToLower() == ".exe" && MsiDatabase.TryDetectMsiHeader(new LessIO.Path(this.SelectedMsiFile.FullName), out long offset)) | ||
| { | ||
| if (View.ShowUserMessageQuestionYesNo("It looks like this file contains a .msi file, do you want to extract 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.
Do you really think we need to ask? I would assume if the dropped an exe they want us to do it, so why not just do it and assume that's what they want?
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.
It drops a file on the users pc, and this file will be removed again later on.
I thought it would be nice, but if you have other ideas about this that works for me.
For the tests, can I just drop in the vmware setup or do I have to figure out the license for that? (it is about 30 MB)
Sure, I'll take a look at that
|
|
All good from my side. Let me know if you have a question. |
|
FYI @mega5800 check this out! |
6e7a117 to
a93adbc
Compare
@activescott I have added a test, I'm unsure if this is the exact test you were looking for? |
|
Test looks good to me. |
|
@mega5800 Could you please give this a try and let us know what you think? |
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.
Pull Request Overview
This PR implements a proof-of-concept feature to detect and extract embedded .msi files from .exe installers, allowing the tool to inspect MSI packages contained within executable wrappers like VMware tools setup.
- Added detection logic to scan for MSI file headers within executable files
- Enhanced GUI to prompt users when an embedded MSI is detected and offer extraction
- Added test coverage for the new MSI detection functionality
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/LessMsi.Core/Msi/MsiDatabase.cs | Added core logic for detecting MSI headers and extracting embedded MSI files |
| src/LessMsi.Gui/MainFormPresenter.cs | Integrated MSI detection into file loading workflow with user prompt |
| src/LessMsi.Gui/MainForm.cs | Added support for .exe files in drag-drop and user confirmation dialog |
| src/LessMsi.Gui/IMainFormView.cs | Added interface method for yes/no user prompts |
| src/Lessmsi.Tests/MiscTests.cs | Added test cases for the new MSI detection functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/LessMsi.Gui/IMainFormView.cs
Outdated
| /// Shows an error to the user. | ||
| /// </summary> | ||
| void ShowUserError(string formatStr, params object[] args); | ||
|
|
Copilot
AI
Oct 15, 2025
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 new interface method lacks XML documentation. It should include a summary describing the method's purpose and parameter documentation.
| /// <summary> | |
| /// Shows a Yes/No question message box to the user. | |
| /// </summary> | |
| /// <param name="message">The message to display to the user.</param> | |
| /// <returns>True if the user selects Yes; otherwise, false.</returns> |
|
Hi @activescott, let me check this PR during the weekend. Thanks |
5862bc7 to
c7ebd86
Compare
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.
Hello @learn-more.
Thank you for this PR.
I left some comments about the readability of the presented logic.
Please take a look and feel free to apply the ones you find helpful.
Thanks.
src/LessMsi.Core/Msi/MsiDatabase.cs
Outdated
| } | ||
|
|
||
|
|
||
| static readonly Byte[] STORAGE_magic = new byte[]{ 0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1 }; |
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 please give a meaningful name to the STORAGE_magic array
| while (true) | ||
| { | ||
| var totalRemaining = accessor.Capacity - readOffset; | ||
| if (totalRemaining <= 0) |
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 it's worth to extract totalRemaining <= 0 into a separate method, since you're using this bool statement twice
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 second condition is not strictly needed, if you prefer no repetition I'll just remove it.
| /// Displays a message to the user with a Yes/No question and returns the user's response. | ||
| /// </summary> | ||
| /// <param name="message">The message to display to the user.</param> | ||
| bool ShowUserMessageQuestionYesNo(string message); |
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.
why is this method necessary?
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.
Because we drop a file on disk for the user (even if it is just in the temp file),
I consider it good practice to ask it.
src/Lessmsi.Tests/MiscTests.cs
Outdated
| { | ||
| bool containsMsi = Msi.MsiDatabase.TryDetectMsiHeader(GetMsiTestFile("vmware_tools_setup.exe"), out long offset); | ||
| Assert.True(containsMsi); | ||
| Assert.Equal(0x315E3C, offset); |
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 does 0x315E3C mean?
Can you store it in a variable with a meaningful name?
src/Lessmsi.Tests/MiscTests.cs
Outdated
|
|
||
| containsMsi = Msi.MsiDatabase.TryDetectMsiHeader(GetMsiTestFile("IviNetSharedComponents32_Fx20_1.3.0.msi"), out offset); | ||
| Assert.True(containsMsi); | ||
| Assert.Equal(0, offset); |
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 does 0 mean?
Can you store it in a variable with a meaningful name?
src/Lessmsi.Tests/MiscTests.cs
Outdated
|
|
||
| containsMsi = Msi.MsiDatabase.TryDetectMsiHeader(GetMsiTestFile("msi_with_external_cab.cab"), out offset); | ||
| Assert.False(containsMsi); | ||
| Assert.Equal(-1, offset); |
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 does -1 mean?
Can you store it in a variable with a meaningful name?
c7ebd86 to
bbb75d0
Compare
Hello @mega5800, Thanks for the review! I do believe that all comments should be resolved or answered now, Thanks. |
This Proof of Concept code tries to find a .msi file embedded inside an .exe,
if the .exe cannot be loaded directly as msi.
This allows to inspect the .msi file from the VMware tools setup.
It is a quick & dirty implementation.
Is this something you would be interested in moving forward?
If so, please suggest changes you'd like to see.