Do not depend on commons-collections4 milestone (use 4.4 instead)

[reschke]

Describe the bug

esapi uses a milestone release of commons-collections4. These are for testing only. Using them forces downstream projects to depend on it as well.

Specify what ESAPI version(s) you are experiencing this bug in

2.6.0.0

[xeno6696]

Using the previous non-milestone release would make esapi (and thus your applications) vulnerable to:

Vulnerabilities from dependencies:
CVE-2020-15250

See: https://mvnrepository.com/artifact/org.apache.commons/commons-collections4/4.4

[reschke]

That's a test dependency for Junit.

[kwwall]

@reschke - Be that as it may, many (most?) SCA tools still flag JUnit (transitive) dependencies even when the scope is marked as 'test'. And then there are certain companies who seem to equate application patching with OS patching who send us private emails insisting these things need fixed. So for us to accept this, you will have to make a better argument that "don't use milestone releases".

[reschke]

Don't let broken SCA tools govern what you use. Better listen to the people who write the projects that you use.

If you're totally concerned by what broken SCA tools report, you probably can override the Junit dependency in your pom.

Should I try that?

[xeno6696]

That is what we recommend developers to do if for any reason they disagree with our dependency choices.

And it's not that the SCA tools (and there are well over 20 vendors) are broken, it's that developers downstream don't bother to do analysis nor even read our own release notes. A blinky red light CVE is a blinky red light. For awhile we were writing advisories that would for example, demonstrate that vulnerable code isn't reachable. However in the real world, most auditors don't have a software background, and they (and many developers) take SCA results as gospel truth. Which means those orgs even after being given those advisories block releases which then creates community pressure to just update the dependency anyway.

So then for every bug report like this one, we get many more emails/reports complaining that we didn't remove it, and being that there's only 3 of us, it's a question of how do we want to spend our time--writing code fixes or answering the same question 30x? Maybe you have the energy to die on that hill, that's not me. I suspect, not Kevin either.

I would turn the question against the commons developers: why not do regular releases with clean dependencies that aren't tied to milestone testing?

[reschke]

Well.

It would be good if SCA tools would flag use of unstable dependencies as well.

[kwwall]

@reschke wrote:

Don't let broken SCA tools govern what you use. Better listen to the people who write the projects that you use.

To add to what @xeno6696 wrote, we don't chose what SCA tools others use, so there's also that. But more to Matt's point, the people making the decisions about the SCA reports for the most part don't understand there's more to it than a "blinky red light". If people want to treat all the warnings it notifies you about as Gospel truth, then I guess they get what they deserve.

That's why we go to great lengths to write up detailed vulnerability analysis in the form of the various security bulletins that we publish when there are issues that we either can't fix at all or can't fix immediately. (There's also the whole problem of treating vulnerability severity (usually reflected as CVSS base scores) as the equivalent of risk scores, but if I went down that rabbit trail, I'd be here all night.)

If you're totally concerned by what broken SCA tools report, you probably can override the Junit dependency in your pom.

The problem is that it's not our JUnit tests that use Commons Collections 4 that has the vulnerabilities, it Commons Collections 4 JUnit tests, and I don't know what we can do about that, short of maybe making a PR to fix it for them, and I just don't have time for that.

As it turns out, we have only have one single direct dependency on Commons Collections 4:

• https://github.com/ESAPI/esapi-java-legacy/blob/develop/src/main/java/org/owasp/esapi/reference/accesscontrol/DelegatingACR.java#L9

So we addressed what we could. However, we also have some transitive dependencies on it as seen via the dependency tree:

$ mvn -B dependency:tree
...
[INFO] +- commons-beanutils:commons-beanutils:jar:1.9.4:compile
[INFO] |  +- commons-logging:commons-logging:jar:1.2:compile
[INFO] |  \- commons-collections:commons-collections:jar:3.2.2:compile
...

