CASSANDRA-21389 4.1 harden snapshot names

src/java/org/apache/cassandra/config/CassandraRelevantProperties.java

@@ -238,6 +238,11 @@ public enum CassandraRelevantProperties
     /** minimum allowed TTL for snapshots */
     SNAPSHOT_MIN_ALLOWED_TTL_SECONDS("cassandra.snapshot.min_allowed_ttl_seconds", "60"),
 
+    /**
+     * 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"),
 

src/java/org/apache/cassandra/db/ColumnFamilyStore.java

@@ -79,6 +79,7 @@
 import org.apache.cassandra.cache.RowCacheSentinel;
 import org.apache.cassandra.concurrent.ExecutorPlus;
 import org.apache.cassandra.concurrent.FutureTask;
+import org.apache.cassandra.config.CassandraRelevantProperties;
 import org.apache.cassandra.config.DatabaseDescriptor;
 import org.apache.cassandra.config.DurationSpec;
 import org.apache.cassandra.db.commitlog.CommitLog;
@@ -174,9 +175,11 @@
 import org.apache.cassandra.utils.concurrent.UncheckedInterruptedException;
 
 import static com.google.common.base.Throwables.propagate;
+import static java.lang.String.format;
 import static org.apache.cassandra.concurrent.ExecutorFactory.Global.executorFactory;
 import static org.apache.cassandra.config.DatabaseDescriptor.getFlushWriters;
 import static org.apache.cassandra.db.commitlog.CommitLogPosition.NONE;
+import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH;
 import static org.apache.cassandra.utils.Clock.Global.currentTimeMillis;
 import static org.apache.cassandra.utils.Clock.Global.nanoTime;
 import static org.apache.cassandra.utils.FBUtilities.now;
@@ -2011,6 +2014,8 @@ public TableSnapshot snapshotWithoutMemtable(String snapshotName, Predicate<SSTa
             throw new IllegalStateException(String.format("can not take ephemeral snapshot (%s) while ttl is specified too", snapshotName));
         }
 
+        validateSnapshotName(snapshotName);
+
         if (rateLimiter == null)
             rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
 
@@ -2034,6 +2039,42 @@ public TableSnapshot snapshotWithoutMemtable(String snapshotName, Predicate<SSTa
         return createSnapshot(snapshotName, ephemeral, ttl, snapshottedSSTables, creationTime);
     }
 
+    private void validateSnapshotName(String snapshotName)
+    {
+        if (snapshotName.contains(File.pathSeparator()))
+        {
+            throw new IllegalArgumentException("Snapshot name cannot contain " + File.pathSeparator());
+        }
+
+        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_LEGTH - 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  -  _  .
+        // 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);
+        }
+    }
+
     protected TableSnapshot createSnapshot(String tag, boolean ephemeral, DurationSpec.IntSecondsBound ttl, Set<SSTableReader> sstables, Instant creationTime) {
         Set<File> snapshotDirs = sstables.stream()
                                          .map(s -> Directories.getSnapshotDirectory(s.descriptor, tag).toAbsolute())

test/unit/org/apache/cassandra/db/SnapshotTest.java

@@ -21,20 +21,32 @@
 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.io.util.File;
+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(ColumnFamilyStore.FlushReason.UNIT_TESTS);
         for (SSTableReader sstable : getCurrentColumnFamilyStore().getLiveSSTables())
         {
@@ -43,4 +55,101 @@ public void testEmptyTOC() throws Throwable
         }
         getCurrentColumnFamilyStore().snapshot("hello");
     }
+
+    @Test
+    public void testSnapshotNameValidation()
+    {
+        ColumnFamilyStore cfs = getCurrentColumnFamilyStore();
+        String sep = File.pathSeparator();
+
+        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();
+
+            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)
+            .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");
+            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);
+
+            // 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);
+    }
 }