From 1362ac6f69ebd2fed89466c85a036f721391e016 Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Wed, 27 May 2026 13:51:58 +0200 Subject: [PATCH] Validate snapshot names The validation of snapshot names is turned off by default. It is controlled by system property cassandra.snapshot.validation which is by default set to false except 7.0 branch. When the validation is turned on, the allowed characters are AWS safe characters together with "+" character. A user can not specify any path separators in snapshot names even if the validation as such is turned off. patch by Stefan Miklosovic; reviewed by Francisco Guerrero, Matt Byrd for CASSANDRA-21389 --- CHANGES.txt | 1 + .../config/CassandraRelevantProperties.java | 5 + .../cassandra/db/ColumnFamilyStore.java | 44 +++++++ .../org/apache/cassandra/db/SnapshotTest.java | 117 +++++++++++++++++- 4 files changed, 165 insertions(+), 2 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index c764952023..274e2dcecf 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.0.21 + * Validate snapshot names (CASSANDRA-21389) * BTree.FastBuilder.reset() fails to clear savedBuffer and savedNextKey, causing ClassCastException and SSTable header corruption during schema disagreement (CASSANDRA-21216, CASSANDRA-21260) * Backport CASSANDRA-17810 fix and improve RTBoundValidator error messages (CASSANDRA-18282) diff --git a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java index 19d89f1e48..7bf72a3fb4 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 571b86e908..4ff6e096e7 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -93,6 +93,8 @@ import org.json.simple.JSONObject; 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 class ColumnFamilyStore implements ColumnFamilyStoreMBean */ public Set snapshotWithoutFlush(String snapshotName, Predicate predicate, boolean ephemeral, RateLimiter rateLimiter) { + validateSnapshotName(snapshotName); + if (rateLimiter == null) rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter(); @@ -1868,6 +1872,46 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean return snapshottedSSTables; } + private void validateSnapshotName(String snapshotName) + { + if (snapshotName.contains(File.separator)) + { + throw new IllegalArgumentException("Snapshot name cannot contain " + File.separator); + } + + if (snapshotName.equals(".") || snapshotName.equals("..")) + { + throw new IllegalArgumentException("Snapshot name '" + snapshotName + "' is reserved"); + } + + if (!CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION.getBoolean()) + return; + + // the length of valid snapshot name has to be less than or equal to FILENAME_LENGTH - that is 255 - + // we are following the max length as it is in SchemaConstants for table name. + if (snapshotName.length() > 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 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 - _ . + // plus '+', which is not an S3 "Safe character" but can legitimately appear in system + // snapshot names via version build metadata (e.g. an upgrade snapshot "-upgrade-5.0.4+build-..."). + // 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()) + { + throw new IllegalArgumentException("Snapshot name contains illegal characters: " + snapshotName + ". " + + "Allowed characters must match the pattern: [a-zA-Z0-9_.+-]+" + + " with a maximum of length of " + FILENAME_LENGTH + " characters."); + } + } + 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 a406048739..544a131dec 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.io.File; 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,105 @@ public class SnapshotTest extends CQLTester } 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(); + + // Only alphanumerics, '-', '_' and '.' are accepted. + assertThatCode(() -> cfs.snapshot("snap.2026-05-20")).doesNotThrowAnyException(); + // Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory. + assertThatCode(() -> cfs.snapshot("a..tag")).doesNotThrowAnyException(); + + // "+" is part of the allowed set because it can appear in a Cassandra version + // (build metadata, e.g. "7.0.0+abc123"), which ends up in system snapshot names. + assertThatCode(() -> cfs.snapshot("this_is_snapshot-7.0.0+abc123")).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 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); + + // The shell-significant S3 "safe" characters (! * ' ( )) are deliberately NOT allowed. + assertThatThrownBy(() -> cfs.snapshot("important!")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Snapshot name contains illegal characters: important!"); + assertThatThrownBy(() -> cfs.snapshot("backup*")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Snapshot name contains illegal characters: backup*"); + assertThatThrownBy(() -> cfs.snapshot("o'snap")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Snapshot name contains illegal characters: o'snap"); + assertThatThrownBy(() -> cfs.snapshot("snap(1)")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("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) + .hasMessageContaining("Snapshot name contains illegal characters: a tag"); + assertThatThrownBy(() -> cfs.snapshot("a:tag")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("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); + + // 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. + 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); + } }