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

fix security issues due to transitive dependency icu4j #790

Closed
wants to merge 4 commits into from

Conversation

anshbansal
Copy link

@anshbansal anshbansal commented May 5, 2022

These dependencies are brought in when using latest release of rest.li

\--- org.antlr:antlr4:4.7.2
     +--- org.antlr:antlr4-runtime:4.7.2
     +--- org.antlr:antlr-runtime:3.5.2
     +--- org.antlr:ST4:4.1
     |    \--- org.antlr:antlr-runtime:3.5.2
     +--- org.abego.treelayout:org.abego.treelayout.core:1.0.3
     +--- org.glassfish:javax.json:1.0.4
     \--- com.ibm.icu:icu4j:61.1

OWASP scanner on a sample project shows these vulnerabilities

image

Upgrading to latest 4.10.1 we get these

\--- org.antlr:antlr4:4.10.1
     +--- org.antlr:antlr4-runtime:4.10.1
     +--- org.antlr:antlr-runtime:3.5.3
     +--- org.antlr:ST4:4.3.3
     |    \--- org.antlr:antlr-runtime:3.5.2 -> 3.5.3
     +--- org.abego.treelayout:org.abego.treelayout.core:1.0.3
     +--- org.glassfish:javax.json:1.0.4
     \--- com.ibm.icu:icu4j:69.1

After update 0 CVEs

image

Ref

@anshbansal
Copy link
Author

@nickibi Can I please get a review so the workflows can be run? I have added in the details of the CVEs found by OWASP scanner, a before and after.

Similar to #788

@anshbansal
Copy link
Author

antlr 4.10.1 was failing with Java 8 so trying a smaller version. No CVEs in this version either.

@anshbansal
Copy link
Author

Earlier change was failing ./gradlew :data:compileJava. Tested that for this version ./gradlew :data:compileJava completes and there are no CVEs

@anshbansal
Copy link
Author

@nickibi Can I please get a review so the workflows can be run?

@shirshanka
Copy link

@mchen07 : could we get someone to approve the CI workflow?

@cgtz
Copy link
Contributor

cgtz commented May 17, 2022

@shirshanka approved it to run

@shirshanka
Copy link

shirshanka commented May 17, 2022

Thanks @cgtz , looks like it failed because there is an incorrect usage of @NotNull in the current code. Maybe your team can fix it using a patch like this?

diff --git a/d2/src/main/java/com/linkedin/d2/discovery/stores/zk/acl/AclAwareZookeeper.java b/d2/src/main/java/com/linkedin/d2/discovery/stores/zk/acl/AclAwareZookeeper.java
index 7c06bc716..646e33271 100644
--- a/d2/src/main/java/com/linkedin/d2/discovery/stores/zk/acl/AclAwareZookeeper.java
+++ b/d2/src/main/java/com/linkedin/d2/discovery/stores/zk/acl/AclAwareZookeeper.java
@@ -20,7 +20,6 @@ import com.linkedin.d2.discovery.stores.zk.AbstractZooKeeper;
 import com.linkedin.d2.discovery.stores.zk.ZKPersistentConnection;
 import com.linkedin.d2.discovery.stores.zk.ZooKeeper;
 import java.util.List;
-import org.antlr.v4.runtime.misc.NotNull;
 import org.apache.zookeeper.AsyncCallback;
 import org.apache.zookeeper.CreateMode;
 import org.apache.zookeeper.KeeperException;
@@ -30,6 +29,8 @@ import org.apache.zookeeper.data.ACL;
 import org.apache.zookeeper.data.Stat;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import javax.annotation.Nonnull;
+


 /**
@@ -44,7 +45,7 @@ public class AclAwareZookeeper extends AbstractZooKeeper

   private final ZKAclProvider _aclProvider;

-  public AclAwareZookeeper(@NotNull ZooKeeper zooKeeper, @NotNull ZKAclProvider aclProvider)
+  public AclAwareZookeeper(@Nonnull ZooKeeper zooKeeper, @Nonnull ZKAclProvider aclProvider)
   {
     super(zooKeeper);
     _aclProvider = aclProvider;

@anshbansal
Copy link
Author

@mchen07 / @cgtz I made the change. Can someone please approve this workflow?

@shirshanka
Copy link

bump on this request since this seems to be low risk to take in @mchen07 @cgtz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants