Wrapper SQL sink method triggers SQL injection detection

[jim-bentler]

Environment

SpotBugs Eclipse plugin: SpotBugs 4.8.4.202401211731-1e2b791 com.github.spotbugs.plugin.eclipse
FindSecBugs plugin: findsecbugs-plugin-1.12.0.jar
Eclipse running in Java 21

Problem

A wrapper method configured as a custom SQL sink that simply calls another method vulnerable to SQL injection should not itself trigger detection of a SQL injection bug.

Code

Suppose we have the following simple utility:

package com.testing;

import java.sql.Connection;
import java.sql.SQLException;
import java.util.Collections;
import java.util.List;
import java.util.Map;

public interface SQLHelper {
	public default List<Map<String, Object>> executeQuery(Connection connection, String command) throws SQLException {
		return executeQuery(connection, command, Collections.emptyMap());
	}

	public List<Map<String, Object>> executeQuery(Connection connection, String command,
			Map<String, Object> valueMap) throws SQLException;
}

And the following added via -Dfindsecbugs.injection.customconfigfile.SqlInjectionDetector=custom.txt:

com/testing/SQLHelper.executeQuery(Ljava/sql/Connection;Ljava/lang/String;)Ljava/util/List;:0
com/testing/SQLHelper.executeQuery(Ljava/sql/Connection;Ljava/lang/String;Ljava/util/Map;)Ljava/util/List;:1

This simple unit test will trigger an expected bug detection:

package com.testing;

import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.mockito.Mockito.mock;

import java.sql.Connection;
import java.sql.SQLException;

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

class SQLHelperTest {
	@ParameterizedTest
	@ValueSource(strings = { "c1", "c2" })
	void testExecuteQuery(String input) throws SQLException {
		SQLHelper sqlHelper = mock();
		Connection connection = mock();
		String command = "select " + input + " from table1";
		var result = sqlHelper.executeQuery(connection, command);

		assertNotNull(result);
	}
}

Here is the issue detected:

Bug: This use of com/testing/SQLHelper.executeQuery(Ljava/sql/Connection;Ljava/lang/String;)Ljava/util/List; can be vulnerable to SQL injection (with JDBC)

The input values included in SQL queries need to be passed in safely. Bind variables in prepared statements can be used to easily mitigate the risk of SQL injection. 

But the following issue is incorrectly detected as well:

Bug: This use of com/testing/SQLHelper.executeQuery(Ljava/sql/Connection;Ljava/lang/String;Ljava/util/Map;)Ljava/util/List; can be vulnerable to SQL injection (with JDBC)

The input values included in SQL queries need to be passed in safely. Bind variables in prepared statements can be used to easily mitigate the risk of SQL injection. 

Such a wrapper method should not trigger the detection. Simply passing the value on (unmodified) to another method requiring an untainted string should not trigger any problem.

[h3xstream]

Interprocedural analysis are not in scope right now. SpotBugs and FindSecBugs are not doing call graph. Taint analysis is only applied at the function level.

[jim-bentler]

I am not asking for interprocedural analysis. The method calls should be treated according to how the method has been marked.

I am asking for it to respect how both the caller and the called method have been annotated. When the SQL has not been manipulated in a calling method with the same annotated taint requirements as the method it calls, this should not itself trigger a warning.

[h3xstream]

Can you provide the file/class and line number where each are found?

I am not sure I understand what is the false positive.

---

This line looks like it should be flagged as a new sink instruct to be used.

var result = sqlHelper.executeQuery(connection, command);

I am not sure where the second finding is occuring.

[jim-bentler]

The following issue is incorrectly detected:

Bug: This use of com/testing/SQLHelper.executeQuery(Ljava/sql/Connection;Ljava/lang/String;Ljava/util/Map;)Ljava/util/List; can be vulnerable to SQL injection (with JDBC)

The input values included in SQL queries need to be passed in safely. Bind variables in prepared statements can be used to easily mitigate the risk of SQL injection. 
Such a wrapper method should not trigger the detection. Simply passing the value on (unmodified) to another method requiring an untainted string should not trigger any problem.

It is triggered by the body of this method in SQLHelper.java:

	public default List<Map<String, Object>> executeQuery(Connection connection, String command) throws SQLException {
		return executeQuery(connection, command, Collections.emptyMap());
	}

But both the method and the calling method have the String command parameter marked via -Dfindsecbugs.injection.customconfigfile.SqlInjectionDetector=custom.txt:

com/testing/SQLHelper.executeQuery(Ljava/sql/Connection;Ljava/lang/String;)Ljava/util/List;:0
com/testing/SQLHelper.executeQuery(Ljava/sql/Connection;Ljava/lang/String;Ljava/util/Map;)Ljava/util/List;:1

Given that it just passes the value though, it shouldn't trigger its own warning.