Ryan2065 / Log4ShellDetection

MIT License
5 stars 1 forks source link

feedback #1

Closed MaartenPauchet closed 2 years ago

MaartenPauchet commented 2 years ago

Hi Ryan2065,

thanks for the script. I've been playing with it for a while. I like the fact that your script is fast and recursively goes into .jar files, as well as the reporting options combined with MECM HW inventory to get the results into the database for reporting.

Initially had a problem with the custom CLASS you used in 1.3.0 but I was happy to see you fixed it in the meanwhile with 1.3.1. Custom Classes only exist from powershell 5 on I think and we had some servers with old PS editions.

I've made a couple of changes as well. maybe there's something you can use:

  1. Added extra OutputType = 'MECM' when this is selected even the start & stop transcript lines do not return anything to screen. It writes the result to the registry just like that OutputOption but the only feedback returned to the sdout is the number of found vulnerable jar files. This can be used in a baseline CI. $VulnerableFiles = $($VulnerableFiles.Count) $VulnerableFiles #echo number of vulnerable files back to MECM

  2. Adjusted the robocopy command to skip 0 byte files and run in the idle cpu time so it does not immediately take full cpu % $results += (& cmd /c START "" /LOW /B robocopy /l "$($root)" null *.jar *.war *.ear /MIN:1 /ns /njh /njs /np /nc /ndl /xjd /mt /s).trim()

  3. StreamReader ran into a problem with the line $sr = [System.IO.StreamReader]::new($Stream) on servers with older .net versions. We changed that to $sr = New-Object System.IO.StreamReader ($Stream) which seems more forgiving in some cases.

  4. Drives to process: Your get-PSDrive command also returns the drive letters for mapped network drives which can make robocopy run very long when it starts to process those also. So we changed that to only get local drive letters : $Roots = Get-WmiObject -Class win32_logicaldisk | Where-Object {$_.DriveType -eq 3} | Select-Object -ExpandProperty DeviceID

Also came accross something I do not understand: Why do you add the CVE-2021-4104 vulnerability to each 2.xx jar release in the Function Get-Log4ShellIdentifiers? I see that this makes otherwise not vulnerable files like a genuine log4j-api-2.13.3.jar get flagged as vulnerable. While I would expect only the log4j-core-2.13.3.jar to be flagged as that one contains the jndiLookup.class & JndiManager.class.

And a last question: are you planning to add CVE-2021-44832 in your detection routine also?

Thanks again for all the work. Maarten

Ryan2065 commented 2 years ago

Thank you for the feedback, sorry it took awhile to get back - everyone in the family suddenly got sick and my wife just had surgery, so it's been a marathon here.

  1. Yeah, that's a good idea. Transcript can be disabled already from the command line, but the output probably doesn't need to return either way. I work somewhere where the CI timeout is 60 seconds so never think to do the CI route for longer scripts.
  2. Now that's really nice. I'll look at implementing something like that.
  3. That's good to know - will make the switch
  4. I could add a switch or something for drives to exclude network drives. Was thinking if someone mapped a personal network drive, that should probably be scanned also, but can see how that'd be a problem - especially if the server hosting the network drives is scanned itself

I'll take a look at the detection issue. I actually wrote a script to scrape Maven and mark different versions as vulnerable based on Maven results. That script tries to get unique lines from Manifest.mf to detect good/bad versions of log4j. My assumption is the detection says it's a vulnerable version with no bad class files. I believe I did not find a unique enough string for the manifest and it's incorrectly identifying some pieces. I think I'll just hash the manifest.mf file itself like the .class files for detection.

For the additional CVE - I can add it. I did not know there was yet another RCE bug in Log4j! Hard to keep up.

Ryan2065 commented 2 years ago

Thank you for the feedback. It should all be incorporated now.

Let me know if you have any more issues!