From 65e186f15cac6b1d8af94f8e3c5bac1742aff057 Mon Sep 17 00:00:00 2001 From: Aurora Lahtela <24460436+AuroraLS3@users.noreply.github.com> Date: Sun, 15 Jan 2023 13:42:13 +0200 Subject: [PATCH] Backported SQL Injection vulnerability fix and other small fixes --- .../plan/delivery/web/ResourceSvc.java | 5 +++++ .../gathering/domain/event/JoinAddress.java | 6 +++++- .../queries/QueryParameterSetter.java | 21 +++++++++++++++++-- .../queries/objects/GeoInfoQueries.java | 7 +++---- .../storage/database/sql/building/Sql.java | 9 ++++++++ .../plan/storage/file/PlanFiles.java | 3 +++ 6 files changed, 44 insertions(+), 7 deletions(-) diff --git a/Plan/common/src/main/java/com/djrapitops/plan/delivery/web/ResourceSvc.java b/Plan/common/src/main/java/com/djrapitops/plan/delivery/web/ResourceSvc.java index 54fb14e0c..1df0476ad 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/delivery/web/ResourceSvc.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/delivery/web/ResourceSvc.java @@ -175,6 +175,11 @@ public class ResourceSvc implements ResourceService { byte[] bytes = original.asBytes(); OpenOption[] overwrite = {StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE}; Path to = resourceSettings.getCustomizationDirectory().resolve(fileName); + if (!to.startsWith(resourceSettings.getCustomizationDirectory())) { + throw new IllegalArgumentException( + "Absolute path was given for writing a customized file, " + + "writing outside customization directory is prevented for security reasons."); + } Path dir = to.getParent(); if (!Files.isSymbolicLink(dir)) Files.createDirectories(dir); Files.write(to, bytes, overwrite); diff --git a/Plan/common/src/main/java/com/djrapitops/plan/gathering/domain/event/JoinAddress.java b/Plan/common/src/main/java/com/djrapitops/plan/gathering/domain/event/JoinAddress.java index 052170001..5b118e31a 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/gathering/domain/event/JoinAddress.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/gathering/domain/event/JoinAddress.java @@ -31,7 +31,11 @@ public class JoinAddress { } public String getAddress() { - return address.get(); + String joinAddress = address.get(); + if (joinAddress.contains("\u0000")) { + joinAddress = joinAddress.split("\u0000", 2)[0]; + } + return joinAddress; } @Override diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryParameterSetter.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryParameterSetter.java index c91de3f6d..2fec50d54 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryParameterSetter.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryParameterSetter.java @@ -21,6 +21,7 @@ import com.djrapitops.plan.identification.ServerUUID; import java.sql.PreparedStatement; import java.sql.SQLException; import java.sql.Types; +import java.util.Collection; import java.util.UUID; public class QueryParameterSetter { @@ -30,20 +31,36 @@ public class QueryParameterSetter { public static void setParameters(PreparedStatement statement, Object... parameters) throws SQLException { int index = 1; for (Object parameter : parameters) { - setParameter(statement, index, parameter); - index++; + if (parameter instanceof Object[]) { + for (Object arrayParameter : (Object[]) parameter) { + setParameter(statement, index, arrayParameter); + index++; + } + } else if (parameter instanceof Collection) { + for (Object collectionParameter : (Collection) parameter) { + setParameter(statement, index, collectionParameter); + index++; + } + } else { + setParameter(statement, index, parameter); + index++; + } } } private static void setParameter(PreparedStatement statement, int index, Object parameter) throws SQLException { if (parameter == null) { statement.setNull(index, Types.VARCHAR); + } else if (parameter instanceof Boolean) { + statement.setBoolean(index, (Boolean) parameter); } else if (parameter instanceof Integer) { statement.setInt(index, (Integer) parameter); } else if (parameter instanceof Long) { statement.setLong(index, (Long) parameter); } else if (parameter instanceof Double) { statement.setDouble(index, (Double) parameter); + } else if (parameter instanceof Character) { + statement.setString(index, String.valueOf(parameter)); } else if (parameter instanceof Float) { statement.setFloat(index, (Float) parameter); } else if (parameter instanceof String) { diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/GeoInfoQueries.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/GeoInfoQueries.java index ffaf181fd..ee3ea4de3 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/GeoInfoQueries.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/GeoInfoQueries.java @@ -21,6 +21,7 @@ import com.djrapitops.plan.identification.ServerUUID; import com.djrapitops.plan.storage.database.queries.Query; import com.djrapitops.plan.storage.database.queries.QueryAllStatement; import com.djrapitops.plan.storage.database.queries.RowExtractors; +import com.djrapitops.plan.storage.database.sql.building.Sql; import com.djrapitops.plan.storage.database.sql.tables.GeoInfoTable; import com.djrapitops.plan.storage.database.sql.tables.ServerTable; import com.djrapitops.plan.storage.database.sql.tables.UserInfoTable; @@ -171,9 +172,7 @@ public class GeoInfoQueries { FROM + GeoInfoTable.TABLE_NAME + " g" + INNER_JOIN + UsersTable.TABLE_NAME + " u on u.id=g." + GeoInfoTable.USER_ID + WHERE + GeoInfoTable.GEOLOCATION + - " IN ('" + - new TextStringBuilder().appendWithSeparators(selected, "','") + - "')"; - return db -> db.querySet(sql, RowExtractors.getInt(UsersTable.ID)); + " IN (" + Sql.nParameters(selected.size()) + ")"; + return db -> db.querySet(sql, RowExtractors.getInt(UsersTable.ID), selected); } } \ No newline at end of file diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/sql/building/Sql.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/sql/building/Sql.java index 6fa9b078e..edfed2755 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/sql/building/Sql.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/sql/building/Sql.java @@ -16,10 +16,13 @@ */ package com.djrapitops.plan.storage.database.sql.building; +import org.apache.commons.text.TextStringBuilder; + import java.sql.PreparedStatement; import java.sql.SQLException; import java.sql.Types; import java.util.concurrent.TimeUnit; +import java.util.stream.IntStream; /** * Duplicate String reducing utility class for SQL language Strings. @@ -56,6 +59,12 @@ public abstract class Sql { private static final String MAX = "MAX("; private static final String VARCHAR = "varchar("; + public static String nParameters(int n) { + return new TextStringBuilder() + .appendWithSeparators(IntStream.range(0, n).mapToObj(i -> "?").iterator(), ",") + .toString(); + } + public static String varchar(int length) { return VARCHAR + length + ')'; } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java index b4fef6c15..f52946f56 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java @@ -161,6 +161,9 @@ public class PlanFiles implements SubSystem { Path dir = config.get().getResourceSettings().getCustomizationDirectory(); if (dir.toFile().exists() && dir.toFile().isDirectory()) { Path asPath = dir.resolve(resourceName); + if (!asPath.startsWith(dir)) { + return Optional.empty(); + } File found = asPath.toFile(); if (found.exists()) { return Optional.of(found);