Skip to content

Commit f955dc3

Browse files
HandSonicHandSonic
committed
fix(jdbc): harden driver upload boundaries
1 parent b608dff commit f955dc3

6 files changed

Lines changed: 143 additions & 27 deletions

File tree

‎chat2db-community-server/chat2db-community-domain/chat2db-community-domain-core/src/main/java/ai/chat2db/community/domain/core/impl/db/JdbcDriverManagementPolicy.java‎

Lines changed: 37 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,21 @@
33
import ai.chat2db.community.tools.exception.BusinessException;
44
import org.apache.commons.lang3.StringUtils;
55

6+
import java.io.IOException;
7+
import java.io.InputStream;
8+
import java.io.OutputStream;
69
import java.nio.file.Files;
7-
import java.nio.file.FileAlreadyExistsException;
8-
import java.nio.file.FileSystemException;
10+
import java.nio.file.LinkOption;
911
import java.nio.file.Path;
12+
import java.nio.file.StandardOpenOption;
1013
import java.util.ArrayList;
1114
import java.util.List;
1215
import java.util.regex.Matcher;
1316
import java.util.regex.Pattern;
1417

1518
final class JdbcDriverManagementPolicy {
1619

17-
private static final Pattern UPLOAD_TOKEN = Pattern.compile("^([0-9a-f]{32}):([^/:\\\\]+\\.jar)$",
20+
private static final Pattern UPLOAD_TOKEN = Pattern.compile("^([0-9a-f]{32}):([^,/:\\\\]+\\.jar)$",
1821
Pattern.CASE_INSENSITIVE);
1922

2023
record PromotedDrivers(String jdbcDriver, List<Path> files) {
@@ -53,14 +56,19 @@ static PromotedDrivers promoteUploadedDrivers(List<String> uploadTokens, Path st
5356
}
5457
String uploadId = matcher.group(1);
5558
String driverName = matcher.group(2);
59+
if (uploadId.contains("..") || uploadId.contains("/") || uploadId.contains("\\")
60+
|| driverName.contains("..") || driverName.contains("/") || driverName.contains("\\")) {
61+
throw uploadFailure("invalid driver upload token");
62+
}
5663
Path stagedFile = normalizedStagingDirectory.resolve(uploadId + ".upload").normalize();
5764
Path driverFile = normalizedDirectory.resolve(driverName).normalize();
5865
targets.add(new UploadTarget(driverName, stagedFile, driverFile));
59-
if (!normalizedStagingDirectory.equals(stagedFile.getParent()) || !Files.isRegularFile(stagedFile)
66+
if (!normalizedStagingDirectory.equals(stagedFile.getParent())
67+
|| !Files.isRegularFile(stagedFile, LinkOption.NOFOLLOW_LINKS)
6068
|| !normalizedDirectory.equals(driverFile.getParent())) {
6169
throw uploadFailure("uploaded driver file is unavailable");
6270
}
63-
if (Files.exists(driverFile)) {
71+
if (Files.exists(driverFile, LinkOption.NOFOLLOW_LINKS)) {
6472
throw uploadFailure("a managed driver with the same file name already exists");
6573
}
6674
}
@@ -92,17 +100,7 @@ static PromotedDrivers promoteUploadedDrivers(List<String> uploadTokens, Path st
92100
}
93101

94102
static void moveWithoutReplacement(Path source, Path target) throws Exception {
95-
moveWithoutReplacement(source, target, (link, existing) -> Files.createLink(link, existing));
96-
}
97-
98-
static void moveWithoutReplacement(Path source, Path target, LinkCreator linkCreator) throws Exception {
99-
try {
100-
linkCreator.create(target, source);
101-
} catch (FileAlreadyExistsException exception) {
102-
throw exception;
103-
} catch (UnsupportedOperationException | FileSystemException ignored) {
104-
Files.copy(source, target);
105-
}
103+
copyRegularFileWithoutReplacement(source, target);
106104
try {
107105
Files.delete(source);
108106
} catch (Exception exception) {
@@ -115,6 +113,29 @@ static void moveWithoutReplacement(Path source, Path target, LinkCreator linkCre
115113
}
116114
}
117115

116+
private static void copyRegularFileWithoutReplacement(Path source, Path target) throws IOException {
117+
if (!Files.isRegularFile(source, LinkOption.NOFOLLOW_LINKS)) {
118+
throw new IOException("uploaded driver file is not a regular file");
119+
}
120+
boolean targetCreated = false;
121+
try (InputStream input = Files.newInputStream(
122+
source, StandardOpenOption.READ, LinkOption.NOFOLLOW_LINKS);
123+
OutputStream output = Files.newOutputStream(
124+
target, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE)) {
125+
targetCreated = true;
126+
input.transferTo(output);
127+
} catch (IOException | RuntimeException exception) {
128+
if (targetCreated) {
129+
try {
130+
Files.deleteIfExists(target);
131+
} catch (Exception cleanupFailure) {
132+
exception.addSuppressed(cleanupFailure);
133+
}
134+
}
135+
throw exception;
136+
}
137+
}
138+
118139
private static void cleanupStagedFiles(List<UploadTarget> targets) {
119140
for (UploadTarget target : targets) {
120141
try {
@@ -128,11 +149,6 @@ private static void cleanupStagedFiles(List<UploadTarget> targets) {
128149
private record UploadTarget(String driverName, Path stagedFile, Path driverFile) {
129150
}
130151

131-
@FunctionalInterface
132-
interface LinkCreator {
133-
void create(Path link, Path existing) throws Exception;
134-
}
135-
136152
private static BusinessException uploadFailure(String reason) {
137153
return new BusinessException("jdbc.driver.uploadFailed", new Object[]{reason});
138154
}

‎chat2db-community-server/chat2db-community-domain/chat2db-community-domain-core/src/test/java/ai/chat2db/community/domain/core/impl/db/JdbcDriverManagementPolicyTest.java‎

Lines changed: 76 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,16 @@
44
import org.junit.jupiter.api.Test;
55
import org.junit.jupiter.api.io.TempDir;
66

7+
import java.io.IOException;
78
import java.nio.file.Files;
89
import java.nio.file.FileSystemException;
10+
import java.nio.file.LinkOption;
911
import java.nio.file.Path;
1012
import java.util.List;
1113
import java.util.concurrent.CountDownLatch;
1214
import java.util.concurrent.Executors;
1315

16+
import static org.junit.jupiter.api.Assumptions.assumeTrue;
1417
import static org.junit.jupiter.api.Assertions.assertEquals;
1518
import static org.junit.jupiter.api.Assertions.assertFalse;
1619
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
@@ -71,9 +74,46 @@ void rejectsMissingMalformedAndEmptyTokens() throws Exception {
7174
assertUploadFailure(List.of("../outside.jar"), stagingDirectory, driverDirectory);
7275
assertUploadFailure(List.of("0123456789abcdef0123456789abcdef:driver.txt"),
7376
stagingDirectory, driverDirectory);
77+
BusinessException traversalFailure = assertUploadFailure(
78+
List.of("0123456789abcdef0123456789abcdef:driver..jar"),
79+
stagingDirectory, driverDirectory);
80+
assertEquals("invalid driver upload token", traversalFailure.getArgs()[0]);
7481
assertUploadFailure(List.of(), stagingDirectory, driverDirectory);
7582
}
7683

84+
@Test
85+
void rejectsCommaDelimitedDriverNameEvenWhenTheStagedFileExists() throws Exception {
86+
Path stagingDirectory = Files.createDirectories(tempDirectory.resolve("staging"));
87+
Path driverDirectory = Files.createDirectories(tempDirectory.resolve("drivers"));
88+
String uploadId = "0123456789abcdef0123456789abcdef";
89+
Files.writeString(stagingDirectory.resolve(uploadId + ".upload"), "driver");
90+
91+
BusinessException exception = assertThrows(BusinessException.class,
92+
() -> JdbcDriverManagementPolicy.promoteUploadedDrivers(
93+
List.of(uploadId + ":first,second.jar"), stagingDirectory, driverDirectory));
94+
95+
assertEquals("jdbc.driver.uploadFailed", exception.getCode());
96+
assertEquals("invalid driver upload token", exception.getArgs()[0]);
97+
assertFalse(Files.exists(driverDirectory.resolve("first,second.jar")));
98+
}
99+
100+
@Test
101+
void rejectsSymlinkedStagedUploadWithoutPublishingItsTarget() throws Exception {
102+
assumeSymbolicLinksAreAvailable();
103+
Path stagingDirectory = Files.createDirectories(tempDirectory.resolve("staging"));
104+
Path driverDirectory = Files.createDirectories(tempDirectory.resolve("drivers"));
105+
Path outsideFile = Files.writeString(tempDirectory.resolve("outside.jar"), "outside");
106+
String uploadId = "0123456789abcdef0123456789abcdef";
107+
Path stagedFile = stagingDirectory.resolve(uploadId + ".upload");
108+
Files.createSymbolicLink(stagedFile, outsideFile);
109+
110+
assertUploadFailure(List.of(uploadId + ":mysql.jar"), stagingDirectory, driverDirectory);
111+
112+
assertEquals("outside", Files.readString(outsideFile));
113+
assertFalse(Files.exists(stagedFile, LinkOption.NOFOLLOW_LINKS));
114+
assertFalse(Files.exists(driverDirectory.resolve("mysql.jar"), LinkOption.NOFOLLOW_LINKS));
115+
}
116+
77117
@Test
78118
void concurrentSameNamePromotionsNeverOverwriteTheWinner() throws Exception {
79119
Path stagingDirectory = Files.createDirectories(tempDirectory.resolve("staging"));
@@ -104,19 +144,35 @@ void concurrentSameNamePromotionsNeverOverwriteTheWinner() throws Exception {
104144
}
105145

106146
@Test
107-
void fallsBackToCreateNewCopyWhenHardLinksAreUnavailable() throws Exception {
147+
void copiesRegularFileAndRemovesTheStagingEntry() throws Exception {
108148
Path source = tempDirectory.resolve("staged.upload");
109149
Path target = tempDirectory.resolve("driver.jar");
110150
Files.writeString(source, "driver");
111151

112-
JdbcDriverManagementPolicy.moveWithoutReplacement(source, target, (link, existing) -> {
113-
throw new FileSystemException(link.toString(), existing.toString(), "hard links unavailable");
114-
});
152+
JdbcDriverManagementPolicy.moveWithoutReplacement(source, target);
115153

116154
assertEquals("driver", Files.readString(target));
117155
assertFalse(Files.exists(source));
118156
}
119157

158+
@Test
159+
void rejectsSymlinkSubstitutedBetweenValidationAndPublication() throws Exception {
160+
assumeSymbolicLinksAreAvailable();
161+
Path source = tempDirectory.resolve("staged.upload");
162+
Path target = tempDirectory.resolve("driver.jar");
163+
Path outsideFile = Files.writeString(tempDirectory.resolve("outside.jar"), "outside");
164+
Files.writeString(source, "driver");
165+
assertTrue(Files.isRegularFile(source, LinkOption.NOFOLLOW_LINKS));
166+
Files.delete(source);
167+
Files.createSymbolicLink(source, outsideFile);
168+
169+
assertThrows(IOException.class,
170+
() -> JdbcDriverManagementPolicy.moveWithoutReplacement(source, target));
171+
172+
assertEquals("outside", Files.readString(outsideFile));
173+
assertFalse(Files.exists(target, LinkOption.NOFOLLOW_LINKS));
174+
}
175+
120176
private Object promoteAfter(CountDownLatch start, String uploadId, Path stagingDirectory,
121177
Path driverDirectory) throws Exception {
122178
start.await();
@@ -128,10 +184,25 @@ private Object promoteAfter(CountDownLatch start, String uploadId, Path stagingD
128184
}
129185
}
130186

131-
private void assertUploadFailure(List<String> uploadTokens, Path stagingDirectory, Path driverDirectory) {
187+
private BusinessException assertUploadFailure(List<String> uploadTokens, Path stagingDirectory,
188+
Path driverDirectory) {
132189
BusinessException exception = assertThrows(BusinessException.class,
133190
() -> JdbcDriverManagementPolicy.promoteUploadedDrivers(
134191
uploadTokens, stagingDirectory, driverDirectory));
135192
assertEquals("jdbc.driver.uploadFailed", exception.getCode());
193+
return exception;
194+
}
195+
196+
private void assumeSymbolicLinksAreAvailable() throws Exception {
197+
Path target = Files.writeString(tempDirectory.resolve("symlink-probe-target"), "probe");
198+
Path link = tempDirectory.resolve("symlink-probe");
199+
try {
200+
Files.createSymbolicLink(link, target);
201+
} catch (UnsupportedOperationException | FileSystemException exception) {
202+
assumeTrue(false, "Symbolic links are unavailable: " + exception.getMessage());
203+
} finally {
204+
Files.deleteIfExists(link);
205+
Files.deleteIfExists(target);
206+
}
136207
}
137208
}

‎chat2db-community-server/chat2db-community-web/src/main/java/ai/chat2db/community/web/api/adapter/db/JdbcDriverUploadSizeFilter.java‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,9 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
3737

3838
static boolean isDriverUploadPath(String requestUri, String contextPath) {
3939
String normalizedContextPath = contextPath == null ? "" : contextPath;
40-
return requestUri != null && requestUri.equals(normalizedContextPath + DRIVER_UPLOAD_PATH);
40+
return requestUri != null
41+
&& stripPathParameters(requestUri).equals(stripPathParameters(normalizedContextPath)
42+
+ DRIVER_UPLOAD_PATH);
4143
}
4244

4345
static boolean isDriverUploadRequest(String method, String requestUri, String contextPath) {
@@ -47,4 +49,21 @@ static boolean isDriverUploadRequest(String method, String requestUri, String co
4749
static boolean isAllowedContentLength(long contentLength) {
4850
return contentLength >= 0 && contentLength <= MAX_DRIVER_UPLOAD_REQUEST_BYTES;
4951
}
52+
53+
private static String stripPathParameters(String path) {
54+
StringBuilder stripped = new StringBuilder(path.length());
55+
boolean inPathParameters = false;
56+
for (int index = 0; index < path.length(); index++) {
57+
char character = path.charAt(index);
58+
if (character == ';') {
59+
inPathParameters = true;
60+
} else if (character == '/') {
61+
inPathParameters = false;
62+
stripped.append(character);
63+
} else if (!inPathParameters) {
64+
stripped.append(character);
65+
}
66+
}
67+
return stripped.toString();
68+
}
5069
}

‎chat2db-community-server/chat2db-community-web/src/main/java/ai/chat2db/community/web/api/adapter/db/MultipartJdbcDriverUploadAdapter.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ static List<String> store(MultipartFile[] files, Path stagingDirectory, long max
6262
for (MultipartFile file : files) {
6363
String originalFilename = FilenameUtils.getName(file.getOriginalFilename());
6464
if (file.isEmpty() || originalFilename == null || originalFilename.isBlank()
65+
|| originalFilename.indexOf(',') >= 0
6566
|| !"jar".equalsIgnoreCase(FilenameUtils.getExtension(originalFilename))) {
6667
throw new IOException("Only non-empty JDBC driver JAR files can be uploaded");
6768
}

‎chat2db-community-server/chat2db-community-web/src/test/java/ai/chat2db/community/web/api/adapter/db/JdbcDriverUploadSizeFilterTest.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,14 @@ void matchesOnlyTheDriverUploadEndpoint() {
1313
"POST", "/api/jdbc/driver/upload", ""));
1414
assertTrue(JdbcDriverUploadSizeFilter.isDriverUploadRequest(
1515
"POST", "/chat2db/api/jdbc/driver/upload", "/chat2db"));
16+
assertTrue(JdbcDriverUploadSizeFilter.isDriverUploadRequest(
17+
"POST", "/api/jdbc/driver/upload;x=y", ""));
18+
assertTrue(JdbcDriverUploadSizeFilter.isDriverUploadRequest(
19+
"POST", "/chat2db/api;v=1/jdbc/driver/upload;x=y", "/chat2db"));
1620
assertFalse(JdbcDriverUploadSizeFilter.isDriverUploadRequest(
1721
"POST", "/api/converter/upload", ""));
22+
assertFalse(JdbcDriverUploadSizeFilter.isDriverUploadRequest(
23+
"POST", "/api/jdbc/driver/upload;x=y/extra", ""));
1824
assertFalse(JdbcDriverUploadSizeFilter.isDriverUploadRequest(
1925
"OPTIONS", "/api/jdbc/driver/upload", ""));
2026
}

‎chat2db-community-server/chat2db-community-web/src/test/java/ai/chat2db/community/web/api/adapter/db/MultipartJdbcDriverUploadAdapterTest.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ void rejectsEmptyNullNameNonJarAndOversizedUploadsWithoutResidue() throws Except
4040
assertThrows(IOException.class,
4141
() -> MultipartJdbcDriverUploadAdapter.store(
4242
new MultipartFile[]{file("driver.txt", "not a jar")}, driverDirectory));
43+
assertThrows(IOException.class,
44+
() -> MultipartJdbcDriverUploadAdapter.store(
45+
new MultipartFile[]{file("first,second.jar", "driver")}, driverDirectory));
4346
assertThrows(IOException.class,
4447
() -> MultipartJdbcDriverUploadAdapter.store(
4548
new MultipartFile[]{file(null, "driver")}, driverDirectory));

0 commit comments

Comments
 (0)