Updating dependencies (guava and what brought in older guava) to get rid of the guava-related CVE-2018-10237 and CVE-2020-8908 #13716

dlg99 · 2022-01-11T23:29:15Z

Updating dependencies (guava and what brought in older guava) to get rid of the guava-related CVE-2018-10237 and CVE-2020-8908

pulsar-io/canal is excluded (no fix available and guava is bundled into
fatjar), tracked at alibaba/canal#4010

Motivation

mvn clean install verify -Powasp-dependency-check -DskipTests found various CVEs

Modifications

Upgraded guava, dependencies that were forcing (ir bundling in a fatjar) older guava.

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is already covered by existing tests

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): YES
The public API: (yes / no)
The schema: (yes / no / don't know)
The default values of configurations: (yes / no)
The wire protocol: (yes / no)
The rest endpoints: (yes / no)
The admin cli options: (yes / no)
Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

doc-required

(If you need help on updating docs, create a doc issue)
no-need-doc

(Please explain why)
doc

(If this PR contains doc changes)

lhotari · 2022-01-17T09:22:12Z

src/owasp-dependency-check-suppressions.xml

+  <suppress>
+     <notes><![CDATA[
+       file name: canal.client-1.1.4.jar (shaded: com.google.guava:guava:18.0)
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/com\.google\.guava/guava@.*$</packageUrl>
+     <cpe>cpe:/a:google:guava</cpe>
+  </suppress>
+  <suppress>
+     <notes><![CDATA[
+     file name: canal.client-1.1.4.jar (shaded: com.google.guava:guava:18.0)
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/com\.google\.guava/guava@.*$</packageUrl>
+     <vulnerabilityName>CVE-2018-10237</vulnerabilityName>
+  </suppress>
+  <suppress>
+     <notes><![CDATA[
+     file name: canal.client-1.1.4.jar (shaded: com.google.guava:guava:18.0)
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/com\.google\.guava/guava@.*$</packageUrl>
+     <vulnerabilityName>CVE-2020-8908</vulnerabilityName>
+  </suppress>
+  <suppress>
+     <notes><![CDATA[
+     file name: logback-core-1.1.3.jar
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/ch\.qos\.logback/logback\-core@.*$</packageUrl>
+     <cpe>cpe:/a:qos:logback</cpe>
+  </suppress>
+  <suppress>
+     <notes><![CDATA[
+     file name: rocketmq-acl-4.5.2.jar
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/org\.apache\.rocketmq/rocketmq\-acl@.*$</packageUrl>
+     <cpe>cpe:/a:apache:rocketmq</cpe>
+  </suppress>
+  <suppress>
+     <notes><![CDATA[
+     file name: spring-core-3.2.18.RELEASE.jar
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/org\.springframework/spring\-core@.*$</packageUrl>
+     <cpe>cpe:/a:pivotal_software:spring_framework</cpe>
+   </suppress>
+  <suppress>
+     <notes><![CDATA[
+     file name: spring-core-3.2.18.RELEASE.jar
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/org\.springframework/spring\-core@.*$</packageUrl>
+     <cpe>cpe:/a:springsource:spring_framework</cpe>
+  </suppress>
+  <suppress>
+     <notes><![CDATA[
+     file name: spring-core-3.2.18.RELEASE.jar
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/org\.springframework/spring\-core@.*$</packageUrl>
+     <cpe>cpe:/a:vmware:spring_framework</cpe>
+  </suppress>
+  <suppress>
+     <notes><![CDATA[
+     file name: spring-core-3.2.18.RELEASE.jar
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/org\.springframework/spring\-core@.*$</packageUrl>
+     <cpe>cpe:/a:vmware:springsource_spring_framework</cpe>
+  </suppress>
+  <suppress>
+     <notes><![CDATA[
+     file name: logback-classic-1.1.3.jar
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/ch\.qos\.logback/logback\-classic@.*$</packageUrl>
+     <cpe>cpe:/a:qos:logback</cpe>
+  </suppress>
+  <suppress>
+     <notes><![CDATA[
+     file name: logback-core-1.1.3.jar
+     ]]></notes>
+     <packageUrl regex="true">^pkg:maven/ch\.qos\.logback/logback\-core@.*$</packageUrl>
+     <vulnerabilityName>CVE-2017-5929</vulnerabilityName>
+   </suppress>


Please revisit the suppression rules since a lot of the rules would also match any future CVEs.

@lhotari Ended up upgrading canal and spring; excluded lgback, guava is shaded with canal but I set packageUrl to match specific guava version.
I don't think I can do anything else with the matcher there. We can consider using something like suppress until="2023-01-01Z" to force review of the suppressions but this can go as a separate PR if we agree on this approach.

If that's the case, I wonder if it would make sense to add a owasp dependency check suppression file just for pulsar-io connectors?
That would also make PIP-62 transition easier.

After a quick look, suppressiosn shoudl be defiend on the top level jeremylong/DependencyCheck#2152 (comment)
and the dependency check was limited ocnsiderations for multi-module builds jeremylong/DependencyCheck#1628
AFAICT, there is a way to disable scan on submodules completely but I'd prefer to not go that route for now.
I would not worry much about PIP-62; it's been about a year without progress, IMO we'll worry about i when we get there.

@dlg99 you should be able to suppress a specific jar file with the sha1 checksum, https://jeremylong.github.io/DependencyCheck/general/suppression.html
Does that help?

@lhotari updated with sha1

Thanks, that looks better with the sha1 information for targeting the library. Just wondering if it would make sense to target specific CVEs for each dependency that gets matched by sha1. The reason for this is that it's possible that some new CVEs show up in the future. Everyone tends to copy the way the rules are written for new rules, so there's a long standing consequence in the way the rules are defined in this case.
If there are multiple CVEs for a single sha1/dep, just adding sufficient amount of suppression rules where the cve element varies?

@lhotari correct. This way we are suppressing specific CVE for specific sha1.
Also it is either a sha1 or a regex, no mixing.

…rid of the guava-related CVE-2018-10237 and CVE-2020-8908 pulsar-io/canal is excluded (no fix available and guava is bundled into fatjar), tracked at alibaba/canal#4010

…k#1118

…anal uses to get rid of multiple CVEs

dlg99 · 2022-01-19T01:40:52Z

/pulsarbot run-failure-checks

dlg99 · 2022-01-19T19:11:23Z

/pulsarbot run-failure-checks

lhotari · 2022-01-20T03:55:17Z

pulsar-io/flume/pom.xml

@@ -106,7 +125,7 @@
        <dependency>
            <groupId>com.google.guava</groupId>
            <artifactId>guava</artifactId>
-            <version>18.0</version>
+            <version>${guava.version}</version>


Does the current Flume version support latest Guava version? There are multiple breaking changes between 18.0 and 31.0 .

The tests passed, that's as much as I can tell.

I believe that there must have been a reason why the version was pinned to 18.0 . I doubt that the tests in pulsar-io/flume validate the full functionality. @tuteng could you shade some light into this since you are the original author?

@lhotari the best testing I could do is:

get flume's code.

checkout d4fcab4f501d41597bc616921329a4339f73585e (last release), build. Build fails, with jdk 11 and jdk 8.

checkout trunk (some dependencies upgraded after the release but no dramatic changes). build succeeds.

the guava dependency is brought in by org.apache.flume:flume-ng-node - so cd flume/flume-ng-node and run tests there. tests pass (with jdk8, some failed with jdk11)

force <version>31.0.1-jre</version> for guava in flume-ng-node. Build succeeds. mvn dependency:tree confirms that updated guava is used. The tests pass.

great work in confirming the compatibility @dlg99

lhotari · 2022-01-20T04:00:08Z

pom.xml

-      <dependency>
-        <groupId>org.eclipse.jetty</groupId>
-        <artifactId>jetty-util</artifactId>
+        <artifactId>jetty-bom</artifactId>


Nice change! Please create a separate PR for the jetty-bom change and the jetty.version upgrade. This change isn't related to Guava and would be valuable on it's own.

@lhotari IIRC this came in after I upgraded other dependencies that used older guava (confluent?), to settle on the newer version. I'd prefer to avoid spending time on ripping the PR apart/rebuilding/rescanning, testing what else needs to be downgraded etc.

to settle on the newer version. I'd prefer to avoid spending time on ripping the PR apart/rebuilding/rescanning, testing what else needs to be downgraded etc.

You can keep the change in this PR. If you submit it separately, the PR could be merged before this PR. If this PR gets later on reverted for some reason, let's say that it breaks something, we can continue to keep the Jetty change. The Jetty change might also be cherry-picked to maintenance branches, but the Guava change might not be picked. There are multiple reasons to do minimal PRs. I know that it's terrible now that our CI is in such bad shape with a lot of flaky tests and long build times. We just have to keep on improving.
@eolivelli What's your opinion about splitting the jetty-bom change to a new PR?

IIUC importing the BOM of Jetty solves some problems with the version of Guava imported by Jetty.

While I agree with @lhotari's concern about tracking broken patches and reverting, I also understand @dlg99's pain.

Considering that this patch is only about updating dependencies that are related one to each other I am +1 with merging this patch as it is.

how is the Jetty upgrade related to Guava upgrade?

found the relation to Jetty, np.

dlg99 · 2022-01-20T04:27:02Z

/pulsarbot run-failure-checks

eolivelli

LGTM

eolivelli · 2022-01-20T13:20:38Z

pom.xml

-      <dependency>
-        <groupId>org.eclipse.jetty</groupId>
-        <artifactId>jetty-util</artifactId>
+        <artifactId>jetty-bom</artifactId>


IIUC importing the BOM of Jetty solves some problems with the version of Guava imported by Jetty.

While I agree with @lhotari's concern about tracking broken patches and reverting, I also understand @dlg99's pain.

Considering that this patch is only about updating dependencies that are related one to each other I am +1 with merging this patch as it is.

lhotari

I'm fine with merging. We can always fix issues that show up.

codelipenghui · 2022-01-21T06:23:44Z

pulsar-io/canal/pom.xml

+    <properties>
+        <spring.version>5.0.20.RELEASE</spring.version>
+        <canal.version>1.1.5</canal.version>
+    </properties>


It's better to move the dependency management to the root pom

codelipenghui · 2022-01-21T06:24:05Z

pulsar-io/flume/pom.xml

+    <properties>
+        <avro.version>1.8.2</avro.version>
+    </properties>


It's better to move the dependency management to the root pom

eolivelli · 2022-01-21T09:43:37Z

@codelipenghui I am sorry, I misread your comments.
I have merged the patch.

@dlg99 would you mind following up according to @codelipenghui 's comments ?

dlg99 · 2022-01-21T16:06:53Z

@codelipenghui @eolivelli There are quite a few connectors that have versions of their dependencies set in the connector's pom. I always assumed this is on purpose. SOme of them may override what's set in the root pom's. I'll finish up with other changes and do a check if any versions can be moved to the root pom safely.

…rid of the guava-related CVE-2018-10237 and CVE-2020-8908 (apache#13716) * Updating dependencies (guava and what brought in older guava) to get rid of the guava-related CVE-2018-10237 and CVE-2020-8908 * testng 7.5 isn't compatible with current powermock powermock/powermock#1118 * Upgraded canal to 1.1.5, excluded logback, and upgraded spring that canal uses to get rid of multiple CVEs (cherry picked from commit 8083333)

…rid of the guava-related CVE-2018-10237 and CVE-2020-8908 (apache#13716) * Updating dependencies (guava and what brought in older guava) to get rid of the guava-related CVE-2018-10237 and CVE-2020-8908 * testng 7.5 isn't compatible with current powermock powermock/powermock#1118 * Upgraded canal to 1.1.5, excluded logback, and upgraded spring that canal uses to get rid of multiple CVEs (cherry picked from commit 8083333) (cherry picked from commit 48311ed)

github-actions bot assigned dlg99 Jan 11, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 11, 2022

shoothzj approved these changes Jan 12, 2022

View reviewed changes

nicoloboschi approved these changes Jan 12, 2022

View reviewed changes

codelipenghui added this to the 2.10.0 milestone Jan 12, 2022

dlg99 force-pushed the cve-1 branch from e312315 to 177e733 Compare January 13, 2022 00:11

shoothzj approved these changes Jan 13, 2022

View reviewed changes

lhotari requested changes Jan 17, 2022

View reviewed changes

dlg99 added 10 commits January 18, 2022 13:31

Updating dependencies (guava and what brought in older guava) to get …

7a7a850

…rid of the guava-related CVE-2018-10237 and CVE-2020-8908 pulsar-io/canal is excluded (no fix available and guava is bundled into fatjar), tracked at alibaba/canal#4010

license check

d6510ee

testng 7.5 isn't compatible with current powermock powermock/powermoc…

06fa8eb

…k#1118

more canal-related suppressions

efa603d

more canal-related suppressions

0411e33

Make sure presto does not mix in older guava

fe9e10f

Presto: correct guava version

a3d27c4

revert testng to 7.3 because of test failures on CI

43014ac

downgrade avro for flume

02b36a8

Upgraded canal to 1.1.5, excluded logback, and upgraded spring that c…

2128a26

…anal uses to get rid of multiple CVEs

dlg99 force-pushed the cve-1 branch from c546a29 to 2128a26 Compare January 18, 2022 21:56

specified sha1 for suppressions intead of the regexes

ed4dde9

lhotari reviewed Jan 20, 2022

View reviewed changes

lhotari requested a review from tuteng January 20, 2022 13:12

eolivelli approved these changes Jan 20, 2022

View reviewed changes

lhotari approved these changes Jan 20, 2022

View reviewed changes

lhotari requested review from Jason918 and shoothzj January 20, 2022 14:51

tuteng approved these changes Jan 20, 2022

View reviewed changes

codelipenghui reviewed Jan 21, 2022

View reviewed changes

eolivelli merged commit 8083333 into apache:master Jan 21, 2022

This was referenced Dec 7, 2022

#7628 - change to jetty bom in root pom #7665

Closed

Use jersey bom in root pom #7658

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating dependencies (guava and what brought in older guava) to get rid of the guava-related CVE-2018-10237 and CVE-2020-8908 #13716

Updating dependencies (guava and what brought in older guava) to get rid of the guava-related CVE-2018-10237 and CVE-2020-8908 #13716

dlg99 commented Jan 11, 2022

lhotari Jan 17, 2022

dlg99 Jan 18, 2022

lhotari Jan 19, 2022

dlg99 Jan 19, 2022

lhotari Jan 19, 2022

dlg99 Jan 19, 2022

lhotari Jan 20, 2022

dlg99 Jan 20, 2022

dlg99 commented Jan 19, 2022

dlg99 commented Jan 19, 2022

lhotari Jan 20, 2022

dlg99 Jan 20, 2022

lhotari Jan 20, 2022

dlg99 Jan 20, 2022

lhotari Jan 20, 2022

lhotari Jan 20, 2022

dlg99 Jan 20, 2022

lhotari Jan 20, 2022

eolivelli Jan 20, 2022

lhotari Jan 20, 2022

lhotari Jan 20, 2022

dlg99 commented Jan 20, 2022

eolivelli left a comment

eolivelli Jan 20, 2022

lhotari left a comment

codelipenghui Jan 21, 2022

codelipenghui Jan 21, 2022

eolivelli commented Jan 21, 2022

dlg99 commented Jan 21, 2022

Updating dependencies (guava and what brought in older guava) to get rid of the guava-related CVE-2018-10237 and CVE-2020-8908 #13716

Updating dependencies (guava and what brought in older guava) to get rid of the guava-related CVE-2018-10237 and CVE-2020-8908 #13716

Conversation

dlg99 commented Jan 11, 2022

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlg99 commented Jan 19, 2022

dlg99 commented Jan 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlg99 commented Jan 20, 2022

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhotari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli commented Jan 21, 2022

dlg99 commented Jan 21, 2022