fix: #8140 AssemblyAnalyzer version resolution issue

[nhumblot]

Description of Change

Witnessed behavior change

Commit fix: #8140 fix version resolution 1fb840ba7d94cc894f5a4fb92dfa8ed2f384adfd

With the AssemblyAnalyzer version resolution commit, the shared dll in #8140 is now resolved as being in version 1.7.0 instead of 1.700.22.46903.

This commit allow to raise CVE-2023-36414

Commit fix: #8140 hint azure_identity_library_for_.net 9f58d2f2c4fcde3c8e3df36837efcee10f972234

This additional hint make DependencyCheck report CVE-2024-29992 on the shared dll in #8140

Business logic

This PR proposes to fix the AssemblyAnalyzer version resolution at priority levels 2, 3 & 4.

Priority level 1 resolution remains unchanged. In case ProductVersion and FileVersion both match on the first 3 parts, the common pattern between both versions is used as the dependency version.

Priority level 2 adds a stronger filtering mechanism to prevent picking one of the ProductVersion or FileVersion as the dependency version in case the less specified version contradicts the longer one.

As an example:
parsed ProductVersion = 1.2 & parsed FileVersion = 1.2.13.0 will resolves the dependency version to 1.2.13.0 as 1.2 is included in the longer one - not a change with the current implementation.
parsed ProductVersion = 1.2.13.0 & parsed FileVersion = 1.2 will resolves the dependency version to 1.2.13.0 as 1.2 is included in the longer one - not a change with the current implementation.
parsed ProductVersion = 1.7.0 & parsed FileVersion = 1.700.22.46903 will not resolve the dependency version as there is a contradiction at minor version level between 1.7 & 1.700 - change with the current implementation which set the longer version.

Priority level 3 will pick the ProductVersion if exists and can be parsed - change with the current implementation which would pick the parsed FileVersion. This aligns with our current evidence confidence where ProductVersion has a higher confidence than FileVersion.

Priority level 4 will pick parsed FileVersion if no ProductVersion could be parsed - change with the current implementation which would pick the parsed ProductVersion at this priority level.

Finally, if nothing before could help us set the dependency version, we do not set it and leave the evidences, which do not get filtered by the VersionFilterAnalyzer as now dependency version is set. - not a change with the current implementation

Related issues

• fixes TPL is not reported as vulnerable #8140

Have test cases been added to cover the new functionality?

yes, the added test covers the AssemblyAnalyzer dependency version resolution, with its input strictly being exactly what was provided into the method during real execution in debug mode.

Test snippet used to execute the CLI in e2e

In AppTest, with my-secrets.properties containing my NVD API Key and OssIndex credentials:

    @Test
    void should_app_reports_tpl_as_vulnerable() {
        // Given
        String[] args =
                new String[]{"--format", "HTML", "--format", "JSON", "--out", "ReportFileName",
                                "--scan", "/my/absolute/path/ScanFolder",
                                "--propertyfile", "my-secrets.properties", "--log", "cli.log"};

        // When
        App.main(args);

        // Then
    }

Additional notes identified during the investigation

Thanks to @chadwilson for a first investigation which already located the root of the issue

I identified additional room of improvements/analysis into the product, and would like to share to see if we agree to pursue some additional improvements.

AssenblyAnalyzer dependency version resolution refactoring

The code holding the logic to decide to set a version could be refactored to improve clarify and reveal the business logic. I did not do it in this change on purpose as I want to emphasis the behavior change in this PR. If the approach of the change is approved, I am planning to come with an additional refactoring PR, looking for an easier to read logic.

VersionFilterAnalyzer responsibility

As stated in this comment the VersionFilterAnalyzer removes version evidences if a version is set, unless evidence is a hint.

The JavaDoc shows the intention behind the version analyzer:

This analyzer attempts to filter out erroneous version numbers collected. Initially, this will focus on JAR files that contain a POM version number that matches the file name - if identified all other version information will be removed.

It is actually covering much more than that as it will inspect all dependencies. Is this JavaDoc still showing the true intention of this analyzer, in such case we have a bug, or is the doc obsolete and to be updated?

I believe we could consider narrowing the scope of the VersionFilterAnalyzer to be specifically dedicated to jar files, as intended by the JavaDoc. This would increase the number of false positive, but reduce the risk of false negatives (and I believe with the purpose of DependencyCheck, it is better to have false positives than false negatives). Please note this was first considered as the fix in this PR, which has the same effect as the dependency version resolution, but I did not pick this route in fear of raising the amount of false positive while the assembly dependency version resolution logic was already looking broken, it may still improve some other scenarios

CPEAnalyzer integration issue with VersionFilterAnalyzer and owner of the decision to rely on the dependency version and evidences

In the VersionFilterAnalyzer, if a dependency version is set, then we remove all version evidences not coming from an hint.

Later in the analysis, we enter the CPEAnalyzer which has a method considerDependencyVersion(Dependency dependency, String vendor, String product, Confidence confidence, final Set<IdentifierMatch> collected) which decides to rely on version evidences in some cases even if a dependency version is set, while all the evidences got removed by the VersionFilterAnalyzer. It looks like we have 2 pieces of code at 2 different locations trying to have the same responsibility and they don't interact very well between each other 🤔

CPEAnalyzer decision to rely on the dependency version or evidences seems to have a case sensitivity bug

In CPEAnalyzer, the method considerDependencyVersion(Dependency dependency, String vendor, String product, Confidence confidence, final Set<IdentifierMatch> collected) has the following code snippet:

for (String word : product.split("[^a-zA-Z0-9]")) {
                    useDependencyVersion &= name.contains(word) || stopWords.contains(word)
                            || wordMatchesEcosystem(dependency.getEcosystem(), word);
                }

While debugging in this snippet, for this specific case, a "Azure".contains("azure") ends up being made, which evaluates useDependencyVersion to false, which makes the CPEAnalyzer decides to not base itself on the dependency version but the version evidences (who were wiped out by the VersionFilterAnalyzer). I did not pursue a fix in this area as in this specific case, the analyzer still decides to rely on evidences for another reason afterwards. It might be interesting to consider an case insensitive comparison.

[nhumblot]

note: See the mention of Priority 2 in the behavior change of the PR description for additional information.

[nhumblot]

note: See the mention of Priority 3 in the behavior change of the PR description for additional information.

[jeremylong]

lgtm

[jeremylong]

Great analysis! thank you. I completely agree that we should limit the VersionFilterAnalyzer to just Java/JAR files - this is the only place we really get a vast amount of evidence that needed to be filtered.