snyk / snyk-gradle-plugin

Basic Snyk CLI plugin for Gradle support
Other
25 stars 19 forks source link

Snyk reports some upgradable non-direct dependencies as non-upgradable #174

Closed artemptushkin closed 3 years ago

artemptushkin commented 3 years ago

Problem

I noticed that Gradle reports non-direct (transitive) dependencies as fixable https://github.com/snyk/snyk-gradle-plugin/issues/173 but assume it is the policy there is still is an ambiguous behaviour of the tool.

In some cases, it reports transitive dependencies in section Issues to fix by upgrading where there is an upgrade available but at the same time for some transitives it doesn't.

Outcomes

It affects that snyk test --fail-on=all returns 0 (ok) in cases when there are still fixable issues exist. When the documentation of that parameters says:

Only fail when there are vulnerabilities that can be fixed.

It affects the portal shows a dependency as non-fixable when it is "Fixed in"

image

Transitive dependency report

Please find the example here: https://github.com/artemptushkin/snyk-gradle-plugin/tree/issue/transitives/test/fixtures/transitives

Gradle file:

dependencies {
    implementation 'org.mock-server:mockserver-netty:5.11.2'
}

$ snyk test output:

...
Issues to fix by upgrading:
  Upgrade io.netty:netty-handler@4.1.53.Final to io.netty:netty-handler@4.1.59.Final to fix
  ✗ Information Disclosure [Medium Severity][https://snyk.io/vuln/SNYK-JAVA-IONETTY-1082235] in io.netty:netty-handler@4.1.53.Final
    introduced by io.netty:netty-handler@4.1.53.Final
...

Issues with no direct upgrade or patch:
  No upgrade or patch available
  ✗ Arbitrary Code Execution [High Severity][https://snyk.io/vuln/SNYK-JAVA-ORGAPACHEVELOCITY-1083992] in org.apache.velocity:velocity-engine-core@2.2
    introduced by org.mock-server:mockserver-core@5.11.2 > org.apache.velocity:velocity-engine-core@2.2 and 1 other path(s)
  This issue was fixed in versions: 2.3
...

It reports ambiguous text:

Issues with no direct upgrade or patch... This issue was fixed in versions: 2.3

In other words:

io.netty:netty-handler:
   transitive: true
   fixable: true
   upgradable:
     actual: true
     expected: true
org.apache.velocity:velocity-engine-core:
   transitive: true
   fixable: true
   upgradable:
     actual: false
     expected: true

Transitives dependencies as direct

At the same time, when we declare it explicitly, i.e. is direct these are reported as updatable as expected Please find the example here: https://github.com/artemptushkin/snyk-gradle-plugin/tree/issue/transitives/test/fixtures/transitive-as-direct

Gradle file:

dependencies {
    implementation 'org.mock-server:mockserver-netty:5.11.2'

    implementation "org.apache.velocity:velocity-engine-core:2.2"
    implementation "io.netty:netty-codec-http:4.1.53.Final"
}

Output:

...
Issues to fix by upgrading:
...
  Upgrade io.netty:netty-codec-http@4.1.53.Final to io.netty:netty-codec-http@4.1.59.Final to fix
  ✗ Information Disclosure [Medium Severity][https://snyk.io/vuln/SNYK-JAVA-IONETTY-1070799] in io.netty:netty-codec-http@4.1.53.Final
    introduced by io.netty:netty-codec-http@4.1.53.Final and 3 other path(s)
...
  Upgrade org.apache.velocity:velocity-engine-core@2.2 to org.apache.velocity:velocity-engine-core@2.3 to fix
  ✗ Arbitrary Code Execution (new) [High Severity][https://snyk.io/vuln/SNYK-JAVA-ORGAPACHEVELOCITY-1083992] in org.apache.velocity:velocity-engine-core@2.2
    introduced by org.apache.velocity:velocity-engine-core@2.2 and 2 other path(s)
....

In my understanding, assuming you include into the report transitive dependencies, i.e. https://github.com/snyk/snyk-gradle-plugin/issues/173 - is not the problem, the report sectionIssues to fix by upgrading should include all the updatable transitive dependencies but it doesn't.

gitphill commented 3 years ago

Hi @artemptushkin I've tried to re-create using https://github.com/artemptushkin/snyk-gradle-plugin/tree/issue/transitives/test/fixtures/transitives (thanks for providing this). I get Tested 79 dependencies for known issues, found 10 issues, 305 vulnerable paths. with all 10 issues reported under Issues with no direct upgrade or patch. Which I think is correct, https://mvnrepository.com/artifact/org.mock-server/mockserver-netty shows 5.11.2 is the latest version of this dependency, so there is no version Snyk can move that dependency to to fix any of these issues.

gitphill commented 3 years ago

I can see the confusion though, it looks like this wasn't always the case. There's a version of the Snyk CLI after 1.388.0 where we do report transitives as 'upgradable' which is wrong. I'm looking at when that was introduced and why. However it looks correct in the latest version of the Snyk CLI, I was using 1.536.0

artemptushkin commented 3 years ago

@gitphill thanks! I used several version but looks like not the latest, or brew provides not the latest several weeks ago :)

It works fine with 1.532.0 (standalone) as the latest for brew for now

So the official policy that Snyk doesn't enforce to update transitives/non-direct dependencies, as I understood. I haven't found anything related to this in your doc / FAQ, shouldn't we update it?

gitphill commented 3 years ago

https://support.snyk.io/hc/en-us/articles/360006113798-Remediate-your-vulnerabilities is our help page that talks about remediation. It says Snyk helps you to fix vulnerabilities in two ways. Either by upgrading the direct dependencies to a vulnerability free version or by patching the vulnerability. the word direct is maybe a little subtle, but it's important to the issue you've reported here. Also worth noting Snyk only supports patching in npm at the moment, so the only way we remediate gradle at the moment is through direct dependencies.

I tracked this down to a change we made around cyclic dependencies, where we incorrectly hoist transitives to the top level which then has this knock on effect to how we report vulnerabilities in the CLI. This has been fixed recently and it looks like we're generating better dependency graphs for gradle in the latest CLI.

Sorry for the confusion, hopefully this helps - can I close this issue?

artemptushkin commented 3 years ago

Yes, thanks and #173 can be closed according to this thread. I appreciate the explanation.