Skip to content

Commit a67f1c2

Browse files
aaronziaaronzi
committed
fix(security): block critical CWE-22 path traversal in file uploads
Mitigate unauthenticated arbitrary file write / potential RCE vector in upload flows. - enforce normalized path resolution and storage-boundary checks before file writes - reject traversal attempts and invalid path input with HTTP 400 - decouple logical filename from physical storage via UUID-based keys - preserve original client filename as metadata and expose it in download headers - add malicious-payload regression tests for service and repository HTTP APIs - replay PoC payloads and verify no marker file is created outside storage boundary
1 parent 4848749 commit a67f1c2

14 files changed

Lines changed: 259 additions & 30 deletions

File tree

‎basyx.common/basyx.filerepository-backend-inmemory/src/main/java/org/eclipse/digitaltwin/basyx/core/filerepository/InMemoryFileRepository.java‎

Lines changed: 89 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,10 @@
3232
import java.io.InputStream;
3333
import java.nio.file.Files;
3434
import java.nio.file.InvalidPathException;
35+
import java.nio.file.Path;
3536
import java.nio.file.Paths;
37+
import java.util.Map;
38+
import java.util.concurrent.ConcurrentHashMap;
3639

3740
import org.apache.commons.io.IOUtils;
3841
import org.eclipse.digitaltwin.basyx.core.exceptions.FileDoesNotExistException;
@@ -50,13 +53,18 @@
5053
public class InMemoryFileRepository implements FileRepository {
5154

5255
private static final String TEMP_DIRECTORY_PREFIX = "basyx-temp";
53-
private String tmpDirectory = getTemporaryDirectoryPath();
56+
private final Path tmpDirectoryPath = Paths.get(getTemporaryDirectoryPath()).toAbsolutePath().normalize();
57+
private final Map<String, String> originalFileNames = new ConcurrentHashMap<>();
5458

5559
@Override
5660
public String save(FileMetadata fileMetadata) throws FileHandlingException {
57-
String filePath = createFilePath(fileMetadata.getFileName());
61+
Path targetPath = createFilePath(fileMetadata.getFileName());
62+
String filePath = targetPath.toString();
5863

59-
java.io.File targetFile = new java.io.File(filePath);
64+
if (Files.exists(targetPath))
65+
throw new FileHandlingException("File '%s' already exists.".formatted(filePath));
66+
67+
java.io.File targetFile = targetPath.toFile();
6068

6169
try (FileOutputStream outStream = new FileOutputStream(targetFile)) {
6270
IOUtils.copy(fileMetadata.getFileContent(), outStream);
@@ -65,50 +73,82 @@ public String save(FileMetadata fileMetadata) throws FileHandlingException {
6573
}
6674

6775
fileMetadata.setFileName(filePath);
76+
String originalFileName = fileMetadata.getOriginalFileName();
77+
78+
if (originalFileName == null || originalFileName.isBlank())
79+
originalFileName = targetPath.getFileName().toString();
80+
81+
originalFileNames.put(filePath, originalFileName);
6882

6983
return filePath;
7084
}
7185

7286
@Override
7387
public InputStream find(String fileId) throws FileDoesNotExistException {
88+
Path filePath;
7489

7590
try {
76-
return new FileInputStream(fileId);
91+
filePath = resolveStoredFilePath(fileId);
92+
} catch (IllegalArgumentException | SecurityException e) {
93+
throw new FileDoesNotExistException();
94+
}
95+
96+
try {
97+
return new FileInputStream(filePath.toFile());
7798
} catch (FileNotFoundException e) {
7899
throw new FileDoesNotExistException();
79100
}
80101
}
81102

82103
@Override
83104
public void delete(String fileId) throws FileDoesNotExistException {
105+
Path filePath;
84106

85-
if (!exists(fileId))
107+
try {
108+
filePath = resolveStoredFilePath(fileId);
109+
} catch (IllegalArgumentException | SecurityException e) {
86110
throw new FileDoesNotExistException();
111+
}
87112

88-
java.io.File tmpFile = new java.io.File(fileId);
113+
if (!Files.exists(filePath))
114+
throw new FileDoesNotExistException();
115+
116+
java.io.File tmpFile = filePath.toFile();
89117

90118
tmpFile.delete();
119+
originalFileNames.remove(filePath.toString());
91120
}
92121

93122
@Override
94123
public boolean exists(String fileId) {
95124
if(fileId == null) return false;
96125

97-
if (fileId.isBlank() || !isFilePathValid(fileId))
126+
if (fileId.isBlank())
127+
return false;
128+
129+
Path filePath;
130+
131+
try {
132+
filePath = resolveStoredFilePath(fileId);
133+
} catch (IllegalArgumentException | SecurityException e) {
98134
return false;
135+
}
99136

100-
return Files.exists(Paths.get(fileId));
137+
return Files.exists(filePath);
101138
}
102139

103-
private boolean isFilePathValid(String filePath) {
140+
@Override
141+
public String getOriginalFileName(String fileId) {
142+
Path filePath;
104143

105144
try {
106-
Paths.get(filePath);
107-
} catch (InvalidPathException | NullPointerException ex) {
108-
return false;
145+
filePath = resolveStoredFilePath(fileId);
146+
} catch (IllegalArgumentException | SecurityException e) {
147+
return fileId;
109148
}
110149

111-
return true;
150+
String key = filePath.toString();
151+
return originalFileNames.getOrDefault(key, filePath.getFileName().toString());
112152
}
113153

114154
private String getTemporaryDirectoryPath() {
@@ -123,8 +163,42 @@ private String getTemporaryDirectoryPath() {
123163
return tempDirectoryPath;
124164
}
125165

126-
private String createFilePath(String fileName) {
127-
return tmpDirectory + "/" + fileName;
166+
private Path createFilePath(String fileName) {
167+
if (fileName == null || fileName.isBlank())
168+
throw new IllegalArgumentException("File name must not be null or blank.");
169+
170+
Path resolvedPath;
171+
172+
try {
173+
resolvedPath = tmpDirectoryPath.resolve(fileName).normalize();
174+
} catch (InvalidPathException e) {
175+
throw new IllegalArgumentException("Invalid file name '%s'.".formatted(fileName), e);
176+
}
177+
178+
if (!resolvedPath.startsWith(tmpDirectoryPath))
179+
throw new SecurityException("Path traversal attempt detected.");
180+
181+
return resolvedPath;
182+
}
183+
184+
private Path resolveStoredFilePath(String fileId) {
185+
Path filePath;
186+
187+
try {
188+
filePath = Paths.get(fileId);
189+
} catch (InvalidPathException e) {
190+
throw new IllegalArgumentException("Invalid file id '%s'.".formatted(fileId), e);
191+
}
192+
193+
if (!filePath.isAbsolute())
194+
filePath = tmpDirectoryPath.resolve(filePath);
195+
196+
filePath = filePath.normalize();
197+
198+
if (!filePath.startsWith(tmpDirectoryPath))
199+
throw new SecurityException("Path traversal attempt detected.");
200+
201+
return filePath;
128202
}
129203

130204
}

‎basyx.common/basyx.filerepository-backend-inmemory/src/test/java/org/eclipse/digitaltwin/basyx/core/filerepository/TestInMemoryFileRepository.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,11 @@
2424
******************************************************************************/
2525
package org.eclipse.digitaltwin.basyx.core.filerepository;
2626

27+
import java.io.ByteArrayInputStream;
28+
import java.nio.file.Paths;
29+
2730
import org.eclipse.digitaltwin.basyx.core.filerepository.backend.FileRepositoryTestSuite;
31+
import org.junit.Test;
2832

2933
/**
3034
* Integration Test for InMemoryFileRepository
@@ -38,4 +42,21 @@ public class TestInMemoryFileRepository extends FileRepositoryTestSuite{
3842
protected FileRepository getFileRepository() {
3943
return new InMemoryFileRepository();
4044
}
45+
46+
@Test(expected = SecurityException.class)
47+
public void saveFileRejectsPathTraversal() {
48+
FileRepository fileRepository = getFileRepository();
49+
FileMetadata metadata = new FileMetadata("../../../../tmp/basyx-pwned.txt", "text/plain", new ByteArrayInputStream("test".getBytes()));
50+
51+
fileRepository.save(metadata);
52+
}
53+
54+
@Test(expected = SecurityException.class)
55+
public void saveFileRejectsAbsolutePath() {
56+
FileRepository fileRepository = getFileRepository();
57+
String absolutePath = Paths.get(System.getProperty("java.io.tmpdir"), "basyx-pwned.txt").toString();
58+
FileMetadata metadata = new FileMetadata(absolutePath, "text/plain", new ByteArrayInputStream("test".getBytes()));
59+
60+
fileRepository.save(metadata);
61+
}
4162
}

‎basyx.common/basyx.filerepository-backend-mongodb/src/main/java/org/eclipse/digitaltwin/basyx/core/filerepository/MongoDBFileRepository.java‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import org.springframework.data.mongodb.core.query.Query;
3838
import org.springframework.data.mongodb.gridfs.GridFsTemplate;
3939
import org.springframework.stereotype.Component;
40+
import org.bson.Document;
4041

4142
import com.mongodb.client.gridfs.model.GridFSFile;
4243

@@ -50,6 +51,7 @@
5051
public class MongoDBFileRepository implements FileRepository {
5152

5253
private static final String MONGO_FILENAME_FIELD = "filename";
54+
private static final String MONGO_METADATA_ORIGINAL_FILENAME_FIELD = "originalFileName";
5355

5456
private GridFsTemplate gridFsTemplate;
5557

@@ -72,7 +74,10 @@ public String save(FileMetadata fileMetadata) throws FileHandlingException {
7274
if (exists(fileMetadata.getFileName()))
7375
throw new FileHandlingException("File '%s' already exists.".formatted(fileMetadata.getFileName()));
7476

75-
gridFsTemplate.store(fileMetadata.getFileContent(), fileMetadata.getFileName(), fileMetadata.getContentType());
77+
Document metadata = new Document();
78+
metadata.put(MONGO_METADATA_ORIGINAL_FILENAME_FIELD, fileMetadata.getOriginalFileName());
79+
80+
gridFsTemplate.store(fileMetadata.getFileContent(), fileMetadata.getFileName(), fileMetadata.getContentType(), metadata);
7681

7782
return fileMetadata.getFileName();
7883
}
@@ -110,6 +115,21 @@ public boolean exists(String fileName) {
110115
return gridFSFile != null;
111116
}
112117

118+
@Override
119+
public String getOriginalFileName(String fileId) {
120+
GridFSFile file = getFile(fileId);
121+
122+
if (file == null || file.getMetadata() == null)
123+
return fileId;
124+
125+
String originalFileName = file.getMetadata().getString(MONGO_METADATA_ORIGINAL_FILENAME_FIELD);
126+
127+
if (originalFileName == null || originalFileName.isBlank())
128+
return fileId;
129+
130+
return originalFileName;
131+
}
132+
113133
private GridFSFile getFile(String mongoDBfileId) {
114134
return gridFsTemplate.findOne(new Query(Criteria.where(MONGO_FILENAME_FIELD).is(mongoDBfileId)));
115135
}

‎basyx.common/basyx.filerepository-backend/src/main/java/org/eclipse/digitaltwin/basyx/core/filerepository/FileMetadata.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,18 @@
3838
public class FileMetadata {
3939

4040
private String fileName;
41+
private String originalFileName;
4142
private String contentType;
4243
private InputStream fileContent;
4344

4445
public FileMetadata(String fileName, String contentType, InputStream fileContent) {
46+
this(fileName, fileName, contentType, fileContent);
47+
}
48+
49+
public FileMetadata(String fileName, String originalFileName, String contentType, InputStream fileContent) {
4550
super();
4651
this.fileName = fileName;
52+
this.originalFileName = originalFileName;
4753
this.contentType = contentType;
4854
this.fileContent = fileContent;
4955
}
@@ -56,6 +62,14 @@ public String getFileName() {
5662
return fileName;
5763
}
5864

65+
public void setOriginalFileName(String originalFileName) {
66+
this.originalFileName = originalFileName;
67+
}
68+
69+
public String getOriginalFileName() {
70+
return originalFileName;
71+
}
72+
5973
public void setContentType(String contentType) {
6074
this.contentType = contentType;
6175
}

‎basyx.common/basyx.filerepository-backend/src/main/java/org/eclipse/digitaltwin/basyx/core/filerepository/FileRepository.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,4 +72,14 @@ public interface FileRepository {
7272
*/
7373
public boolean exists(String fileId);
7474

75+
/**
76+
* Resolves the original logical filename that should be exposed to clients.
77+
*
78+
* @param fileId
79+
* @return original filename or a fallback implementation-specific value
80+
*/
81+
public default String getOriginalFileName(String fileId) {
82+
return fileId;
83+
}
84+
7585
}

‎basyx.common/basyx.http/src/main/java/org/eclipse/digitaltwin/basyx/http/BaSyxExceptionHandler.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,11 @@ public ResponseEntity<Object> handleCollidingIdentifierException(CollidingAssetL
8888
public ResponseEntity<Object> handleIllegalArgumentException(IllegalArgumentException exception) {
8989
return buildResponse(exception.getMessage(), HttpStatus.BAD_REQUEST, exception);
9090
}
91+
92+
@ExceptionHandler(SecurityException.class)
93+
public ResponseEntity<Object> handleSecurityException(SecurityException exception) {
94+
return buildResponse(exception.getMessage(), HttpStatus.BAD_REQUEST, exception);
95+
}
9196

9297
@ExceptionHandler(IdentificationMismatchException.class)
9398
public ResponseEntity<Object> handleIdMismatchException(IdentificationMismatchException exception) {

‎basyx.submodelrepository/basyx.submodelrepository-backend/src/main/java/org/eclipse/digitaltwin/basyx/submodelrepository/backend/CrudSubmodelRepository.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,11 @@ public InputStream getFileByFilePath(String submodelId, String filePath) {
207207
return getService(submodelId).getFileByFilePath(filePath);
208208
}
209209

210+
@Override
211+
public String getOriginalFileNameByPath(String submodelId, String idShortPath) {
212+
return getService(submodelId).getOriginalFileNameByPath(idShortPath);
213+
}
214+
210215
private void initializeRemoteCollection(@NonNull Collection<Submodel> submodels) {
211216
if (submodels.isEmpty())
212217
return;

‎basyx.submodelrepository/basyx.submodelrepository-core/src/main/java/org/eclipse/digitaltwin/basyx/submodelrepository/SubmodelRepository.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,4 +297,17 @@ public default String getName() {
297297
* @return File InputStream
298298
*/
299299
public InputStream getFileByFilePath(String submodelId, String filePath);
300+
301+
/**
302+
* Resolves the original logical filename for a file submodel element.
303+
*
304+
* @param submodelId
305+
* the Submodel id
306+
* @param idShortPath
307+
* the IdShort path of the file element
308+
* @return original filename to expose to clients
309+
*/
310+
public default String getOriginalFileNameByPath(String submodelId, String idShortPath) {
311+
return getFileByPathSubmodel(submodelId, idShortPath).getName();
312+
}
300313
}

‎basyx.submodelrepository/basyx.submodelrepository-http/src/main/java/org/eclipse/digitaltwin/basyx/submodelrepository/http/SubmodelRepositoryApiHTTPController.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,9 +230,10 @@ public ResponseEntity<Submodel> getSubmodelByIdMetadata(Base64UrlEncodedIdentifi
230230

231231
@Override
232232
public ResponseEntity<Resource> getFileByPath(Base64UrlEncodedIdentifier submodelIdentifier, String idShortPath) {
233-
Resource resource = new FileSystemResource(repository.getFileByPathSubmodel(submodelIdentifier.getIdentifier(), idShortPath));
233+
java.io.File file = repository.getFileByPathSubmodel(submodelIdentifier.getIdentifier(), idShortPath);
234+
Resource resource = new FileSystemResource(file);
234235

235-
String fileName = resource.getFilename();
236+
String fileName = repository.getOriginalFileNameByPath(submodelIdentifier.getIdentifier(), idShortPath);
236237

237238
return ResponseEntity.ok()
238239
.header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + fileName + "\"")

‎basyx.submodelrepository/basyx.submodelrepository-http/src/test/java/org/eclipse/digitaltwin/basyx/submodelrepository/http/SubmodelRepositorySubmodelHTTPTestSuite.java‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,14 @@ public void uploadFileToNotExistElement() throws FileNotFoundException, Unsuppor
339339
assertEquals(HttpStatus.NOT_FOUND.value(), submodelElementFileUploadResponse.getCode());
340340
}
341341

342+
@Test
343+
public void uploadFileWithPathTraversalFileName() throws IOException {
344+
String maliciousFileName = "..%2F..%2F..%2F..%2Ftmp%2Fbasyx-pwned.txt";
345+
CloseableHttpResponse submodelElementFileUploadResponse = uploadFileToSubmodelElement(DummySubmodelFactory.SUBMODEL_FOR_FILE_TEST, DummySubmodelFactory.SUBMODEL_ELEMENT_FILE_ID_SHORT, maliciousFileName);
346+
347+
assertEquals(HttpStatus.BAD_REQUEST.value(), submodelElementFileUploadResponse.getCode());
348+
}
349+
342350
@Test
343351
public void deleteFile() throws FileNotFoundException, IOException {
344352
uploadFileToSubmodelElement(DummySubmodelFactory.SUBMODEL_FOR_FILE_TEST, DummySubmodelFactory.SUBMODEL_ELEMENT_FILE_ID_SHORT);
@@ -369,7 +377,7 @@ public void deleteFileFromNotExistElement() throws FileNotFoundException, Unsupp
369377
@Test
370378
public void getFile() throws FileNotFoundException, IOException, ParseException {
371379
String fileName = DummySubmodelFactory.FILE_NAME;
372-
String expectedFileName = Base64UrlEncodedIdentifier.encodeIdentifier(DummySubmodelFactory.SUBMODEL_FOR_FILE_TEST) + "-" + DummySubmodelFactory.SUBMODEL_ELEMENT_FILE_ID_SHORT + "-" + fileName;
380+
String expectedFileName = fileName;
373381

374382
byte[] expectedFile = readBytesFromClasspath(fileName);
375383

@@ -445,11 +453,13 @@ private String createSMEFileGetURL(String submodelId, String submodelElementIdSh
445453
}
446454

447455
private CloseableHttpResponse uploadFileToSubmodelElement(String submodelId, String submodelElementIdShort) throws IOException {
448-
CloseableHttpClient client = HttpClients.createDefault();
449-
450456
String fileName = DummySubmodelFactory.FILE_NAME;
457+
return uploadFileToSubmodelElement(submodelId, submodelElementIdShort, fileName);
458+
}
451459

452-
java.io.File file = ResourceUtils.getFile("classpath:" + fileName);
460+
private CloseableHttpResponse uploadFileToSubmodelElement(String submodelId, String submodelElementIdShort, String fileName) throws IOException {
461+
CloseableHttpClient client = HttpClients.createDefault();
462+
java.io.File file = ResourceUtils.getFile("classpath:" + DummySubmodelFactory.FILE_NAME);
453463

454464
HttpPut putRequest = createPutRequestWithFile(submodelId, submodelElementIdShort, fileName, file);
455465

0 commit comments

Comments
 (0)