CVE-2023-41886: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: OpenRefine is a powerful free, open source tool for working with messy data. Prior to version 3.7.5, an arbitrary file read vulnerability allows any unauthenticated user to read a file on a server. Version 3.7.5 fixes this issue.
CWE description: The product constructs all or part of an SQL command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended SQL command when it is sent to a downstream component. Without sufficient removal or quoting of SQL syntax in user-controllable inputs, the generated SQL query can cause those inputs to be interpreted as SQL instead of ordinary user data.
CVSS v3 base score: 7.5
Published: 2023-09-15T21:15Z
Repository: https://github.com/OpenRefine/OpenRefine
Language: Java
Files changed: extensions/database/src/com/google/refine/extension/database/DatabaseConfiguration.java, extensions/database/src/com/google/refine/extension/database/mariadb/MariaDBConnectionManager.java, extensions/database/src/com/google/refine/extension/database/mysql/MySQLConnectionManager.java, extensions/database/src/com/google/refine/extension/database/pgsql/PgSQLConnectionManager.java, extensions/database/src/com/google/refine/extension/database/sqlite/SQLiteConnectionManager.java, extensions/database/tests/src/com/google/refine/extension/database/DatabaseConfigurationTest.java
Commit date: 2023-09-12 08:34:13 +0200
Commit message: Merge pull request from GHSA-qqh2-wvmv-h72m


Diff: diff --git a/extensions/database/src/com/google/refine/extension/database/DatabaseConfiguration.java b/extensions/database/src/com/google/refine/extension/database/DatabaseConfiguration.java
index c9b8976bd..4f4a14bef 100644
--- a/extensions/database/src/com/google/refine/extension/database/DatabaseConfiguration.java
+++ b/extensions/database/src/com/google/refine/extension/database/DatabaseConfiguration.java
@@ -27,10 +27,13 @@
  * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 package com.google.refine.extension.database;
 
+import java.net.URI;
+import java.net.URISyntaxException;
+
 public class DatabaseConfiguration {
 
     private String connectionName;
     private String databaseType;
     private String databaseHost;
@@ -121,6 +124,19 @@ public class DatabaseConfiguration {
                 + ", databaseHost=" + databaseHost + ", databasePort=" + databasePort + ", databaseUser=" + databaseUser
                 + ", databaseName=" + databaseName + ", databaseSchema="
                 + databaseSchema + ", useSSL=" + useSSL + "]";
     }
 
+    public URI toURI() {
+        try {
+            return new URI(
+                    "jdbc:" + databaseType.toLowerCase(),
+                    databaseHost + ((databasePort == 0) ? "" : (":" + databasePort)),
+                    "/" + databaseName,
+                    useSSL ? "useSSL=true" : null,
+                    null
+            );
+        } catch (URISyntaxException e) {
+            throw new IllegalArgumentException(e);
+        }
+    }
 }
diff --git a/extensions/database/src/com/google/refine/extension/database/mariadb/MariaDBConnectionManager.java b/extensions/database/src/com/google/refine/extension/database/mariadb/MariaDBConnectionManager.java
index 422735d1c..93d63c00e 100644
--- a/extensions/database/src/com/google/refine/extension/database/mariadb/MariaDBConnectionManager.java
+++ b/extensions/database/src/com/google/refine/extension/database/mariadb/MariaDBConnectionManager.java
@@ -132,11 +132,11 @@ public class MariaDBConnectionManager {
                 }
             }
 
             Class.forName(type.getClassPath());
             DriverManager.setLoginTimeout(10);
-            String dbURL = getDatabaseUrl(databaseConfiguration);
+            String dbURL = databaseConfiguration.toURI().toString();
             connection = DriverManager.getConnection(dbURL, databaseConfiguration.getDatabaseUser(),
                     databaseConfiguration.getDatabasePassword());
 
             if (logger.isDebugEnabled()) {
                 logger.debug("*** Acquired New  connection for ::{} **** ", dbURL);
@@ -160,16 +160,8 @@ public class MariaDBConnectionManager {
                 connection.close();
             } catch (SQLException e) {
                 logger.warn("Non-Managed connection could not be closed. Whoops!", e);
             }
         }
-
     }
 
-    private static String getDatabaseUrl(DatabaseConfiguration dbConfig) {
-
-        int port = dbConfig.getDatabasePort();
-        return "jdbc:" + dbConfig.getDatabaseType().toLowerCase() + "://" + dbConfig.getDatabaseHost()
-                + ((port == 0) ? "" : (":" + port)) + "/" + dbConfig.getDatabaseName();
-
-    }
 }
diff --git a/extensions/database/src/com/google/refine/extension/database/mysql/MySQLConnectionManager.java b/extensions/database/src/com/google/refine/extension/database/mysql/MySQLConnectionManager.java
index f4a7ce5dd..4fea86a90 100644
--- a/extensions/database/src/com/google/refine/extension/database/mysql/MySQLConnectionManager.java
+++ b/extensions/database/src/com/google/refine/extension/database/mysql/MySQLConnectionManager.java
@@ -127,11 +127,11 @@ public class MySQLConnectionManager {
                     }
 
                     return connection;
                 }
             }
-            String dbURL = getDatabaseUrl(databaseConfiguration);
+            String dbURL = databaseConfiguration.toURI().toString();
             Class.forName(type.getClassPath());
 
             // logger.info("*** type.getClassPath() ::{}, {}**** ", type.getClassPath());
 
             DriverManager.setLoginTimeout(10);
@@ -164,13 +164,6 @@ public class MySQLConnectionManager {
             }
         }
 
     }
 
-    private String getDatabaseUrl(DatabaseConfiguration dbConfig) {
-
-        int port = dbConfig.getDatabasePort();
-        return "jdbc:" + dbConfig.getDatabaseType() + "://" + dbConfig.getDatabaseHost()
-                + ((port == 0) ? "" : (":" + port)) + "/" + dbConfig.getDatabaseName() + "?useSSL=" + dbConfig.isUseSSL();
-
-    }
 }
diff --git a/extensions/database/src/com/google/refine/extension/database/pgsql/PgSQLConnectionManager.java b/extensions/database/src/com/google/refine/extension/database/pgsql/PgSQLConnectionManager.java
index bea76d091..1637c222e 100644
--- a/extensions/database/src/com/google/refine/extension/database/pgsql/PgSQLConnectionManager.java
+++ b/extensions/database/src/com/google/refine/extension/database/pgsql/PgSQLConnectionManager.java
@@ -134,11 +134,11 @@ public class PgSQLConnectionManager {
                 }
             }
 
             Class.forName(type.getClassPath());
             DriverManager.setLoginTimeout(10);
-            String dbURL = getDatabaseUrl(databaseConfiguration);
+            String dbURL = databaseConfiguration.toURI().toString();
             connection = DriverManager.getConnection(dbURL, databaseConfiguration.getDatabaseUser(),
                     databaseConfiguration.getDatabasePassword());
 
             logger.debug("*** Acquired New  connection for ::{} **** ", dbURL);
 
@@ -163,13 +163,6 @@ public class PgSQLConnectionManager {
             }
         }
 
     }
 
-    private static String getDatabaseUrl(DatabaseConfiguration dbConfig) {
-
-        int port = dbConfig.getDatabasePort();
-        return "jdbc:" + dbConfig.getDatabaseType().toLowerCase() + "://" + dbConfig.getDatabaseHost()
-                + ((port == 0) ? "" : (":" + port)) + "/" + dbConfig.getDatabaseName();
-
-    }
 }
