Skip to content

Commit 3c2de25

Browse files
cppwfsspring-builds
authored andcommitted
GH-11095: Prevent file writing outside output directory
- Throw exception if '..' is used within the canonical directory. - Also prevent the absolute directory paths. - Add tests to verify directory traversal prevention. - Removed the NOSONAR's from class - Utilize checkFile for file creation in: - FileWriteingMessageHandler - AbstractRemoteFileOutboundGateway - Add additional test for multiple file navigation - Add additional test for isAbsolute filepath - Apply changes from code review Rename checkFile to newFileInDirectoryIfValid - Move newFileInDirectoryIfValid to FileUtils Refactor file validation and clean up comments - Remove try-catch blocks since checked exceptions are removed. - Delete Sonar suppression comments to maintain clean code. - Remove informational comments from tests. Refactor file validation and add traversal tests - Simplify path resolution in FileUtils. - Throw InvalidPathException instead of MessagingException. - Add directory traversal tests to FileWritingMessageHandlerTests. - Add directory traversal tests to FtpTests. - AbstractRemoteFileOutboundGateway does not have tests so, FTPTests were used as a proxy - FTP tests showed where a space can be inserted in front of a directory and still be accepted. This was remediated by stripping blanks in front of the fileName - Update the traversal validation code to use the canonical calls to identify changes It handles the navigation for all character variations, where the homegrown only checked for `..`. newFileInDirectoryIfValid must return un altered File Any File returned (after inspection) must contain the original filename Refactor absolute path validation and tests - Allow absolute paths in file validation when a directory is provided. - Fix FileUtils to reject absolute paths only if directory is empty. - Revert resultFile creation to avoid unintended path validation. - Refactor synchronizer tests to use Mockito for simpler session mocking. - Update FTP and synchronizer tests to align with new validation rules. Reject absolute directory for file sync - Absolute directory in file sync is reject regardles if parent directory is present. Move return in FileUtils to outside of try block - Add NOSONAR flags back to code - Remove unnecessary afterPropertiesSet and setBeanFactory from test - Resolve checkstyle error Refactor file validation for empty directories - Resolve absolute paths properly when target directories are empty. - Remove leading whitespace stripping to maintain strict file paths. - Add tests to verify path traversal logic for empty local directories. resultFile in the handleRequestMessage must be validated - Security scanner recommended that resultFile be validated for traversal. - tempfile also needs to be validated because it can be used instead of result file in AIL, IGNORE, REPLACE, REPLACE_IF_MODIFIED cases. Refactor file validation and update tests Refactor FileUtils to validate paths by comparing canonical and absolute paths instead of checking if they start with the directory path. This simplifies the validation logic and removes the need for the ABSOLUTE_DIR constant. - Remove redundant tests from AbstractRemoteFileSynchronizerTests - Add OS-specific directory traversal tests These tests still fail because the filter routine considers absolute navigation a security issue - Update FileWritingMessageHandler to use standard File instantiation This is based on our discussion that we only needed to test one of the entries not both - Refactor helper methods to be static - Set java.io.tmpdir to tmp dir when on macOS Java tmpdir defaults to /var however on macOS this is a symlink to /private/var Thus any test that tests for improper traversal of directories or uses code that inspects for improper traversal will fail because of the symlink due to how java implements its canonical resolution - Atomically replace the destination file with the temporary file, ensuring data integrity by overwriting any existing version in a single, indivisible operation. Resolves Issue: There is a Time-of-Check to Time-of-Use race condition between the canonical path check in FileUtils.newFileInDirectoryIfValid and the directory creation/file writing. A local attacker can replace a valid directory with a symlink during this window, leading to an Arbitrary File Overwrite. Consolidate test temp dir config and update tests - Move Mac OS X test temp dir config to root to reduce duplication. - Split checkPathOnAbsolute FTP test for OS-specific validation. - Add comment in FileWritingMessageHandler to clarify path checks. Move java_tmp prop set for mac to configure method - Remove blank lines left over from previous commit - Move property set to common configuration section * Some code cleanup and Windows compatibility for tests Fixes: #11095 (cherry picked from commit 2614c19)
1 parent 03c7897 commit 3c2de25

