FP on squid:S2658 with `Class.forName(CONSTANT)`

java

(Jens Bannmann) #1

When calling Class.forName() with a compile-time constant, the class loading is not depending on any externally supplied value. In other words, it is not dynamic at all, but rather static, and not a vulnerability: if somebody managed to exploit an app that calls Class.forName("com.example.AnotherClass"), they could just as easily exploit an app that calls new com.example.AnotherClass().

However, SonarJava 5.5 and earlier (on SonarCloud) report an issue from S2658 saying “Remove this use of dynamic class loading.”

In my opinion, S2658 should make an exception if the argument to Class.forName() is a compile-time constant - either a string literal parameter or a reference to a static final constant with a compile-time value (i.e. no System.getProperty() or the like).

One use case for such class loading is if you have optional dependencies, as illustrated below in the case of a JSON marshaller. Another, more historic use case is registering a JDBC driver (which is no longer needed for JDBC 4.0 drivers).

import com.google.gson.GsonBuilder;

public class Demo
{
    private static final String DATE_TIME_CLASS = "org.joda.time.DateTime";

    public void initialize(GsonBuilder gsonBuilder)
    {
        try
        {
            Class<?> dateTimeClass = Class.forName(DATE_TIME_CLASS); // FP: reference to constant
            gsonBuilder.registerTypeAdapter(dateTimeClass, new DateTimeTypeAdapter());
        }
        catch (ClassNotFoundException ignored)
        {
        }
        try
        {
            Class<?> dateTimeZoneClass = Class.forName("org.joda.time.DateTimeZone"); // FP: string literal
            gsonBuilder.registerTypeAdapter(dateTimeZoneClass, new DateTimeZoneTypeAdapter());
        }
        catch (ClassNotFoundException ignored)
        {
        }
    }
}

JDBC drivers should not be registered via `Class.forName()`
(Alexandre Gigleux) #2

Hello,

S2658 implementation is really not clever as said in the rule’s description:

This rule raises an issue for each use of dynamic class loading

The idea was to raise the issues and have a security auditor reviewing them to confirm whether or not there is a vulnerability to fix.

Anyway, I believe you are right for the cases you mentioned and so I created https://jira.sonarsource.com/browse/SONARJAVA-2819 to raise fewer issues.

Thanks