diff --git a/extensions/database/src/com/google/refine/extension/database/sqlite/SQLiteConnectionManager.java b/extensions/database/src/com/google/refine/extension/database/sqlite/SQLiteConnectionManager.java
index 5b9b4cfcd..7d42e002a 100644
--- a/extensions/database/src/com/google/refine/extension/database/sqlite/SQLiteConnectionManager.java
+++ b/extensions/database/src/com/google/refine/extension/database/sqlite/SQLiteConnectionManager.java
@@ -33,10 +33,12 @@ import com.google.refine.extension.database.DatabaseConfiguration;
 import com.google.refine.extension.database.DatabaseServiceException;
 import com.google.refine.extension.database.SQLType;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.sql.Connection;
 import java.sql.DriverManager;
 import java.sql.SQLException;
 
 public class SQLiteConnectionManager {
@@ -64,11 +66,16 @@ public class SQLiteConnectionManager {
         }
         return instance;
     }
 
     public static String getDatabaseUrl(DatabaseConfiguration dbConfig) {
-        return "jdbc:" + dbConfig.getDatabaseType().toLowerCase() + ":" + dbConfig.getDatabaseName();
+        try {
+            URI uri = new URI("jdbc:" + dbConfig.getDatabaseType().toLowerCase(), dbConfig.getDatabaseName(), null);
+            return uri.toASCIIString();
+        } catch (URISyntaxException e) {
+            throw new IllegalArgumentException(e);
+        }
     }
 
     /**
      * Get the SQL Database type.
      *
diff --git a/extensions/database/tests/src/com/google/refine/extension/database/DatabaseConfigurationTest.java b/extensions/database/tests/src/com/google/refine/extension/database/DatabaseConfigurationTest.java
new file mode 100644
index 000000000..5a571e8f0
--- /dev/null
+++ b/extensions/database/tests/src/com/google/refine/extension/database/DatabaseConfigurationTest.java
@@ -0,0 +1,21 @@
+package com.google.refine.extension.database;
+
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+public class DatabaseConfigurationTest {
+
+    @Test
+    public void testToURI() {
+        DatabaseConfiguration config = new DatabaseConfiguration();
+        config.setDatabaseType("mysql");
+        config.setDatabaseHost("my.host");
+        // maliciously crafted database name which attempts to enable local file reads for an exploit
+        config.setDatabaseName("test?allowLoadLocalInfile=true#");
+
+        String url = config.toURI().toString();
+        // the database name is escaped, preventing the exploit
+        assertEquals(url, "jdbc:mysql://my.host/test%3FallowLoadLocalInfile=true%23");
+    }
+}

Vulnerable code:             String dbURL = getDatabaseUrl(databaseConfiguration);

    private static String getDatabaseUrl(DatabaseConfiguration dbConfig) {

        int port = dbConfig.getDatabasePort();
        return "jdbc:" + dbConfig.getDatabaseType().toLowerCase() + "://" + dbConfig.getDatabaseHost()
                + ((port == 0) ? "" : (":" + port)) + "/" + dbConfig.getDatabaseName();

    }
            String dbURL = getDatabaseUrl(databaseConfiguration);
    private String getDatabaseUrl(DatabaseConfiguration dbConfig) {

        int port = dbConfig.getDatabasePort();
        return "jdbc:" + dbConfig.getDatabaseType() + "://" + dbConfig.getDatabaseHost()
                + ((port == 0) ? "" : (":" + port)) + "/" + dbConfig.getDatabaseName() + "?useSSL=" + dbConfig.isUseSSL();

    }
            String dbURL = getDatabaseUrl(databaseConfiguration);
    private static String getDatabaseUrl(DatabaseConfiguration dbConfig) {

        int port = dbConfig.getDatabasePort();
        return "jdbc:" + dbConfig.getDatabaseType().toLowerCase() + "://" + dbConfig.getDatabaseHost()
                + ((port == 0) ? "" : (":" + port)) + "/" + dbConfig.getDatabaseName();

    }
        return "jdbc:" + dbConfig.getDatabaseType().toLowerCase() + ":" + dbConfig.getDatabaseName();
Fixed code: import java.net.URI;
import java.net.URISyntaxException;

    public URI toURI() {
        try {
            return new URI(
                    "jdbc:" + databaseType.toLowerCase(),
                    databaseHost + ((databasePort == 0) ? "" : (":" + databasePort)),
                    "/" + databaseName,
                    useSSL ? "useSSL=true" : null,
                    null
            );
        } catch (URISyntaxException e) {
            throw new IllegalArgumentException(e);
        }
    }
            String dbURL = databaseConfiguration.toURI().toString();
            String dbURL = databaseConfiguration.toURI().toString();
            String dbURL = databaseConfiguration.toURI().toString();
import java.net.URI;
import java.net.URISyntaxException;
        try {
            URI uri = new URI("jdbc:" + dbConfig.getDatabaseType().toLowerCase(), dbConfig.getDatabaseName(), null);
            return uri.toASCIIString();
        } catch (URISyntaxException e) {
            throw new IllegalArgumentException(e);
        }
package com.google.refine.extension.database;

import org.testng.annotations.Test;

import static org.testng.Assert.assertEquals;

public class DatabaseConfigurationTest {

    @Test
    public void testToURI() {
        DatabaseConfiguration config = new DatabaseConfiguration();
        config.setDatabaseType("mysql");
        config.setDatabaseHost("my.host");
        // maliciously crafted database name which attempts to enable local file reads for an exploit
        config.setDatabaseName("test?allowLoadLocalInfile=true#");

        String url = config.toURI().toString();
        // the database name is escaped, preventing the exploit
        assertEquals(url, "jdbc:mysql://my.host/test%3FallowLoadLocalInfile=true%23");
    }
}