Skip to content

Commit

Permalink
sanitize id paths via base64 encoding (#5740)
Browse files Browse the repository at this point in the history
- supports relaxed restrictions on entity names
- prevents malicious path injection
  • Loading branch information
arifogel committed Apr 30, 2020
1 parent 9a7a5a4 commit 9fdf408
Show file tree
Hide file tree
Showing 5 changed files with 157 additions and 10 deletions.
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package org.batfish.identifiers;

import static java.util.Optional.ofNullable;
import static org.batfish.storage.FileBasedStorage.fromBase64;
import static org.batfish.storage.FileBasedStorage.toBase64;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
Expand All @@ -17,6 +20,7 @@
import javax.annotation.Nullable;
import org.batfish.common.BatfishException;
import org.batfish.common.util.CommonUtil;
import org.batfish.storage.FileBasedStorage;
import org.batfish.storage.FileBasedStorageDirectoryProvider;

/**
Expand Down Expand Up @@ -46,15 +50,24 @@ public class FileBasedIdResolver implements IdResolver {
return Hashing.murmur3_128().hashString(input, StandardCharsets.UTF_8).toString();
}

private static @Nonnull Set<String> listResolvableNames(Path idsDir) {
@VisibleForTesting
static @Nonnull Set<String> listResolvableNames(Path idsDir) {
if (!Files.exists(idsDir)) {
return ImmutableSet.of();
}
try (Stream<Path> files = Files.list(idsDir)) {
return files
.filter(path -> path.toString().endsWith(ID_EXTENSION))
.filter(
path -> {
try {
return fromBase64(path.getFileName().toString()).endsWith(ID_EXTENSION);
} catch (IllegalArgumentException e) {
return false;
}
})
.map(Path::getFileName)
.map(Path::toString)
.map(FileBasedStorage::fromBase64)
.map(
nameWithExtension ->
nameWithExtension.substring(
Expand All @@ -81,7 +94,8 @@ public FileBasedIdResolver(Path storageBase) {
}

protected @Nonnull Path getAnalysisIdPath(String analysis, NetworkId networkId) {
return getAnalysisIdsDir(networkId).resolve(String.format("%s%s", analysis, ID_EXTENSION));
return getAnalysisIdsDir(networkId)
.resolve(toBase64(String.format("%s%s", analysis, ID_EXTENSION)));
}

protected @Nonnull Path getAnalysisIdsDir(NetworkId networkId) {
Expand Down Expand Up @@ -134,7 +148,7 @@ public FileBasedIdResolver(Path storageBase) {

protected @Nonnull Path getIssueSettingsIdPath(String majorIssueType, NetworkId networkId) {
return getIssueSettingsIdsDir(networkId)
.resolve(String.format("%s%s", majorIssueType, ID_EXTENSION));
.resolve(toBase64(String.format("%s%s", majorIssueType, ID_EXTENSION)));
}

protected @Nonnull Path getIssueSettingsIdsDir(NetworkId networkId) {
Expand All @@ -151,7 +165,7 @@ public FileBasedIdResolver(Path storageBase) {
}

protected @Nonnull Path getNetworkIdPath(String network) {
return getNetworkIdsDir().resolve(String.format("%s%s", network, ID_EXTENSION));
return getNetworkIdsDir().resolve(toBase64(String.format("%s%s", network, ID_EXTENSION)));
}

protected @Nonnull Path getNetworkIdsDir() {
Expand Down Expand Up @@ -183,7 +197,7 @@ public NodeRolesId getNetworkNodeRolesId(NetworkId networkId) {
protected @Nonnull Path getQuestionIdPath(
String question, NetworkId networkId, @Nullable AnalysisId analysisId) {
return getQuestionIdsDir(networkId, analysisId)
.resolve(String.format("%s%s", question, ID_EXTENSION));
.resolve(toBase64(String.format("%s%s", question, ID_EXTENSION)));
}

protected @Nonnull Path getQuestionIdsDir(NetworkId networkId, @Nullable AnalysisId analysisId) {
Expand All @@ -209,7 +223,7 @@ public NodeRolesId getNetworkNodeRolesId(NetworkId networkId) {

protected @Nonnull Path getQuestionSettingsIdPath(String questionClassId, NetworkId networkId) {
return getQuestionSettingsIdsDir(networkId)
.resolve(String.format("%s%s", questionClassId, ID_EXTENSION));
.resolve(toBase64(String.format("%s%s", questionClassId, ID_EXTENSION)));
}

protected @Nonnull Path getQuestionSettingsIdsDir(NetworkId networkId) {
Expand All @@ -226,7 +240,8 @@ public NodeRolesId getNetworkNodeRolesId(NetworkId networkId) {
}

protected @Nonnull Path getSnapshotIdPath(String snapshot, NetworkId networkId) {
return getSnapshotIdsDir(networkId).resolve(String.format("%s%s", snapshot, ID_EXTENSION));
return getSnapshotIdsDir(networkId)
.resolve(toBase64(String.format("%s%s", snapshot, ID_EXTENSION)));
}

protected @Nonnull Path getSnapshotIdsDir(NetworkId networkId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -841,11 +841,11 @@ public void deleteNetworkObject(NetworkId networkId, String key)
return _d.getSnapshotObjectsDir(networkId, snapshotId).resolve(encodedKey);
}

private static @Nonnull String toBase64(String key) {
public static @Nonnull String toBase64(String key) {
return Base64.getUrlEncoder().encodeToString(key.getBytes(StandardCharsets.UTF_8));
}

private static @Nonnull String fromBase64(String key) {
public static @Nonnull String fromBase64(String key) {
return new String(Base64.getUrlDecoder().decode(key), StandardCharsets.UTF_8);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
load("@rules_java//java:defs.bzl", "java_library")

package(
default_testonly = True,
default_visibility = ["//visibility:public"],
)

load("@batfish//skylark:junit.bzl", "junit_tests")
load("@batfish//skylark:pmd_test.bzl", "pmd_test")

java_library(
name = "identifiers",
srcs = glob(
["*.java"],
exclude = ["*Test.java"],
),
deps = [
"//projects/batfish-common-protocol:common",
"@maven//:com_google_code_findbugs_jsr305",
],
)

pmd_test(
name = "pmd",
lib = ":identifiers",
)

junit_tests(
name = "tests",
srcs = glob(
["*.java"],
exclude = ["Test*.java"],

),
deps = [
"//projects/batfish-common-protocol:common",
"@maven//:com_fasterxml_jackson_core_jackson_core",
"@maven//:com_google_code_findbugs_jsr305",
"@maven//:com_google_guava_guava",
"@maven//:junit_junit",
"@maven//:org_hamcrest_hamcrest",
],
)


Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package org.batfish.identifiers;

import static org.batfish.identifiers.FileBasedIdResolver.listResolvableNames;
import static org.batfish.storage.FileBasedStorage.fromBase64;
import static org.batfish.storage.FileBasedStorage.toBase64;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThat;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;

/** Test of {@link FileBasedIdResolver}. */
public final class FileBasedIdResolverTest {

private FileBasedIdResolver _resolver;

@Rule public TemporaryFolder _folder = new TemporaryFolder();

@Before
public void setup() {
_resolver = new FileBasedIdResolver(_folder.getRoot().toPath());
}

private static void assertLastComponentBase64Encoded(Path idPath) {
assertNotNull(fromBase64(idPath.getFileName().toString()));
}

@Test
public void testListResolvableNames() throws IOException {
Path root = _folder.getRoot().toPath();
Files.createFile(root.resolve(toBase64("foo.id")));
Files.createFile(root.resolve(toBase64("bar.id")));
Files.createFile(root.resolve("baz.id")); // should be excluded because it is not base64-encoded
Files.createFile(
root.resolve(toBase64("bath"))); // should be excluded because it doesn't end in ID

assertThat(listResolvableNames(root), containsInAnyOrder("foo", "bar"));
}

@Test
public void testGetAnalysisIdPath() {
assertLastComponentBase64Encoded(
_resolver.getAnalysisIdPath("analysis1", new NetworkId("net1_id")));
}

@Test
public void testGetIssueSettingsIdPath() {
assertLastComponentBase64Encoded(
_resolver.getIssueSettingsIdPath("majorIssueType1", new NetworkId("net1_id")));
}

@Test
public void testGetNetworkIdPath() {
assertLastComponentBase64Encoded(_resolver.getNetworkIdPath("net1"));
}

@Test
public void testGetQuestionIdPath() {
// with analysis
assertLastComponentBase64Encoded(
_resolver.getQuestionIdPath(
"question1", new NetworkId("net1_id"), new AnalysisId("analysis1_id")));

// without analysis
assertLastComponentBase64Encoded(
_resolver.getQuestionIdPath("question1", new NetworkId("net1_id"), null));
}

@Test
public void testGetQuestionSettingsIdPath() {
assertLastComponentBase64Encoded(
_resolver.getQuestionSettingsIdPath("questionClassId1", new NetworkId("net1_id")));
}

@Test
public void testGetSnapshotIdPath() {
assertLastComponentBase64Encoded(
_resolver.getSnapshotIdPath("snapshot1", new NetworkId("net1_id")));
}
}
1 change: 1 addition & 0 deletions projects/batfish/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ junit_tests(
"//projects/batfish-common-protocol/src/test/java/org/batfish/common/bdd:matchers",
"//projects/batfish-common-protocol/src/test/java/org/batfish/common/matchers",
"//projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/matchers",
"//projects/batfish-common-protocol/src/test/java/org/batfish/identifiers",
"//projects/bdd",
"//projects/symbolic",
"@maven//:com_google_code_findbugs_jsr305",
Expand Down

0 comments on commit 9fdf408

Please sign in to comment.