From 4a1cad7b950cf74a0b2e7a948a688abfb9634738 Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Wed, 27 May 2026 13:51:58 +0200 Subject: [PATCH 1/2] harden snapshot names --- .../config/CassandraRelevantProperties.java | 5 + .../cassandra/db/ColumnFamilyStore.java | 39 +++++++ .../org/apache/cassandra/db/SnapshotTest.java | 103 +++++++++++++++++- 3 files changed, 145 insertions(+), 2 deletions(-) diff --git a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java index 19d89f1e48a4..7bf72a3fb465 100644 --- a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java +++ b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java @@ -210,6 +210,11 @@ public enum CassandraRelevantProperties /** This property indicates whether disable_mbean_registration is true */ IS_DISABLED_MBEAN_REGISTRATION("org.apache.cassandra.disable_mbean_registration"), + /** + * Validates names of to-be-taken snapshots, defaults to false. + */ + SNAPSHOT_NAME_VALIDATION("cassandra.snapshot.validation", "false"), + /** what class to use for mbean registeration */ MBEAN_REGISTRATION_CLASS("org.apache.cassandra.mbean_registration_class"), diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index 571b86e90843..e358594f84d7 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -93,6 +93,8 @@ import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.lang.String.format; +import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH; import static org.apache.cassandra.utils.Throwables.maybeFail; import static org.apache.cassandra.utils.Throwables.merge; @@ -1837,6 +1839,8 @@ public void snapshotWithoutFlush(String snapshotName) */ public Set snapshotWithoutFlush(String snapshotName, Predicate predicate, boolean ephemeral, RateLimiter rateLimiter) { + validateSnapshotName(snapshotName); + if (rateLimiter == null) rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter(); @@ -1868,6 +1872,41 @@ public Set snapshotWithoutFlush(String snapshotName, Predicate SchemaConstants.FILENAME_LENGTH) + { + throw new IllegalArgumentException(format("Snapshot name must not be more than %d characters long for " + + "resolved snapshot name (got %d characters for \"%s\")", + FILENAME_LENGTH, snapshotName.length(), snapshotName)); + } + + // Allowed characters follow the AWS S3 "Safe characters" set documented at + // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines : + // 0-9 a-z A-Z ! - _ . * ' ( ) + // The path separator '/' is intentionally excluded, + // which is what blocks traversal attempts such as "../../mysnapshot" + if (!Pattern.compile("[a-zA-Z0-9!_.*'()-]+").matcher(snapshotName).matches()) + { + throw new IllegalArgumentException("Snapshot name contains illegal characters: " + snapshotName); + } + } + private void writeSnapshotManifest(final JSONArray filesJSONArr, final String snapshotName) { final File manifestFile = getDirectories().getSnapshotManifestFile(snapshotName); diff --git a/test/unit/org/apache/cassandra/db/SnapshotTest.java b/test/unit/org/apache/cassandra/db/SnapshotTest.java index a40604873996..9fdd0ae12065 100644 --- a/test/unit/org/apache/cassandra/db/SnapshotTest.java +++ b/test/unit/org/apache/cassandra/db/SnapshotTest.java @@ -22,19 +22,31 @@ import java.nio.file.Files; import java.nio.file.StandardOpenOption; +import org.junit.Before; import org.junit.Test; +import org.apache.cassandra.config.CassandraRelevantProperties; import org.apache.cassandra.cql3.CQLTester; +import org.apache.cassandra.distributed.shared.WithProperties; import org.apache.cassandra.io.sstable.Component; import org.apache.cassandra.io.sstable.format.SSTableReader; +import org.apache.cassandra.schema.SchemaConstants; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class SnapshotTest extends CQLTester { - @Test - public void testEmptyTOC() throws Throwable + @Before + public void setUpTable() throws Throwable { createTable("create table %s (id int primary key, k int)"); execute("insert into %s (id, k) values (1,1)"); + } + + @Test + public void testEmptyTOC() throws Throwable + { getCurrentColumnFamilyStore().forceBlockingFlush(); for (SSTableReader sstable : getCurrentColumnFamilyStore().getLiveSSTables()) { @@ -43,4 +55,91 @@ public void testEmptyTOC() throws Throwable } getCurrentColumnFamilyStore().snapshot("hello"); } + + @Test + public void testSnapshotNameValidation() + { + ColumnFamilyStore cfs = getCurrentColumnFamilyStore(); + String sep = File.separator; + + try (WithProperties p = new WithProperties()) + { + p.set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, true); + + // Previously-allowed alphanumerics, '-' and '_' must still be accepted. + assertThatCode(() -> cfs.snapshot("atag")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("a-tag")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("a_tag")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("a_tag_1and_something2-more")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot(repeat('a', SchemaConstants.FILENAME_LENGTH))).doesNotThrowAnyException(); + + // AWS S3 "Safe characters" accepted by the relaxed allowlist: + // ! . * ' ( ) + // See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines + assertThatCode(() -> cfs.snapshot("snap.2026-05-20")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("important!")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("backup*")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("o'snap")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("snap(1)")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("!._-*'()")).doesNotThrowAnyException(); + // Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory. + assertThatCode(() -> cfs.snapshot("a..tag")).doesNotThrowAnyException(); + + String tooLong = repeat('a', SchemaConstants.FILENAME_LENGTH + 1); + assertThatThrownBy(() -> cfs.snapshot(tooLong)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name must not be more than 255 characters long for " + + "resolved snapshot name (got 256 characters for \"" + tooLong + "\")"); + + // '/' is not in the S3-safe set; this is what kills traversal attempts like "../../mysnapshot". + assertThatThrownBy(() -> cfs.snapshot("a" + sep + "tag")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name cannot contain " + sep); + + // Other characters outside the S3-safe set must still be rejected. + assertThatThrownBy(() -> cfs.snapshot("a tag")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: a tag"); + assertThatThrownBy(() -> cfs.snapshot("a:tag")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: a:tag"); + + // "." and ".." pass the charset check but resolve to the snapshots/ dir itself + // and its parent (the live table dir) respectively, so they must be rejected as reserved. + assertThatThrownBy(() -> cfs.snapshot(".")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '.' is reserved"); + assertThatThrownBy(() -> cfs.snapshot("..")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '..' is reserved"); + } + + try (WithProperties p = new WithProperties()) + { + p.set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false); + + // Previously-rejected characters are now accepted: space, ':', and other non-S3-safe chars. + assertThatCode(() -> cfs.snapshot("a tag")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("a:tag")).doesNotThrowAnyException(); + + // Path separator and "." / ".." rejections are unconditional — they guard against + // traversal regardless of the toggle. + assertThatThrownBy(() -> cfs.snapshot("a" + sep + "tag")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name cannot contain " + sep); + assertThatThrownBy(() -> cfs.snapshot(".")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '.' is reserved"); + assertThatThrownBy(() -> cfs.snapshot("..")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '..' is reserved"); + } + } + + private static String repeat(char c, int times) + { + char[] chars = new char[times]; + java.util.Arrays.fill(chars, c); + return new String(chars); + } } From 057c705e433f8c4d1b68811da4f7910e4ecd3938 Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Fri, 29 May 2026 11:33:56 +0200 Subject: [PATCH 2/2] further restrict safe set --- .../cassandra/db/ColumnFamilyStore.java | 11 ++++--- .../org/apache/cassandra/db/SnapshotTest.java | 32 ++++++++++++------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index e358594f84d7..e62ba4e02cdb 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -1896,12 +1896,13 @@ private void validateSnapshotName(String snapshotName) FILENAME_LENGTH, snapshotName.length(), snapshotName)); } - // Allowed characters follow the AWS S3 "Safe characters" set documented at - // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines : - // 0-9 a-z A-Z ! - _ . * ' ( ) - // The path separator '/' is intentionally excluded, + // Allowed characters are a conservative subset of the AWS S3 "Safe characters" set + // (https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines): + // 0-9 a-z A-Z - _ . + // The remaining S3-safe characters (! * ' ( )) are intentionally excluded as they are + // shell-significant and error-prone in paths, and the path separator '/' is excluded too, // which is what blocks traversal attempts such as "../../mysnapshot" - if (!Pattern.compile("[a-zA-Z0-9!_.*'()-]+").matcher(snapshotName).matches()) + if (!Pattern.compile("[a-zA-Z0-9_.-]+").matcher(snapshotName).matches()) { throw new IllegalArgumentException("Snapshot name contains illegal characters: " + snapshotName); } diff --git a/test/unit/org/apache/cassandra/db/SnapshotTest.java b/test/unit/org/apache/cassandra/db/SnapshotTest.java index 9fdd0ae12065..16202fe8a90c 100644 --- a/test/unit/org/apache/cassandra/db/SnapshotTest.java +++ b/test/unit/org/apache/cassandra/db/SnapshotTest.java @@ -73,15 +73,8 @@ public void testSnapshotNameValidation() assertThatCode(() -> cfs.snapshot("a_tag_1and_something2-more")).doesNotThrowAnyException(); assertThatCode(() -> cfs.snapshot(repeat('a', SchemaConstants.FILENAME_LENGTH))).doesNotThrowAnyException(); - // AWS S3 "Safe characters" accepted by the relaxed allowlist: - // ! . * ' ( ) - // See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines + // Only alphanumerics, '-', '_' and '.' are accepted. assertThatCode(() -> cfs.snapshot("snap.2026-05-20")).doesNotThrowAnyException(); - assertThatCode(() -> cfs.snapshot("important!")).doesNotThrowAnyException(); - assertThatCode(() -> cfs.snapshot("backup*")).doesNotThrowAnyException(); - assertThatCode(() -> cfs.snapshot("o'snap")).doesNotThrowAnyException(); - assertThatCode(() -> cfs.snapshot("snap(1)")).doesNotThrowAnyException(); - assertThatCode(() -> cfs.snapshot("!._-*'()")).doesNotThrowAnyException(); // Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory. assertThatCode(() -> cfs.snapshot("a..tag")).doesNotThrowAnyException(); @@ -91,12 +84,26 @@ public void testSnapshotNameValidation() .hasMessage("Snapshot name must not be more than 255 characters long for " + "resolved snapshot name (got 256 characters for \"" + tooLong + "\")"); - // '/' is not in the S3-safe set; this is what kills traversal attempts like "../../mysnapshot". + // '/' is not in the allowed set; this is what kills traversal attempts like "../../mysnapshot". assertThatThrownBy(() -> cfs.snapshot("a" + sep + "tag")) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Snapshot name cannot contain " + sep); - // Other characters outside the S3-safe set must still be rejected. + // The shell-significant S3 "safe" characters (! * ' ( )) are deliberately NOT allowed. + assertThatThrownBy(() -> cfs.snapshot("important!")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: important!"); + assertThatThrownBy(() -> cfs.snapshot("backup*")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: backup*"); + assertThatThrownBy(() -> cfs.snapshot("o'snap")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: o'snap"); + assertThatThrownBy(() -> cfs.snapshot("snap(1)")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: snap(1)"); + + // Other characters outside the allowed set must still be rejected. assertThatThrownBy(() -> cfs.snapshot("a tag")) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Snapshot name contains illegal characters: a tag"); @@ -118,9 +125,12 @@ public void testSnapshotNameValidation() { p.set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false); - // Previously-rejected characters are now accepted: space, ':', and other non-S3-safe chars. + // The character check is bypassed entirely: space, ':', and the now-disallowed + // shell-significant characters (! * ' ( )) are all accepted. assertThatCode(() -> cfs.snapshot("a tag")).doesNotThrowAnyException(); assertThatCode(() -> cfs.snapshot("a:tag")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("important!")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("snap(1)")).doesNotThrowAnyException(); // Path separator and "." / ".." rejections are unconditional — they guard against // traversal regardless of the toggle.