8 files changed

Lines changed: 227 additions & 10 deletions

File tree

build.gradle

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,11 @@ configure(javaProjects) { subproject ->
325325

326326
jvmArgs '-Xshare:off',
327327
'--enable-native-access=ALL-UNNAMED'
328+
329+
if (org.gradle.internal.os.OperatingSystem.current().isMacOsX()) {
330+
systemProperty 'java.io.tmpdir', '/private/tmp'
331+
}
332+
328333
}
329334

330335
checkstyle {

spring-integration-file/src/main/java/org/springframework/integration/file/outbound/FileWritingMessageHandler.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -505,7 +505,9 @@ public boolean isRunning() {
505505

506506
File destinationDirectoryToUse = evaluateDestinationDirectoryExpression(requestMessage);
507507

508-
File tempFile = new File(destinationDirectoryToUse, generatedFileName + this.temporaryFileSuffix);
508+
File tempFile = FileUtils.newFileInDirectoryIfValid(destinationDirectoryToUse,
509+
generatedFileName + this.temporaryFileSuffix);
510+
// Validation above prevents Path Traversal for the line below; no redundant check needed.
509511
File resultFile = new File(destinationDirectoryToUse, generatedFileName);
510512

511513
boolean exists = resultFile.exists();

spring-integration-file/src/main/java/org/springframework/integration/file/remote/gateway/AbstractRemoteFileOutboundGateway.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
import org.springframework.integration.file.remote.session.Session;
5353
import org.springframework.integration.file.remote.session.SessionFactory;
5454
import org.springframework.integration.file.support.FileExistsMode;
55+
import org.springframework.integration.file.support.FileUtils;
5556
import org.springframework.integration.handler.AbstractReplyProducingMessageHandler;
5657
import org.springframework.integration.handler.ExpressionEvaluatingMessageProcessor;
5758
import org.springframework.integration.handler.MessageProcessor;
@@ -1170,8 +1171,9 @@ protected void purgeDots(List<F> lsFiles) {
11701171
}
11711172
fileInfo = files[0];
11721173
}
1173-
final File localFile =
1174-
new File(generateLocalDirectory(message, remoteDir), generateLocalFileName(message, remoteFilename));
1174+
File localFile =
1175+
FileUtils.newFileInDirectoryIfValid(generateLocalDirectory(message, remoteDir),
1176+
generateLocalFileName(message, remoteFilename));
11751177
FileExistsMode existsMode = resolveFileExistsMode(message);
11761178
boolean appending = FileExistsMode.APPEND.equals(existsMode);
11771179
boolean exists = localFile.exists();

spring-integration-file/src/main/java/org/springframework/integration/file/remote/synchronizer/AbstractInboundFileSynchronizer.java

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
import java.io.OutputStream;
2525
import java.net.URI;
2626
import java.net.URISyntaxException;
27+
import java.nio.file.Files;
28+
import java.nio.file.StandardCopyOption;
2729
import java.util.ArrayList;
2830
import java.util.Arrays;
2931
import java.util.Collections;
@@ -81,6 +83,7 @@
8183
* @author Gary Russell
8284
* @author Artem Bilan
8385
* @author Ngoc Nhan
86+
* @author Glenn Renfro
8487
*
8588
* @since 2.0
8689
*/
@@ -524,7 +527,7 @@ protected boolean copyFileToLocalDirectory(@Nullable String remoteDirectoryPath,
524527

525528
long modified = getModified(remoteFile);
526529

527-
File localFile = new File(localDirectory, localFileName);
530+
File localFile = FileUtils.newFileInDirectoryIfValid(localDirectory, localFileName);
528531
boolean exists = localFile.exists();
529532
if (!exists || (this.preserveTimestamp && modified != localFile.lastModified())) {
530533
if (!exists &&
@@ -619,11 +622,25 @@ private boolean copyRemoteContentToLocalFile(Session<F> session, String remoteFi
619622
+ "' from the remote to the local directory", e);
620623
}
621624

622-
renamed = tempFile.renameTo(localFile);
625+
try {
626+
Files.move(tempFile.toPath(), localFile.toPath(), StandardCopyOption.ATOMIC_MOVE,
627+
StandardCopyOption.REPLACE_EXISTING);
628+
renamed = true;
629+
}
630+
catch (IOException e) {
631+
renamed = false;
632+
}
623633

624634
if (!renamed) {
625635
if (localFile.delete()) {
626-
renamed = tempFile.renameTo(localFile);
636+
try {
637+
Files.move(tempFile.toPath(), localFile.toPath(), StandardCopyOption.ATOMIC_MOVE,
638+
StandardCopyOption.REPLACE_EXISTING);
639+
renamed = true;
640+
}
641+
catch (IOException e) {
642+
renamed = false;
643+
}
627644
if (!renamed && this.logger.isInfoEnabled()) {
628645
this.logger.info("Cannot rename '"
629646
+ tempFileName

spring-integration-file/src/main/java/org/springframework/integration/file/support/FileUtils.java

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,12 @@
1616

1717
package org.springframework.integration.file.support;
1818

19+
import java.io.File;
20+
import java.io.IOException;
21+
import java.io.UncheckedIOException;
1922
import java.lang.reflect.Array;
2023
import java.nio.file.FileSystems;
24+
import java.nio.file.InvalidPathException;
2125
import java.util.Arrays;
2226
import java.util.Comparator;
2327
import java.util.function.Predicate;
@@ -70,6 +74,37 @@ public static <F> F[] purgeUnwantedElements(F[] fileArray, Predicate<? extends F
7074
}
7175
}
7276

77+
/**
78+
* Create a new file instance representing the specified file within the given base directory,
79+
* ensuring that the resulting file remains within the intended target directory.
80+
* Prevent Path Traversal vulnerabilities by explicitly rejecting file names that are absolute
81+
* paths or that resolve to canonical paths outside the provided base directory.
82+
* @param directory The base directory where the file is intended to be placed.
83+
* @param fileName The name of the file or the relative path to be resolved against the base directory.
84+
* @return A {@link File} object representing the securely resolved destination path.
85+
* @throws InvalidPathException If the {@code fileName} is an absolute path or attempts to traverse
86+
* outside the {@code directory}.
87+
* @throws UncheckedIOException If an I/O error occurs while resolving the canonical paths.
88+
* @since 5.5.21
89+
*/
90+
public static File newFileInDirectoryIfValid(File directory, String fileName) {
91+
if (new File(fileName).isAbsolute()) {
92+
throw new InvalidPathException(fileName,
93+
"The file is trying to leave the target output directory of " + directory);
94+
}
95+
try {
96+
File file = new File(directory.getCanonicalFile(), fileName);
97+
if (!file.getCanonicalPath().equals(file.getAbsolutePath())) {
98+
throw new InvalidPathException(fileName,
99+
"The file is trying to leave the target output directory of " + directory);
100+
}
101+
return file;
102+
}
103+
catch (IOException ex) {
104+
throw new UncheckedIOException(ex);
105+
}
106+
}
107+
73108
private FileUtils() {
74109
}
75110

spring-integration-file/src/test/java/org/springframework/integration/file/outbound/FileWritingMessageHandlerTests.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import java.io.IOException;
2525
import java.io.InputStream;
2626
import java.nio.file.Files;
27+
import java.nio.file.InvalidPathException;
2728
import java.nio.file.attribute.PosixFilePermission;
2829
import java.util.Map;
2930
import java.util.Set;
@@ -46,6 +47,7 @@
4647
import org.springframework.expression.Expression;
4748
import org.springframework.integration.channel.NullChannel;
4849
import org.springframework.integration.channel.QueueChannel;
50+
import org.springframework.integration.file.DefaultFileNameGenerator;
4951
import org.springframework.integration.file.FileHeaders;
5052
import org.springframework.integration.file.support.FileExistsMode;
5153
import org.springframework.integration.file.support.FileUtils;
@@ -54,6 +56,7 @@
5456
import org.springframework.integration.test.util.TestUtils;
5557
import org.springframework.messaging.Message;
5658
import org.springframework.messaging.MessageHandlingException;
59+
import org.springframework.messaging.MessagingException;
5760
import org.springframework.messaging.support.GenericMessage;
5861
import org.springframework.scheduling.concurrent.ThreadPoolTaskScheduler;
5962
import org.springframework.util.FileCopyUtils;
@@ -694,6 +697,19 @@ public void newFileCallback() throws Exception {
694697
assertFileContentIs(result, "foo" + System.lineSeparator() + "barbar");
695698
}
696699

700+
@Test
701+
public void checkPathThrowsOnDirectoryTraversal() {
702+
Message<?> message = MessageBuilder.withPayload("testdata").build();
703+
DefaultFileNameGenerator fileNameGenerator = new DefaultFileNameGenerator();
704+
fileNameGenerator.setExpression("'base/../../../escaped.txt'");
705+
handler.setFileNameGenerator(fileNameGenerator);
706+
707+
assertThatExceptionOfType(MessagingException.class)
708+
.isThrownBy(() -> handler.handleMessage(message))
709+
.withStackTraceContaining("trying to leave the target output directory")
710+
.withRootCauseInstanceOf(InvalidPathException.class);
711+
}
712+
697713
void assertFileContentIsMatching(Message<?> result) throws IOException {
698714
assertFileContentIs(result, SAMPLE_CONTENT);
699715
}

spring-integration-file/src/test/java/org/springframework/integration/file/remote/synchronizer/AbstractRemoteFileSynchronizerTests.java

Lines changed: 90 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.io.IOException;
2121
import java.io.InputStream;
2222
import java.io.OutputStream;
23+
import java.nio.file.InvalidPathException;
2324
import java.util.LinkedList;
2425
import java.util.List;
2526
import java.util.Map;
@@ -32,6 +33,8 @@
3233

3334
import org.apache.commons.io.FileUtils;
3435
import org.junit.jupiter.api.Test;
36+
import org.junit.jupiter.api.condition.EnabledOnOs;
37+
import org.junit.jupiter.api.condition.OS;
3538
import org.junit.jupiter.api.io.TempDir;
3639

3740
import org.springframework.expression.EvaluationContext;
@@ -48,6 +51,7 @@
4851
import static org.assertj.core.api.Assertions.assertThat;
4952
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
5053
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
54+
import static org.mockito.BDDMockito.given;
5155
import static org.mockito.Mockito.mock;
5256

5357
/**
@@ -298,11 +302,93 @@ protected String protocol() {
298302
assertThat(localDir.list()).contains("dir1", "dir2");
299303
}
300304

301-
private AbstractInboundFileSynchronizingMessageSource<String> createSource(AtomicInteger count) {
305+
@Test
306+
public void checkPathThrowsOnDirectoryTraversal(@TempDir File localDir) {
307+
verifyDirectoryNotTheSame(localDir, "../base/escaped.txt");
308+
}
309+
310+
@Test
311+
public void checkPathOnDirectoryMidLineTraversal(@TempDir File localDir) {
312+
verifyDirectoryNotTheSame(localDir, "base/../escaped.txt");
313+
}
314+
315+
private static AbstractInboundFileSynchronizer<String> getAbstractInboundFileSynchronizer(String fileName) {
316+
StringSession session = mock(StringSession.class);
317+
given(session.list("remoteDir")).willReturn(new String[] {fileName});
318+
given(session.getHostPort()).willReturn("mock:1234");
319+
320+
AbstractInboundFileSynchronizer<String> sync = createSynchronizerWithSession(session);
321+
sync.setRemoteDirectory("remoteDir");
322+
return sync;
323+
}
324+
325+
@Test
326+
public void checkPathThrowsOnDirectoryExtendMidLineTraversal(@TempDir File localDir) {
327+
verifyDirectoryNotTheSame(localDir, "base/subdir/../../../../escaped.txt");
328+
}
329+
330+
@Test
331+
@EnabledOnOs(OS.WINDOWS)
332+
void checkPathThrowsOnAbsoluteDirectoryTraversalLinuxMacWindows(@TempDir File localDir) {
333+
verifyDirectoryNotTheSame(localDir, "C:\\projects\\data\\file.txt");
334+
}
335+
336+
@Test
337+
@EnabledOnOs(OS.WINDOWS)
338+
void notTraversalOnCurrentDirWindows() {
339+
AbstractInboundFileSynchronizer<String> sync = getAbstractInboundFileSynchronizer("build\\tmp\\file.txt");
340+
341+
sync.synchronizeToLocalDirectory(new File(""));
342+
343+
assertThat(new File("build\\tmp\\file.txt")).exists();
344+
}
345+
346+
@Test
347+
@EnabledOnOs({OS.LINUX, OS.MAC})
348+
void checkPathThrowsOnAbsoluteDirectoryTraversalLinuxMac(@TempDir File localDir) {
349+
verifyDirectoryNotTheSame(localDir, "/base/escaped.txt");
350+
}
351+
352+
private static void verifyDirectoryNotTheSame(File localDir, String fileName) {
353+
AbstractInboundFileSynchronizer<String> sync = getAbstractInboundFileSynchronizer(fileName);
354+
355+
assertThatExceptionOfType(MessagingException.class)
356+
.isThrownBy(() -> sync.synchronizeToLocalDirectory(localDir))
357+
.withStackTraceContaining("trying to leave the target output directory")
358+
.withRootCauseInstanceOf(InvalidPathException.class);
359+
}
360+
361+
private static AbstractInboundFileSynchronizer<String> createSynchronizerWithSession(StringSession session) {
362+
return new AbstractInboundFileSynchronizer<>(() -> session) {
363+
364+
@Override
365+
protected boolean isFile(String file) {
366+
return true;
367+
}
368+
369+
@Override
370+
protected String getFilename(String file) {
371+
return file;
372+
}
373+
374+
@Override
375+
protected long getModified(String file) {
376+
return 0;
377+
}
378+
379+
@Override
380+
protected String protocol() {
381+
return "mock";
382+
}
383+
384+
};
385+
}
386+
387+
private static AbstractInboundFileSynchronizingMessageSource<String> createSource(AtomicInteger count) {
302388
return createSource(createLimitingSynchronizer(count));
303389
}
304390

305-
private AbstractInboundFileSynchronizingMessageSource<String> createSource(
391+
private static AbstractInboundFileSynchronizingMessageSource<String> createSource(
306392
AbstractInboundFileSynchronizer<String> sync) {
307393

308394
AbstractInboundFileSynchronizingMessageSource<String> source =
@@ -322,7 +408,7 @@ public String getComponentType() {
322408
return source;
323409
}
324410

325-
private AbstractInboundFileSynchronizer<String> createLimitingSynchronizer(final AtomicInteger count) {
411+
private static AbstractInboundFileSynchronizer<String> createLimitingSynchronizer(final AtomicInteger count) {
326412
SessionFactory<String> sf = new StringSessionFactory();
327413
AbstractInboundFileSynchronizer<String> sync = new AbstractInboundFileSynchronizer<>(sf) {
328414

@@ -453,7 +539,7 @@ public Object getClientInstance() {
453539

454540
@Override
455541
public String getHostPort() {
456-
return "mock:6666";
542+
return "mock:1234";
457543
}
458544

459545
}

0 commit comments

Comments
 (0)