Skip to content

Commit 4b7a59f

Browse files
gaulgaul
committed
Prevent parent path traversal in filesystem-nio2
Reported-by: Nico Waisman <[email protected]>
1 parent e7d12ca commit 4b7a59f

File tree

2 files changed

+111
-6
lines changed

2 files changed

+111
-6
lines changed

‎src/main/java/org/gaul/s3proxy/nio2blob/AbstractNio2BlobStore.java

+28-6
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,11 @@ public final PageSet<? extends StorageMetadata> list(String container,
183183
} else {
184184
prefix = "";
185185
}
186-
var pathPrefix = root.resolve(container).resolve(prefix);
186+
var pathPrefix = root.resolve(container).resolve(prefix).normalize();
187+
if (!pathPrefix.startsWith(container)) {
188+
throw new IllegalArgumentException("Invalid key name: path traversal attempt detected.");
189+
}
190+
logger.debug("Listing blobs at: {}", pathPrefix);
187191
var set = ImmutableSortedSet.<StorageMetadata>naturalOrder();
188192
try {
189193
listHelper(set, container, dirPrefix, pathPrefix, delimiter);
@@ -344,7 +348,10 @@ public final Blob getBlob(String container, String key, GetOptions options) {
344348
throw new ContainerNotFoundException(container, "");
345349
}
346350

347-
var path = root.resolve(container).resolve(key);
351+
var path = root.resolve(container).resolve(key).normalize();
352+
if (!path.startsWith(container)) {
353+
throw new IllegalArgumentException("Invalid key name: path traversal attempt detected.");
354+
}
348355
logger.debug("Getting blob at: {}", path);
349356

350357
try {
@@ -528,7 +535,10 @@ public final String putBlob(String container, Blob blob, PutOptions options) {
528535
throw new ContainerNotFoundException(container, "");
529536
}
530537

531-
var path = root.resolve(container).resolve(blob.getMetadata().getName());
538+
var path = root.resolve(container).resolve(blob.getMetadata().getName()).normalize();
539+
if (!path.startsWith(container)) {
540+
throw new IllegalArgumentException("Invalid key name: path traversal attempt detected.");
541+
}
532542
// TODO: should we use a known suffix to filter these out during list?
533543
var tmpPath = root.resolve(container).resolve(blob.getMetadata().getName() + "-" + UUID.randomUUID());
534544
logger.debug("Creating blob at: {}", path);
@@ -693,7 +703,11 @@ public final String copyBlob(String fromContainer, String fromName,
693703
public final void removeBlob(String container, String key) {
694704
try {
695705
var containerPath = root.resolve(container);
696-
var path = containerPath.resolve(key);
706+
var path = containerPath.resolve(key).normalize();
707+
if (!path.startsWith(container)) {
708+
throw new IllegalArgumentException("Invalid key name: path traversal attempt detected.");
709+
}
710+
logger.debug("Deleting blob at: {}", path);
697711
Files.delete(path);
698712
removeEmptyParentDirectories(containerPath, path.getParent());
699713
} catch (NoSuchFileException nsfe) {
@@ -771,7 +785,11 @@ public final BlobAccess getBlobAccess(String container, String key) {
771785
throw new KeyNotFoundException(container, key, "");
772786
}
773787

774-
var path = root.resolve(container).resolve(key);
788+
var path = root.resolve(container).resolve(key).normalize();
789+
if (!path.startsWith(container)) {
790+
throw new IllegalArgumentException("Invalid key name: path traversal attempt detected.");
791+
}
792+
775793
Set<PosixFilePermission> permissions;
776794
try {
777795
permissions = Files.getPosixFilePermissions(path);
@@ -791,7 +809,11 @@ public final void setBlobAccess(String container, String key, BlobAccess access)
791809
throw new KeyNotFoundException(container, key, "");
792810
}
793811

794-
var path = root.resolve(container).resolve(key);
812+
var path = root.resolve(container).resolve(key).normalize();
813+
if (!path.startsWith(container)) {
814+
throw new IllegalArgumentException("Invalid key name: path traversal attempt detected.");
815+
}
816+
795817
Set<PosixFilePermission> permissions;
796818
try {
797819
permissions = new HashSet<>(Files.getPosixFilePermissions(path));

‎src/test/java/org/gaul/s3proxy/AwsSdkTest.java

+83
Original file line numberDiff line numberDiff line change
@@ -1717,6 +1717,89 @@ public Map.Entry<String, BlobStore> locateBlobStore(
17171717
}
17181718
}
17191719

1720+
@Test
1721+
public void testCopyRelativePath() throws Exception {
1722+
try {
1723+
client.copyObject(new CopyObjectRequest(
1724+
containerName, "../evil.txt", containerName, "good.txt"));
1725+
Fail.failBecauseExceptionWasNotThrown(AmazonS3Exception.class);
1726+
} catch (AmazonS3Exception e) {
1727+
if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
1728+
assertThat(e.getErrorCode()).isEqualTo("400 Bad Request");
1729+
} else {
1730+
assertThat(e.getErrorCode()).isEqualTo("NoSuchKey");
1731+
}
1732+
}
1733+
}
1734+
1735+
@Test
1736+
public void testDeleteRelativePath() throws Exception {
1737+
try {
1738+
client.deleteObject(containerName, "../evil.txt");
1739+
if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
1740+
Fail.failBecauseExceptionWasNotThrown(AmazonS3Exception.class);
1741+
}
1742+
} catch (AmazonS3Exception e) {
1743+
if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
1744+
assertThat(e.getErrorCode()).isEqualTo("400 Bad Request");
1745+
} else {
1746+
assertThat(e.getErrorCode()).isEqualTo("NoSuchKey");
1747+
}
1748+
}
1749+
}
1750+
1751+
@Test
1752+
public void testGetRelativePath() throws Exception {
1753+
try {
1754+
client.getObject(containerName, "../evil.txt");
1755+
Fail.failBecauseExceptionWasNotThrown(AmazonS3Exception.class);
1756+
} catch (AmazonS3Exception e) {
1757+
if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
1758+
assertThat(e.getErrorCode()).isEqualTo("400 Bad Request");
1759+
} else {
1760+
assertThat(e.getErrorCode()).isEqualTo("NoSuchKey");
1761+
}
1762+
}
1763+
}
1764+
1765+
@Test
1766+
public void testPutRelativePath() throws Exception {
1767+
try {
1768+
var metadata = new ObjectMetadata();
1769+
metadata.setContentLength(BYTE_SOURCE.size());
1770+
PutObjectResult result = client.putObject(containerName, "../evil.txt",
1771+
BYTE_SOURCE.openStream(), metadata);
1772+
if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
1773+
Fail.failBecauseExceptionWasNotThrown(AmazonS3Exception.class);
1774+
}
1775+
} catch (AmazonS3Exception e) {
1776+
if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
1777+
assertThat(e.getErrorCode()).isEqualTo("400 Bad Request");
1778+
} else {
1779+
assertThat(e.getErrorCode()).isEqualTo("NoSuchKey");
1780+
}
1781+
}
1782+
}
1783+
1784+
@Test
1785+
public void testListRelativePath() throws Exception {
1786+
assumeTrue(!blobStoreType.equals("filesystem"));
1787+
try {
1788+
client.listObjects(new ListObjectsRequest()
1789+
.withBucketName(containerName)
1790+
.withPrefix("../evil/"));
1791+
if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
1792+
Fail.failBecauseExceptionWasNotThrown(AmazonS3Exception.class);
1793+
}
1794+
} catch (AmazonS3Exception e) {
1795+
if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
1796+
assertThat(e.getErrorCode()).isEqualTo("400 Bad Request");
1797+
} else {
1798+
throw e;
1799+
}
1800+
}
1801+
}
1802+
17201803
private static final class NullX509TrustManager
17211804
implements X509TrustManager {
17221805
@Override

0 commit comments

Comments
 (0)