jeremylong / DependencyCheck

OWASP dependency-check is a software composition analysis utility that detects publicly disclosed vulnerabilities in application dependencies.
https://owasp.org/www-project-dependency-check/
Apache License 2.0
6.29k stars 1.26k forks source link

Incorrect Version Identification of pdf.js #6821

Closed hawkchen closed 1 month ago

hawkchen commented 1 month ago

Describe the bug The dependency-check Maven plugin incorrectly identifies the version of pdf.js. It misinterprets a comment in index.src.js containing the string "pdfjs-dist@3.2.146" as the actual version of the pdf.js package.

Version of dependency-check used The problem occurs using version 10.0.2 of the dependency-check Maven plugin.

Log file report

To Reproduce Steps to reproduce the behavior:

  1. Add the dependency-check Maven plugin with the following configuration:
    <plugin>
        <groupId>org.owasp</groupId>
        <artifactId>dependency-check-maven</artifactId>
        <version>10.0.2</version>
        <executions>
            <execution>
                <goals>
                    <goal>check</goal>
                </goals>
            </execution>
        </executions>
    </plugin>
  2. Run the plugin.
  3. Observe that it incorrectly identifies the pdf.js version in zkex-10.0.1.jar as pdf.js@3.2.146.

Expected behavior The plugin should correctly identify the version of pdf.js and not misinterpret comments within the code.

Additional context The issue arises because index.src.js contains pdfjs-dist@3.2.146 in the comment:

if (!textLayer) {
    // `textLayer` could be non-existent as described in ZK-4754.
    // As of pdfjs-dist@3.2.146, `PDFJS.renderTextLayer` (see the end of this function) cannot
    // accept undefined or null for the `container` field. This is not just the type definitions
    // in pdfjs-dist@3.2.146 being too strict. The actual implementation in `ext/pdf.js` operates
    // under the assumption of a non-null `container`.
    return Promise.resolve(); // Empty promise
}

I verified the problem by rewriting the string "pdfjs-dist@3.2.146" to "3.2.146", and it doesn't report the wrong version.

chadlwilson commented 1 month ago

This happens due to this very simplistic (and highly non-performant!) logic within the h3xstream RetireJS scanner which this project relies upon, not because of ODC itself:

https://github.com/h3xstream/burp-retire-js/blob/bd06f7d9f6802b02c693947f67814eb711ac3378/retirejs-core/src/main/java/com/h3xstream/retirejs/repo/VulnerabilitiesRepository.java#L103-L143

The list of file patterns to evaluate is maintained by RetireJS upstream here and this is their defined methodology; which is not really for ODC or h3xstream to override.

https://github.com/RetireJS/retire.js/blob/5baff2dc4905a2c00c34ef747eb6812c2dfe6aa5/repository/jsrepository.json#L7649-L7655

As you can see, the first pattern is the one creating the false positive here: " pdfjs-dist@(§§version§§) "

To fix this would need either

If ODC users more widely disagree with the methodology of RetireJS matching file content regexes, the workaround is to disable the RetireJS scanner and reply on npm audit/yarn audit/NVD data only.

eoftedal commented 1 month ago

You can use surpressions to avoid this false positive until it is fixed. Retire.js is not an alternative to npm audit. Npm has a better database for anything that can be detected through package.json or similar. The goal for retire.js is to find places where developers have included javascript libraries by just bundling or copy the library file itself without any proper process for handling versions. Npm audit and retire.js serve different purposes.

I agree this regex was a bit simplistic and should have included more context around. I cannot fix it the next days though, so surpress or submit a fix PR if you need a fix faster.

chadlwilson commented 1 month ago

I wasn't trying to imply they are equivalent - if they were there'd be no need for ODC to support :-) I took it was a given that OP would know they could suppress the individual issue.

It's also an unfortunately reality that sometimes suppressions are not possible/maintainable in cases where ODC doesn't know co-ordinates for the jar, as you get into sha or file-path based suppressions which can be pretty brittle.

Since this one seems to relate to an unreleased version of ZK, possibly unlikely that other users will see the specific issue here, but I tend to find people are surprised that the RetireJS analyzer is on by default (with the ODC Gradle plugin at least) and thus aren't familiar with the suite of analyzers within ODC and where the data sources are, especially if some jar they depend on happens to include some JS for functionality they don't use.

Anyway, more trying to help people understand where the logic sits, as otherwise downstream projects like ODC get inundated with issues that need to be addressed upstream (NVD, RetireJS, OSSIndex etc).

eoftedal commented 1 month ago

This should be fixed now with @chadlwilson ‘s PR to the retire.js repo