Skip to content

Adding Process analysis#1641

Closed
iserrano76 wants to merge 8 commits into
microsoft:mainfrom
iserrano76:Test-ExchAVExclusions
Closed

Adding Process analysis#1641
iserrano76 wants to merge 8 commits into
microsoft:mainfrom
iserrano76:Test-ExchAVExclusions

Conversation

@iserrano76
Copy link
Copy Markdown
Contributor

Issue:
Included a Test for Exchange processes looking for 3rd party DLLs

Reason:
Include processes test in the script.
It is based in the #1130 PR but Including signature validation of the files.

Fix:
Check the files running are signed by a know issuer (Microsoft or VeriSign in case of FIPS files)
If the file is not signed, it will verify the company or the module name is well-known.

Validation:
Test it in lab.

@iserrano76 iserrano76 requested a review from a team as a code owner March 30, 2023 13:33
@lusassl-msft
Copy link
Copy Markdown
Contributor

@iserrano76 spell check failed:
image

Comment thread Diagnostics/AVTester/Test-ExchAVExclusions.ps1 Outdated
Comment thread Diagnostics/AVTester/Test-ExchAVExclusions.ps1 Outdated
Comment thread Diagnostics/AVTester/Test-ExchAVExclusions.ps1 Outdated
Comment thread Diagnostics/AVTester/Test-ExchAVExclusions.ps1 Outdated
@lusassl-msft lusassl-msft added the Ready for review Pull Request is ready to be reviewed label May 15, 2023
Comment on lines +215 to +228
$response = $null
$response = Invoke-WebRequest http://crl.microsoft.com/pki/crl/products/CodeSigPCA.crl
if ($null -ne $response) {
if ( $response.StatusCode -eq 200 ) {
$Offline = $false
} else {
$Offline = $true
}
} else {
$Offline = $true
}
# If -recurse then we need to find all SubFolders and Add them to the list to be tested
if ($Recurse) {

# Add the root folder
$FolderList.Add($path.ToLower())
} catch {
$Offline = $true
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can make this simpler:
try {
$response = $null
$Offline = $true
$response = Invoke-WebRequest http://crl.microsoft.com/pki/crl/products/CodeSigPCA.crl
$Offline = $null -eq $response -or $response.StatusCode -ne 200
} catch {
Write-verbose "Failed web request with inner error: $_"
}

Comment on lines +432 to +439
if ( ($module.FileName.ToLower().StartsWith(('c:\windows\assembly\NativeImages_').ToLower()) -and (
$module.FileName.ToLower().EndsWith('.ni.dll') -or $module.FileName.ToLower().EndsWith('.ni.exe') -or
$module.FileName.ToLower().EndsWith('.wrapper.dll') ) ) -or
($module.FileName.ToLower().StartsWith('c:\windows\system32\') -and [environment]::OSVersion.Version.Major -le 6 ) -or
($module.FileName.ToLower().StartsWith(('c:\windows\WinSxS\').ToLower()) -and [environment]::OSVersion.Version.Major -le 6 ) -or
($module.FileName.ToLower().StartsWith('c:\windows\assembly\') -and [environment]::OSVersion.Version.Major -le 6 ) -or
($module.FileName.ToLower().StartsWith('c:\windows\microsoft.net\assembly\gac_64\') -and [environment]::OSVersion.Version.Major -le 6 ) -or
($module.FileName.ToLower() -eq (Join-Path ($env:ExchangeInstallPath).ToLower() 'bin\osafehtm.dll') -and ([byte]$serverExchangeInstallDirectory.MsiProductMinor) -gt 0 ) ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add comment for what we are trying to do here. This is extremely complicated and need more context here. But I believe this is testing for things that aren't signed for known modules that are on Windows 2012 R2 or below.

We should just do a list of known unsigned files if it isn't long. As this would be better for security reasons.

$BadProcessList["$($Instance.Id) - $($module.FileName)"] = "$($Instance.Id), $($module.FileName), $($module.Company), $($module.Description), $($module.Product), $($Instance.Path), $CommandLine"
Write-SimpleLogFile -String ("[FAIL] - We could not get root certificate on Module: $($module.FileName) `n`tCompany: $($module.Company) `n`tDescription: $($module.Description) `n`tProduct: $($module.Product) `n`tProcess ID: $($Instance.Id) `n`tCommand Line: $CommandLine") -name $LogFile -OutHost
} else {
$FIPSPath = Join-Path $env:ExchangeInstallPath 'FIP-FS\Bin\TE'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Variable set and never changed inside of a loop. Best practice is to not do this.

$BadProcessList["$($Instance.Id) - $($module.FileName)"] = "$($Instance.Id), $($module.FileName), $($module.Company), $($module.Description), $($module.Product), $($Instance.Path), $CommandLine"
Write-SimpleLogFile -String ("[FAIL] - We could not get root certificate on Module: $($module.FileName) `n`tCompany: $($module.Company) `n`tDescription: $($module.Description) `n`tProduct: $($module.Product) `n`tProcess ID: $($Instance.Id) `n`tCommand Line: $CommandLine") -name $LogFile -OutHost
} else {
$FIPSPath = Join-Path $env:ExchangeInstallPath 'FIP-FS\Bin\TE'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using $env:ExchangeInstallPath but not a guarantee that it is there. We already define the value in $ExchangePath we should be using that.

Comment on lines +469 to +472
if ( -not (CheckIfISAcceptedRootCA -CAString $rootCertificate -isFIPFS -Offline) ) {
$BadProcessList["$($Instance.Id) - $($module.FileName)"] = "$($Instance.Id), $($module.FileName), $($module.Company), $($module.Description), $($module.Product), $($Instance.Path), $CommandLine"
Write-SimpleLogFile -String ("[FAIL] - root do not expected (FIP-FS) on Module: $($module.FileName) `n`tCompany: $($module.Company) `n`tDescription: $($module.Description) `n`tProduct: $($module.Product) `n`tRoot CA $rootCertificate `n`tProcess ID: $($Instance.Id) `n`tCommand Line: $CommandLine") -name $LogFile -OutHost
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are we checking to see if the module is signed correctly in this script?

This script is to check for AV Exclusions the easiest way possible. If there is a threat concern that we are trying to validate here. That would need to be done in a different script, not in an AV Exclusions script.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We are checking that the modules are signed correctly by well-know CAs.

CN=Microsoft Corporate Root CA, O=Microsoft Corporation
CN=Microsoft Root Authority, OU=Microsoft Corporation, OU=Copyright (c) 1997 Microsoft Corp.
CN=Microsoft Root Certificate Authority, DC=microsoft, DC=com
CN=Microsoft Root Certificate Authority 2010, O=Microsoft Corporation, L=Redmond, S=Washington, C=US
CN=Microsoft Root Certificate Authority 2011, O=Microsoft Corporation, L=Redmond, S=Washington, C=US

In case or FIPS files Verisign is valid as well.

CN=VeriSign Class 3 Public Primary Certification Authority - G5, OU="(c) 2006 VeriSign, Inc. - For authorized use only", OU=VeriSign Trust Network, O="VeriSign, Inc.", C=US

I think it is better to use the signature than the company name in the file.
In this way, we can verify that the module is not from a different company and that our modules were not modified.

@dpaulson45
Copy link
Copy Markdown
Member

Closing PR, as this has already been updated.

@dpaulson45 dpaulson45 closed this Jul 18, 2023
@iserrano76 iserrano76 deleted the Test-ExchAVExclusions branch July 19, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Pull Request is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants