mergebase / log4j-detector

A public open sourced tool. Log4J scanner that detects vulnerable Log4J versions (CVE-2021-44228, CVE-2021-45046, etc) on your file-system within any application. It is able to even find Log4J instances that are hidden several layers deep. Works on Linux, Windows, and Mac, and everywhere else Java runs, too! TAG_OS_TOOL, OWNER_KELLY, DC_PUBLIC
Other
638 stars 98 forks source link

pom.properties processing #53

Open HynekPetrak opened 2 years ago

HynekPetrak commented 2 years ago

Hi, I'm not sure whether that's the right way to go, I'm sorry if I did not understand your code properly. If you set the empirical condition variables based on version detected via pom.properties, this may actually lead to false positives. Take for instance: https://www.apache.org/dyn/closer.lua/logging/log4j/2.17.0/apache-log4j-2.17.0-bin.zip False positives ((I mean non-compiled, harmless code) ) might be reported on

Problematic code: https://github.com/mergebase/log4j-detector/blob/f8883b49e202ae8d1967f4764e4775255f160187/src/main/java/com/mergebase/log4j/Log4JDetector.java#L376

rgmz commented 2 years ago

Hey @HynekPetrak, can you elaborate on why that code is problematic?

The code was added to address edge cases like #49, and only seems to warn that log4j-core may be present. Perhaps the wording could be clearer:

-- github.com/mergebase/log4j-detector v2021.12.17 (by mergebase.com) analyzing paths (could take a while).
-- Note: specify the '--verbose' flag to have every file examined printed to STDERR.
-- Warning: /private/tmp/apache-log4j-2.17.0-bin.zip!/apache-log4j-2.17.0-bin/log4j-core-2.17.0-sources.jar does not contain Log4J bytecode, but claims it does (!/META-INF/maven/org.apache.logging.log4j/log4j-core/pom.properties)
/private/tmp/apache-log4j-2.17.0-bin.zip!/apache-log4j-2.17.0-bin/log4j-core-2.17.0-sources.jar contains Log4J-2.x   >= 2.17.0 _SAFE_
-- Warning: /private/tmp/apache-log4j-2.17.0-bin.zip!/apache-log4j-2.17.0-bin/log4j-core-2.17.0-tests.jar does not contain Log4J bytecode, but claims it does (!/META-INF/maven/org.apache.logging.log4j/log4j-core/pom.properties)
/private/tmp/apache-log4j-2.17.0-bin.zip!/apache-log4j-2.17.0-bin/log4j-core-2.17.0-tests.jar contains Log4J-2.x   >= 2.17.0 _SAFE_
-- No vulnerable Log4J 2.x samples found in supplied paths: [/tmp/apache-log4j-2.17.0-bin.zip]
-- Congratulations, the supplied paths are not vulnerable to CVE-2021-44228 or CVE-2021-45046 !  :-)

That said, given the severity of CVE-2021-44228 I would rather have a few false positives (instances where log4j-core is present but not exploitable) than false negatives (missing exploitable instances of log4j-core).

HynekPetrak commented 2 years ago

elastic-apm-agent case can be handled even without pom.properties, if you consider alternative class file extension, that is being used by them - they use ".esclazz" instead of ".class"

and perhaps lets take better example of analyzing version 2.14.0 - below.

~/log4j-detector$ java -jar target/log4j-detector-2021.12.17.jar ../war/apache-log4j-2.14.0-bin
-- github.com/mergebase/log4j-detector v2021.12.17 (by mergebase.com) analyzing paths (could take a while).
-- Note: specify the '--verbose' flag to have every file examined printed to STDERR.
-- Warning: /home/hynek/war/apache-log4j-2.14.0-bin/log4j-core-2.14.0-sources.jar does not contain Log4J bytecode, but claims it does (!/META-INF/maven/org.apache.logging.log4j/log4j-core/pom.properties)
/home/hynek/war/apache-log4j-2.14.0-bin/log4j-core-2.14.0-sources.jar contains Log4J-2.x   >= 2.10.0 _VULNERABLE_
-- Warning: /home/hynek/war/apache-log4j-2.14.0-bin/log4j-core-2.14.0-tests.jar does not contain Log4J bytecode, but claims it does (!/META-INF/maven/org.apache.logging.log4j/log4j-core/pom.properties)
/home/hynek/war/apache-log4j-2.14.0-bin/log4j-core-2.14.0-tests.jar contains Log4J-2.x   >= 2.10.0 _VULNERABLE_
/home/hynek/war/apache-log4j-2.14.0-bin/log4j-core-2.14.0.jar contains Log4J-2.x   >= 2.10.0 _VULNERABLE_

If you would use the pom.properties to get the version but not executed this

boolean isLog4j2 = compare("2", version) <= 0;
                                if (isLog4j2) {
                                    log4jProbe = new boolean[]{true, true, true, true, true};
                                    hasJndiLookup = compare("2.0-beta9", version) <= 0;
                                    hasJndiManager = compare("2.1", version) <= 0;
                                    isLog4j2_10 = compare("2.10.0", version) <= 0;
                                    isLog4j2_12_2 = version.startsWith("2.12.") && compare("2.12.2", version) <= 0;
                                    if (isLog4j2_12_2) {
                                        isLog4j2_12_2_override = false;
                                    }
                                    isLog4j2_15 = version.startsWith("2.15.");
                                    isLog4j2_16 = version.startsWith("2.16.");
                                    isLog4j2_17 = compare("2.17.0", version) <= 0;
                                    if (isLog4j2_15 || isLog4j2_16 || isLog4j2_17) {
                                        isLog4j2_15_override = false;
                                    }
                                }

then it rather prints the right output: warning about version without bytecode, but no false positive about vulnerable source.

As I mentioned in the other issue, I ported your code to python (https://github.com/HynekPetrak/log4shell_finder/), but pom.properties handling I did slightly different, which gives such output:

[2021-12-20 02:43:52,881] [INFO] [*] [STRANGE] Package /home/hynek/war/apache-log4j-2.14.0-bin/log4j-core-2.14.0-tests.jar contains pom.properties for Log4J-2.14.0, but classes missing
[2021-12-20 02:43:52,932] [INFO] [+] [VULNERABLE] Package /home/hynek/war/apache-log4j-2.14.0-bin/log4j-core-2.14.0.jar contains Log4J-2.14.0 >= 2.10.0
[2021-12-20 02:43:52,993] [INFO] [*] [STRANGE] Package /home/hynek/war/apache-log4j-2.14.0-bin/log4j-core-2.14.0-sources.jar contains pom.properties for Log4J-2.14.0, but classes missing
juliusmusseau commented 2 years ago

I like that "STRANGE" status for this case. Have to go to bed now, but will think about this more...