We can't as easily replace that one because the package namespace for Collections 3 and Collections 4 is different and it's in a transitive dependency, so we simply fixed we could do with relatively little effort. (Did we mention there's only 3 of us working on this and all 3 of us have full time jobs?)

Ideally, I'd like to see us get rid of our direct dependency on Collections 4 in DelegatingACR but none of us on the core team is that familiar with the ESAPI AccessController implementation, and the JUnit tests for it are pretty skimpy so we're reluctant to change and break something (even though IMO, I think no one is using the reference implementation in the commercial world as it just doesn't scale well). So we updated it to Collections 4 and pretty much left it alone just to drop all the grumbling via private emails to a lower volume.

Should I try that?

If you are concerned with using a "milestone" release (although the Collections 4 README.md seems to imply it's fine to use for production, and looking at their release tags, it appears this is the new normal for them as the have RC (release candidate) releases for their milestone releases), then what I would recommend is to add an exclusion for the Collections 4 version that ESAPI is dragging in and use the 4.3 version.
So, for Maven, it would be something like this (I did this off the top of my head, so check it for errors):

     ...
        <dependency>
            <groupId>org.owasp.esapi</groupId>
            <artifactId>esapi</artifactId>
            <classifier>jakarta</classifier>
            <version>2.6.0.0</version>
            <exclusions>
                <!-- Don't like milestone releases, so exclude this transitive dependency -->
                <exclusion>
                    <groupId>commons-logging</groupId>
                    <artifactId>commons-logging</artifactId>
                </exclusion>
        </dependency>
        <dependency>
            <!-- And import an older version. -->
            <groupId>org.apache.commons</groupId>
            <artifactId>commons-collections4</artifactId>
            <version>4.4</version>
        </dependency>
      ...

Just don't blame us if your SCA tools red blinky lights starts flashing.

P.S.- Let me know if you want to continue this conversation and I will turn it into a discussion on our Discussions forum. Of if you want to volunteer to do a PR to get rid of the Collections 4 dependency in DelegatingACR, I can re-open this issue, but otherwise, I'm closing it because we don't intent to revert to the 4.4 release.

[reschke]

Thanks for the feedback; will have to think about this.

[reschke]

We can't as easily replace that one because the package namespace for Collections 3 and Collections 4 is different and it's in a transitive dependency, so we simply fixed we could do with relatively little effort. (Did we mention there's only 3 of us working on this and all 3 of us have full time jobs?)

beanutils just had a new release, but surprisingly it still uses the old commons-collections project (probably because it's from a maintenance branch).

Ideally, I'd like to see us get rid of our direct dependency on Collections 4 in DelegatingACR but none of us on the core team is that familiar with the ESAPI AccessController implementation...

Well, that fix is entirely trivial:

diff --git a/pom.xml b/pom.xml
index e5b99b91..f0d9050d 100644
--- a/pom.xml
+++ b/pom.xml
@@ -230,11 +230,6 @@
                 </exclusion>
             </exclusions>
         </dependency>
-        <dependency>
-            <groupId>org.apache.commons</groupId>
-            <artifactId>commons-collections4</artifactId>
-            <version>4.5.0-M2</version>
-        </dependency>
         <dependency>
             <groupId>org.apache-extras.beanshell</groupId>
             <artifactId>bsh</artifactId>
diff --git a/src/main/java/org/owasp/esapi/reference/accesscontrol/DelegatingACR.java b/src/main/java/org/owasp/esapi/reference/accesscontrol/DelegatingACR.java
index eaa35799..1209aa08 100644
--- a/src/main/java/org/owasp/esapi/reference/accesscontrol/DelegatingACR.java
+++ b/src/main/java/org/owasp/esapi/reference/accesscontrol/DelegatingACR.java
@@ -6,8 +6,6 @@ import java.util.Iterator;
 import java.util.Vector;
 import java.util.Arrays;
 
-import org.apache.commons.collections4.iterators.ArrayListIterator;
-
 public class DelegatingACR extends BaseACR<DynaBeanACRParameter, Object[]> {
     protected Method delegateMethod;
     protected Object delegateInstance;
@@ -66,10 +64,9 @@ public class DelegatingACR extends BaseACR<DynaBeanACRParameter, Object[]> {
         if (parameterClassNames == null) {
             return new Class[0];
         }
-        Vector<Class> classes = new Vector<Class>();
-        Iterator<String> classNames = new ArrayListIterator(parameterClassNames);
-        while(classNames.hasNext()) {
-            classes.add(getClass(classNames.next(), "parameter"));
+        Vector<Class> classes = new Vector<>();
+        for (String className : parameterClassNames) {
+            classes.add(getClass(className, "parameter"));
         }
         return classes.toArray(new Class[classes.size()]);
     }

So the use of ArrayListIterator didn't make any sense in the first place. Should I open ticket and PR?

[reschke]

Here we go: #870

[xeno6696]

Missed the conversation here before I had approved the PR yesterday.

@reschke Thank you very much for leapfrogging either Kevin or I on investigating that.