-
Notifications
You must be signed in to change notification settings - Fork 74
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 googlechrome to VM-Packages #779
Conversation
To address issue mandiant#469, added: 1. download/install Chrome by checking if installer's signature is valid 2. add associations for opening url/html file in Chrome 3. disable Chrome auto-update related services and registries
1. fix lint issues 2. correct the tool category 3. remove shortcut on desktop
1. fix lint issues
1. fix lint issues
To address issue mandiant#469, added: 1. download/install Chrome by checking if installer's signature is valid 2. add associations for opening url/html file in Chrome 3. disable Chrome auto-update related services and registries
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.
Thanks for all the work @binjo!
- **If the package is out of date please check [Version History](#versionhistory) for the latest submitted version. If you have a question, please ask it in [Chocolatey Community Package Discussions](https://github.com/chocolatey-community/chocolatey-packages/discussions) or raise an issue on the [Chocolatey Community Packages Repository](https://github.com/chocolatey-community/chocolatey-packages/issues) if you have problems with the package. Disqus comments will generally not be responded to.** | ||
]]></description> | ||
<dependencies> | ||
<dependency id="chocolatey-core.extension" /> |
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.
[question] Why do we need this dependency?
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 Get-PackageCacheLocation
method is exported from chocolatey-core.extension
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 about downloading the installer to the %TEMP%
directory instead as we do in other packages?
Join-Path ${Env:TEMP} $installerName
This way we don't need an extra dependency
try { | ||
if ($sigValid) { | ||
$toolName = 'Google Chrome' | ||
$category = 'Utilities' |
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.
Are you using the category? I think we don't need to add Chrome to any category, just install it in the system, as it is a dependency for other tools (cyberchef) not really an analysis tool.
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 lint
required the $category
, otherwise the CI won't be happy 😂
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.
Add the package to the exclusions for this linter rule instead: https://github.com/mandiant/VM-Packages/blob/main/scripts/test/lint.py#L308 😉
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 is still here 👀
Co-authored-by: Ana María Martínez Gómez <[email protected]>
Co-authored-by: Ana María Martínez Gómez <[email protected]>
Co-authored-by: Ana María Martínez Gómez <[email protected]>
$installer = 'googlechromeinstaller.msi' | ||
$packageArgs = @{ | ||
packageName = $env:ChocolateyPackageName | ||
file = Join-Path $downloadDir $installer |
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 we should save this to a variable to access it as $installerFile
instead of $packageArgs['file']
. A bit nicer and easier to read.
- **If the package is out of date please check [Version History](#versionhistory) for the latest submitted version. If you have a question, please ask it in [Chocolatey Community Package Discussions](https://github.com/chocolatey-community/chocolatey-packages/discussions) or raise an issue on the [Chocolatey Community Packages Repository](https://github.com/chocolatey-community/chocolatey-packages/issues) if you have problems with the package. Disqus comments will generally not be responded to.** | ||
]]></description> | ||
<dependencies> | ||
<dependency id="chocolatey-core.extension" /> |
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 about downloading the installer to the %TEMP%
directory instead as we do in other packages?
Join-Path ${Env:TEMP} $installerName
This way we don't need an extra dependency
try { | ||
if ($sigValid) { | ||
$toolName = 'Google Chrome' | ||
$category = 'Utilities' |
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.
Add the package to the exclusions for this linter rule instead: https://github.com/mandiant/VM-Packages/blob/main/scripts/test/lint.py#L308 😉
Set-PTA ChromeHTML http | ||
Set-PTA ChromeHTML https | ||
Set-FTA ChromeHTML .htm | ||
Set-FTA ChromeHTML .html |
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.
my testing env seems working, after installing cyberchef.vm
, the default opening .html
file is Chrome without the prompt as yours.
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 have tested again in a tested environment and opening the cyberchef lnk to the html file shows this pop up. @binjo how is your testing environment set up?
Disable-Chrome-Updates | ||
|
||
# Delete Desktop shortcut | ||
$desktopShortcut = Join-Path ${Env:Public} "Desktop\$toolName.lnk" |
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 the correct path is
$desktopShortcut = Join-Path ${Env:Public} "Desktop\$toolName.lnk" | |
$desktopShortcut = Join-Path ${Env:UserProfile} "Desktop\$toolName.lnk" |
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.
$Env:Public
is correct one, the installer created .lnk
is in the public folder, however I just noticed when open the Chrome for the first time, it will create another .lnk
which is in $Env:UserProfile
😂
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 would be a good opportunity to add a VM-Delete-Desktop-Shortcut
function that just checks both directories. Maybe not required for this PR but that function has been on my list for a while
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 just tested again, the installer created .lnk
file is in the public folder, after install cyberchef.vm
and open the .html
file, a new Chrome .lnk
shortcut file appeared in $Env:UserProfile
. This looks infeasible for post install deletion.
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.
we could open chrome in the installer, close it, and then delete the lnk. But it seems the link also appears if I run Stop-Process -Name explorer -Force
. Strange 😩
try { | ||
if ($sigValid) { | ||
$toolName = 'Google Chrome' | ||
$category = 'Utilities' |
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 is still here 👀
# Download the installer | ||
$packageArgs['file'] = Get-ChocolateyWebFile @packageArgs | ||
|
||
$sigValid = (Get-AuthenticodeSignature -FilePath $packageArgs['file']).Status -eq 'Valid' |
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 wonder if it is enough with checking that it has a valid signature of if we need to check that the signer certificate comes from Google Chrome as well? @vm-packages opinions? (We can extract this signature to a helper for other tools in the future, so it is a general question)
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 another function that has been on my list; VM-Assert-Signature
. I think we should maybe open a PR for this function and hash it out there
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.
Personally I think I'd be ok with just checking if it is Valid since that will check it against CAs, so self signed binaries wont be shown as valid
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.
@day1player I think we should either introduce VM-Assert-Signature
or a helper function to install tools using the signature instead of the hash.
} | ||
|
||
# https://github.com/chocolatey-community/chocolatey-packages/blob/master/extensions/chocolatey-core.extension/extensions/Get-PackageCacheLocation.ps1 | ||
function Get-PackageCacheLocation { |
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 function needed? I think this is equivalent to using Join-Path ${Env:TEMP} $installerName
directly as we do in other packages
Set-PTA ChromeHTML http | ||
Set-PTA ChromeHTML https | ||
Set-FTA ChromeHTML .htm | ||
Set-FTA ChromeHTML .html |
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 have tested again in a tested environment and opening the cyberchef lnk to the html file shows this pop up. @binjo how is your testing environment set up?
Disable-Chrome-Updates | ||
|
||
# Delete Desktop shortcut | ||
$desktopShortcut = Join-Path ${Env:Public} "Desktop\$toolName.lnk" |
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.
we could open chrome in the installer, close it, and then delete the lnk. But it seems the link also appears if I run Stop-Process -Name explorer -Force
. Strange 😩
<id>googlechrome.vm</id> | ||
<version>0.0.0.20231210</version> | ||
<authors>Mandiant</authors> | ||
<description>Chrome is a fast, simple, and secure web browser, built for the modern web.</description> |
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.
[nitpick] I would short this description
<metadata> | ||
<id>googlechrome.vm</id> | ||
<version>0.0.0.20231210</version> | ||
<authors>Mandiant</authors> |
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 field should contain the authors of the tool
<authors>Mandiant</authors> | |
<authors>Google LLC.</authors> |
$installer = 'googlechromeinstaller.msi' | ||
$packageArgs = @{ | ||
packageName = $env:ChocolateyPackageName | ||
file = Join-Path $downloadDir $installer |
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.
According to the documentation Get-ChocolateyWebFile
needs fileFullPath
. Maybe it works because it is the second argument. 🤔
. $toolsPath\helpers.ps1 | ||
|
||
$downloadDir = Get-PackageCacheLocation | ||
$installer = 'googlechromeinstaller.msi' |
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.
[nitpick] I would remove this variable as it is only used once
fileType = 'MSI' | ||
silentArgs = "/quiet /norestart /l*v `"$($env:TEMP)\$($env:chocolateyPackageName).$($env:chocolateyPackageVersion).MsiInstall.log`"" | ||
validExitCodes = @(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.
fileType = 'MSI' | |
silentArgs = "/quiet /norestart /l*v `"$($env:TEMP)\$($env:chocolateyPackageName).$($env:chocolateyPackageVersion).MsiInstall.log`"" | |
validExitCodes = @(0) |
I think we should use different arguments for Install-ChocolateyInstallPackage
and Get-ChocolateyWebFile
as they don't really overlap and it makes more difficult to read the code.
fileType = 'MSI' | ||
silentArgs = "/quiet /norestart /l*v `"$($env:TEMP)\$($env:chocolateyPackageName).$($env:chocolateyPackageVersion).MsiInstall.log`"" | ||
validExitCodes = @(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.
validExitCodes = @(0)
is the default, so it can be removed
Set-FTA ChromeHTML .html | ||
|
||
VM-Write-Log "INFO" "Disable update settings" | ||
Disable-Chrome-Updates |
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.
@binjo disabling updates was not part of the open issues related to chrome, why do you think is needed?
Does anyone has any concern with possible side-effects? 🤔
I have extracted the signature validation and creation of chrome #824 to this PR: #824 I have extracted the rest of the discussions in this PR to the following issues:
I think this will make the review/discussion process a bit easier. 😉 |
To address issue #469, added: