datto / log4shell-tool

Log4Shell Enumeration, Mitigation and Attack Detection Tool
Apache License 2.0
15 stars 13 forks source link

$varDrives not an array with single drive found #8

Closed theologos7 closed 2 years ago

theologos7 commented 2 years ago

On lines 88 and 91 of the script, you should wrap the entire Get-WmiObject call in @() or else when a single drive is returned and you try to foreach into $vardrives this might happen:

~> $vardrives = Get-WmiObject -Class Win32_logicaldisk | ? {$_.DriveType -eq 2 -or $_.DriveType -eq 3} | ? {$_.FreeSpace} | % {$_.DeviceID}
~> $varDrives[0]
C
~> $varDrives[1]
:
~> $vardrives = @(Get-WmiObject -Class Win32_logicaldisk | ? {$_.DriveType -eq 2 -or $_.DriveType -eq 3} | ? {$_.FreeSpace} | % {$_.DeviceID})
~> $varDrives[0]
C:
Datto-StanLee commented 2 years ago

Can I ask why this was closed? Is this not the case or is it not relevant within the context of the script?

theologos7 commented 2 years ago

I must have misclicked! Did not mean to close it. Technically this isn't going to happen on newer versions of PowerShell because foreach doesn't index into a string, but I believe it's best practice to always wrap returns like this in @() just to make sure nothing odd happens. Sorry for the erroneous close.

Datto-StanLee commented 2 years ago

This is a really good catch, thank you. I try and write with PS2.0 compatibility in mind all the time but it's always finding new ways to catch me out. I didn't encounter this during testing; would you say that the way to reproduce this would be to instruct the script to search all drives but make only one available on a PS2.0 system, forcing it to index "C:" into two separate elements? Cheers – SL

theologos7 commented 2 years ago

To replicate, I'd just use what you currently have for $env:usrScanscope = 2 and run on a machine with only one drive. Replicating myself, it looks like even on v2 foreach doesn't index into a string, but if you try to directly index it will, so wrapping in @() is more of a best practice in this case because it will technically still work either way. Sorry for the false positive. This can be closed.

~> powershell -v 2
~> $drives = Get-WmiObject -Class Win32_logicaldisk | ? {$_.DriveType -eq 2 -or $_.DriveType -eq 3} | ? {$_.FreeSpace} | % {$_.DeviceID}
~> $drives #is a single string
C:
~> $drives[0]
C
~> $drives[1]
:
~> foreach($drive in $drives) {$drive} #doesn't index into the string
C:
~> powershell -v 2
~> $drives = @(Get-WmiObject -Class Win32_logicaldisk | ? {$_.DriveType -eq 2 -or $_.DriveType -eq 3} | ? {$_.FreeSpace} | % {$_.DeviceID}) # wrapping in @()
~> $drives #is an array
C:
~> $drives[0]
C:
~> $drives[1] # null
~> foreach($drive in $drives) {$drive} #indexes into the array
C: