Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZOOKEEPER-4469: Suppress OWASP false positives related to Netty TCNative #1817

Closed
wants to merge 2 commits into from

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented Feb 11, 2022

More context here:
https://issues.apache.org/jira/browse/ZOOKEEPER-4469

I am also updating the OWASP dependency check

@eolivelli eolivelli self-assigned this Feb 11, 2022
@symat
Copy link
Contributor

symat commented Feb 11, 2022

Are we absolutely sure we can simply skip these checks for the netty-tcnative library? Isn't this something we use through netty when we do ClientTLS or QuorumTLS?

I see in the pom.xml file that we use a quite recent netty, but a very old netty-tcnative-classes:

    <netty.version>4.1.73.Final</netty.version>
    <netty.tcnative.version>2.0.48.Final</netty.tcnative.version>

(...)

      <dependency>
        <groupId>io.netty</groupId>
        <artifactId>netty-handler</artifactId>
        <version>${netty.version}</version>
        <exclusions>
          <exclusion>
            <groupId>io.netty</groupId>
            <artifactId>netty-tcnative-classes</artifactId>
          </exclusion>
        </exclusions>
      </dependency>
      <dependency>
        <groupId>io.netty</groupId>
        <artifactId>netty-transport-native-epoll</artifactId>
        <version>${netty.version}</version>
      </dependency>
      <dependency>
        <groupId>io.netty</groupId>
        <artifactId>netty-tcnative</artifactId>
        <version>${netty.tcnative.version}</version>
      </dependency>
  

Some of these CVEs are actually quite scary (many affecting only the https admin api interface, but some can affect regular QuorumSSL and ClientSSL interfaces too, AFAICT).

I also don't really understand what the netty-tcnative-classes artifact is. It is not mentioned in the documentation I found about netty-tcnative: https://netty.io/wiki/forked-tomcat-native.html

@symat
Copy link
Contributor

symat commented Feb 11, 2022

I see in the pom.xml file that we use a quite recent netty, but a very old netty-tcnative-classes

never mind, I see 2.0.48.Final is actually the latest netty-tcnative.

In this case I don't understand why these old CVEs appeared now. How can we get e.g. this one: https://nvd.nist.gov/vuln/detail/CVE-2015-2156
This should not be reported for 4.1.73.Final and this has nothing to do with netty-tcnative, AFAICT

Do we have some old netty on our classpath we should exclude?

@symat
Copy link
Contributor

symat commented Feb 11, 2022

I checked the maven dependency tree, and we don't have any old netty on our class path. These CVEs should not have appeared. Maybe OWASP is mixing the netty-tcnative version with the regular netty version?

@nkalmar
Copy link
Contributor

nkalmar commented Feb 11, 2022

Yes, looks like it: https://www.giters.com/jeremylong/DependencyCheck/issues/3867

edit: here's the merged patch, owasp was fixed in 6.5.2: jeremylong/DependencyCheck#3865

@nkalmar
Copy link
Contributor

nkalmar commented Feb 11, 2022

Can we update owasp to latest 6.5.3? Will it cause any issues?

Copy link
Contributor

@nkalmar nkalmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we want to upgrade to latest owasp (6.5.3, we are on 5.3.0, so major version change is required) this is a +1 from me. It is a false positive, see previous comments.

@anmolnar
Copy link
Contributor

@eolivelli @symat OWASP upgrade makes sense to me too.

@symat
Copy link
Contributor

symat commented Feb 11, 2022

yeah... although even the latest OWASP version seems to find false positives:

[ERROR] Failed to execute goal org.owasp:dependency-check-maven:6.5.3:check (default-cli) on project zookeeper:
[ERROR]
[ERROR] One or more dependencies were identified with vulnerabilities that have a CVSS score greater than or equal to '0.0':
[ERROR]
[ERROR] netty-tcnative-2.0.48.Final.jar: CVE-2021-43797, CVE-2019-16869, CVE-2015-2156, CVE-2021-37136, CVE-2014-3488, CVE-2021-37137, CVE-2019-20445, CVE-2019-20444, CVE-2021-21295, CVE-2021-21409, CVE-2021-21290
[ERROR] zookeeper-jute-3.8.0-SNAPSHOT.jar: CVE-2021-29425, CVE-2021-28164, CVE-2021-34429

Here e.g. CVE-2021-43797 should affect only netty prior to 4.1.71.Final and we already have 4.1.73.Final. See: https://nvd.nist.gov/vuln/detail/CVE-2021-43797

Interesting that OWASP 6.5.3 found some additional CVEs. (e.g. CVE-2021-29425, etc) These should be investigated too.

@symat
Copy link
Contributor

symat commented Feb 11, 2022

I think the nicest would be to update to the latest OWASP, then go through the reported CVEs one-by-one to see if they are really false positives.

@symat
Copy link
Contributor

symat commented Feb 11, 2022

I tried again, purging my local CVE database this time before running the new OWASP check with latest OWASP 6.5.3. It still reports the same 11 netty and 3 other CVEs that I listed before.

@nkalmar
Copy link
Contributor

nkalmar commented Feb 11, 2022

That's a bummer, I double checked, this is a closed item in 6.5.2 milestone (one up from last item): https://github.com/jeremylong/DependencyCheck/milestone/38?closed=1

@symat
Copy link
Contributor

symat commented Feb 11, 2022

OK, I double-checked all the CVE errors detected by the latest OWASP 6.5.3. All of these are false positive. Also I checked the maven dependency tree to make sure we don't have any old netty/jetty/commons-io jars on the claspath). I think we are good to go. But I recommend to still update to the latest OWASP version in our project and also suppress these CVEs below. (let's hope OWASP will be fixed later to produce less false positives)

@eolivelli
Copy link
Contributor Author

eolivelli commented Feb 13, 2022

I have updated the OWASP plugin to 6.5.3, but it does not reveal additional CVEs

@symat

@eolivelli
Copy link
Contributor Author

@nkalmar please take another look

@eolivelli
Copy link
Contributor Author

@symat I have run the checker locally several times, cleaning the local cache, but I haven't seen new CVEs reported

Copy link
Contributor

@symat symat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting... I'm also unable now to reproduce the additional jetty and commins-io related false positives for some reason. Anyway, I think your PR is good and the mvn clean package -DskipTests dependency-check:check is successful for me now.

Let's merge this and go ahead with the RC...

@asfgit asfgit closed this in 428e6f9 Feb 14, 2022
asfgit pushed a commit that referenced this pull request Feb 14, 2022
More context here:
https://issues.apache.org/jira/browse/ZOOKEEPER-4469

I am also updating the OWASP dependency check

Author: Enrico Olivelli <eolivelli@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes #1817 from eolivelli/ZOOKEEPER-4469

(cherry picked from commit 428e6f9)
Signed-off-by: Mate Szalay-Beko <symat@apache.org>
asfgit pushed a commit that referenced this pull request Feb 14, 2022
More context here:
https://issues.apache.org/jira/browse/ZOOKEEPER-4469

I am also updating the OWASP dependency check

Author: Enrico Olivelli <eolivelli@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes #1817 from eolivelli/ZOOKEEPER-4469

(cherry picked from commit 428e6f9)
Signed-off-by: Mate Szalay-Beko <symat@apache.org>
asfgit pushed a commit that referenced this pull request Feb 14, 2022
More context here:
https://issues.apache.org/jira/browse/ZOOKEEPER-4469

I am also updating the OWASP dependency check

Author: Enrico Olivelli <eolivelli@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes #1817 from eolivelli/ZOOKEEPER-4469

(cherry picked from commit 428e6f9)
Signed-off-by: Mate Szalay-Beko <symat@apache.org>
asfgit pushed a commit that referenced this pull request Feb 14, 2022
More context here:
https://issues.apache.org/jira/browse/ZOOKEEPER-4469

I am also updating the OWASP dependency check

Author: Enrico Olivelli <eolivelli@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes #1817 from eolivelli/ZOOKEEPER-4469

(cherry picked from commit 428e6f9)
@symat
Copy link
Contributor

symat commented Feb 14, 2022

I merged this to the following branches:

  • master
  • branch-3.8.0
  • branch-3.8
  • branch-3.7
  • branch-3.6

On branch 3.5 I don't see we use netty tcnative, at least we don't have it explicitly added in pom.xml. However, I see some other CVE errors on that branch. We will have to handle branch-3.5 with a separate Jira later (after the 3.8.0 release, when we prepare 3.5.10)

@nkalmar
Copy link
Contributor

nkalmar commented Feb 14, 2022

One of the devs at owasp clarified to me that this was only fixed for netty:netty artifact, and he also opened a new issue to fix it in other artifacts as well. So this is why we still see false positive in netty-tcnative.

anurag-harness pushed a commit to anurag-harness/zookeeper that referenced this pull request Jan 13, 2023
More context here:
https://issues.apache.org/jira/browse/ZOOKEEPER-4469

I am also updating the OWASP dependency check

Author: Enrico Olivelli <eolivelli@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes apache#1817 from eolivelli/ZOOKEEPER-4469
anurag-harness added a commit to anurag-harness/zookeeper that referenced this pull request Jan 13, 2023
…ive (#3)

More context here:
https://issues.apache.org/jira/browse/ZOOKEEPER-4469

I am also updating the OWASP dependency check

Author: Enrico Olivelli <eolivelli@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes apache#1817 from eolivelli/ZOOKEEPER-4469

Co-authored-by: Enrico Olivelli <eolivelli@apache.org>
desaikomal pushed a commit to linkedin/zookeeper that referenced this pull request Jun 17, 2023
More context here:
https://issues.apache.org/jira/browse/ZOOKEEPER-4469

I am also updating the OWASP dependency check

Author: Enrico Olivelli <eolivelli@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes apache#1817 from eolivelli/ZOOKEEPER-4469

(cherry picked from commit 428e6f9)
desaikomal pushed a commit to linkedin/zookeeper that referenced this pull request Jun 27, 2023
More context here:
https://issues.apache.org/jira/browse/ZOOKEEPER-4469

I am also updating the OWASP dependency check

Author: Enrico Olivelli <eolivelli@apache.org>

Reviewers: Norbert Kalmar <nkalmar@apache.org>, Mate Szalay-Beko <symat@apache.org>

Closes apache#1817 from eolivelli/ZOOKEEPER-4469

(cherry picked from commit 428e6f9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants