Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

more validation for containerName and blobKey to avoid access escape #203

Merged
merged 3 commits into from
May 6, 2024
Merged

more validation for containerName and blobKey to avoid access escape #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b817c61
more validation for containerName and blobKey to avoid access escape
jixinchi Apr 3, 2024
0c88967
fix grammar error in sentence from contains to contain
jixinchi Apr 8, 2024
3e2da20
add unit test for Filesystem BlobKey and ContainerName validator
jixinchi Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...org/jclouds/filesystem/predicates/validators/internal/FilesystemBlobKeyValidatorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@

import com.google.inject.Singleton;

import java.io.File;
import java.util.Arrays;

/**
* Validates name for filesystem container blob keys implementation
*
Expand All @@ -38,6 +41,8 @@ public void validate(String name) throws IllegalArgumentException {
//blobkey cannot start with / (or \ in Windows) character
if (name.startsWith("\\") || name.startsWith("/"))
throw new IllegalArgumentException("Blob key '" + name + "' cannot start with \\ or /");
if (Arrays.asList(name.split(File.separator.equals("\\") ? "\\\\" : File.separator)).contains(".."))
throw new IllegalArgumentException("Blob key '" + name + "' cannot contain ../");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this different than the container name validator? For robustness should this tokenize the path via / then check each component to see if one contains . or ..? This would allow keys like ..foo to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ..foo should be allowed. I tried several object storage services, such as gcloud and aliyun oss, they all allow ..foo as object key.

}

}
2 changes: 2 additions & 0 deletions ...louds/filesystem/predicates/validators/internal/FilesystemContainerNameValidatorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ public void validate(String name) throws IllegalArgumentException {
//container name cannot contains / (or \ in Windows) character
if (name.contains("\\") || name.contains("/"))
throw new IllegalArgumentException("Container name '" + name + "' cannot contain \\ or /");
if (name.equals(".") || name.equals(".."))
throw new IllegalArgumentException("Container name cannot be . or ..");
}

}
9 changes: 9 additions & 0 deletions ...src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ public boolean createContainerInLocation(String container, Location location, Cr

@Override
public ContainerAccess getContainerAccess(String container) {
filesystemContainerNameValidator.validate(container);
File file = new File(buildPathStartingFromBaseDir(container));
if (!file.exists()) {
throw new ContainerNotFoundException(container, "in getContainerAccess");
Expand Down Expand Up @@ -217,6 +218,7 @@ public ContainerAccess getContainerAccess(String container) {

@Override
public void setContainerAccess(String container, ContainerAccess access) {
filesystemContainerNameValidator.validate(container);
Path path = new File(buildPathStartingFromBaseDir(container)).toPath();

if ( isWindows() ) {
Expand Down Expand Up @@ -310,6 +312,7 @@ else if (object.isDirectory() && (optsPrefix.endsWith(File.separator) || isNullO

@Override
public StorageMetadata getContainerMetadata(String container) {
filesystemContainerNameValidator.validate(container);
MutableStorageMetadata metadata = new MutableStorageMetadataImpl();
metadata.setName(container);
metadata.setType(StorageType.CONTAINER);
Expand Down Expand Up @@ -378,6 +381,8 @@ public String apply(String string) {

@Override
public Blob getBlob(final String container, final String key) {
filesystemContainerNameValidator.validate(container);
filesystemBlobKeyValidator.validate(key);
BlobBuilder builder = blobBuilders.get();
builder.name(key);
File file = getFileForBlobKey(container, key);
Expand Down Expand Up @@ -658,6 +663,8 @@ public void removeBlob(final String container, final String blobKey) {

@Override
public BlobAccess getBlobAccess(String containerName, String blobName) {
filesystemContainerNameValidator.validate(containerName);
filesystemBlobKeyValidator.validate(blobName);
if (!new File(buildPathStartingFromBaseDir(containerName)).exists()) {
throw new ContainerNotFoundException(containerName, "in getBlobAccess");
}
Expand Down Expand Up @@ -691,6 +698,8 @@ public BlobAccess getBlobAccess(String containerName, String blobName) {

@Override
public void setBlobAccess(String container, String name, BlobAccess access) {
filesystemContainerNameValidator.validate(container);
filesystemBlobKeyValidator.validate(name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also apply to getBlobAccess, putBlob, and removeBlob? getBlobKeysInsideContainer too I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods have been applied by the validators. I just added some missing ones.

Path path = new File(buildPathStartingFromBaseDir(container, name)).toPath();
if ( isWindows() ) {
try {
Expand Down
6 changes: 6 additions & 0 deletions ...org/jclouds/filesystem/predicates/validators/internal/FilesystemBlobKeyValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public void testNamesValidity() {

validator.validate("all.img");
validator.validate("all" + File.separator + "is" + File.separator + "" + "ok");
validator.validate("all" + File.separator + "is" + File.separator + ".." + "ok");
}

@Test
Expand All @@ -51,6 +52,11 @@ public void testInvalidNames() {
validator.validate(File.separator + "is" + File.separator + "" + "ok");
fail("Blob key value incorrect, but was not recognized");
} catch (IllegalArgumentException e) {}

try {
validator.validate("all" + File.separator + ".." + File.separator + "ok");
fail("Blob key value incorrect, but was not recognized");
} catch (IllegalArgumentException e) {}
}


Expand Down
10 changes: 10 additions & 0 deletions ...louds/filesystem/predicates/validators/internal/FilesystemContainerNameValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ public void testInvalidNames() {
validator.validate("all" + File.separator + "is" + File.separator);
fail("Container name value incorrect, but was not recognized");
} catch (IllegalArgumentException e) {}

try {
validator.validate(".");
fail("Container name value incorrect, but was not recognized");
} catch (IllegalArgumentException e) {}

try {
validator.validate("..");
fail("Container name value incorrect, but was not recognized");
} catch (IllegalArgumentException e) {}
}


Expand Down
Loading