CASSANDRA-21389 trunk Harden snapshot names on server side

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

@@ -573,6 +573,10 @@ public enum CassandraRelevantProperties
     SNAPSHOT_MANIFEST_ENRICH_OR_CREATE_ENABLED("cassandra.snapshot.enrich.or.create.enabled", "true"),
     /** 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 true.
+     */
+    SNAPSHOT_NAME_VALIDATION("cassandra.snapshot.validation", "true"),
     SSL_ENABLE("ssl.enable"),
     SSL_STORAGE_PORT("cassandra.ssl_storage_port"),
     START_GOSSIP("cassandra.start_gossip", "true"),

src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java

@@ -23,6 +23,7 @@
 import java.util.Collections;
 import java.util.Map;
 import java.util.function.Predicate;
+import java.util.regex.Pattern;
 
 import com.google.common.util.concurrent.RateLimiter;
 
@@ -33,13 +34,23 @@
 import org.apache.cassandra.config.DurationSpec;
 import org.apache.cassandra.db.ColumnFamilyStore;
 import org.apache.cassandra.io.sstable.format.SSTableReader;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.schema.SchemaConstants;
 
 import static java.lang.String.format;
+import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH;
+import static org.apache.cassandra.utils.FBUtilities.now;
 
 public class SnapshotOptions
 {
     public static final String SKIP_FLUSH = "skipFlush";
     public static final String TTL = "ttl";
+
+    // Conservative subset of the AWS S3 "Safe characters" set: 0-9  a-z  A-Z  -  _  .
+    // See the validation site in validateTag for the full rationale on excluded characters.
+    // Hyphen is placed last in the character class, so it stays literal and never becomes a range operator.
+    private static final Pattern SAFE_SNAPSHOT_NAME = Pattern.compile("[a-zA-Z0-9_.-]+");
+
     public final SnapshotType type;
     public final String tag;
     public final DurationSpec.IntSecondsBound ttl;
@@ -88,7 +99,7 @@ public static SnapshotOptions userSnapshot(String tag, Map<String, String> optio
         return builder.build();
     }
 
-    public String getSnapshotName(Instant creationTime)
+    public static String getSnapshotName(SnapshotType type, String tag, Instant creationTime)
     {
         // Diagnostic snapshots have very specific naming convention hence we are keeping it.
         // Repair snapshots rely on snapshots having name of their repair session ids
@@ -189,10 +200,66 @@ public Builder rateLimiter(RateLimiter rateLimiter)
         }
 
         public SnapshotOptions build()
+        {
+            validateTag(tag);
+            validateTTL(ephemeral, ttl);
+
+            if (rateLimiter == null)
+                rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
+
+            return new SnapshotOptions(this);
+        }
+
+        private void validateTag(String tag)
         {
             if (tag == null || tag.isEmpty())
-                throw new RuntimeException("You must supply a snapshot name.");
+                throw new IllegalArgumentException("You must supply a snapshot name.");
+
+            if (tag.contains(File.pathSeparator()))
+            {
+                throw new IllegalArgumentException("Snapshot name cannot contain " + File.pathSeparator());
+            }
+
+            if (tag.equals(".") || tag.equals(".."))
+            {
+                throw new IllegalArgumentException("Snapshot name '" + tag + "' is reserved");
+            }
 
+            if (!CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION.getBoolean())
+                return;
+
+            // Pre-generate snapshot name for the sake of the validation.
+            // getSnapshotName logic does not return raw "tag" as snapshot name every time,
+            // it e.g. prepends timestamp and type for system snapshots, and we need to validate it as a whole.
+            // If, for example, tag would be less than max allowed FILENAME_LENGTH,
+            // we might in fact produce a snapshot name longer than FILENAME_LENGTH if we prepended a timestamp to it.
+            String resolvedSnapshotName = SnapshotOptions.getSnapshotName(type, tag, now());
+
+            // 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 (resolvedSnapshotName.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, resolvedSnapshotName.length(), resolvedSnapshotName));
+            }
+
+            // 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 (type == SnapshotType.USER && !SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotName).matches())
+            {
+                throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotName + ". " +
+                                                   "Allowed characters must match the pattern: " + SAFE_SNAPSHOT_NAME.pattern() +
+                                                   " with a maximum of length of " + FILENAME_LENGTH + " characters.");
+            }
+        }
+
+        private void validateTTL(boolean ephemeral, DurationSpec.IntSecondsBound ttl)
+        {
             if (ttl != null)
             {
                 int minAllowedTtlSecs = CassandraRelevantProperties.SNAPSHOT_MIN_ALLOWED_TTL_SECONDS.getInt();
@@ -202,11 +269,6 @@ public SnapshotOptions build()
 
             if (ephemeral && ttl != null)
                 throw new IllegalStateException(format("can not take ephemeral snapshot (%s) while ttl is specified too", tag));
-
-            if (rateLimiter == null)
-                rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
-
-            return new SnapshotOptions(this);
         }
     }
 

src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java

@@ -88,7 +88,7 @@ public Map<ColumnFamilyStore, TableSnapshot> getSnapshotsToCreate()
         else
             creationTime = options.creationTime;
 
-        snapshotName = options.getSnapshotName(creationTime);
+        snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, creationTime);
 
         Set<ColumnFamilyStore> entitiesForSnapshot = options.cfs == null ? parseEntitiesForSnapshot(options.entities) : Set.of(options.cfs);
 

test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java

@@ -26,23 +26,135 @@
 
 import org.junit.Test;
 
+import org.apache.cassandra.config.CassandraRelevantProperties;
+import org.apache.cassandra.distributed.shared.WithProperties;
+import org.apache.cassandra.io.util.File;
+
 import static java.lang.String.format;
+import static org.apache.cassandra.service.snapshot.SnapshotType.USER;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.Assert.assertEquals;
 
 public class SnapshotOptionsTest
 {
+    @Test
+    public void testSnapshotNameValidation()
+    {
+        String sep = File.pathSeparator();
+
+        try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, true))
+        {
+            // Only alphanumerics, '-', '_' and '.' are accepted.
+            validate("atag", USER);
+            validate("a-tag", USER);
+            validate("a_tag", USER);
+            validate("a_tag" + Instant.now().toEpochMilli(), USER);
+            validate("a_tag_1and_something2-more", USER);
+            validate("a".repeat(255), USER);
+            validate("snap.2026-05-20", USER);
+            // Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory.
+            validate("a..tag", USER);
+
+            assertThatThrownBy(() -> validate("a".repeat(256), USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("Snapshot name must not be more than 255 characters long for " +
+                        "resolved snapshot name (got 256 characters for \"" + "a".repeat(256) + "\")");
+
+            // this would append timestamp + type which would violate < 255 when tag is 250 chars long only
+            assertThatThrownBy(() -> validate("a".repeat(250), SnapshotType.UPGRADE))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessageContaining("Snapshot name must not be more than 255 characters long");
+
+            // '/' is not in the allowed set; this is what kills traversal attempts like "../../mysnapshot".
+            assertThatThrownBy(() -> validate('a' + sep + "tag", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("Snapshot name cannot contain " + sep);
+
+            // The shell-significant S3 "safe" characters (! * ' ( )) are deliberately NOT allowed.
+            assertThatThrownBy(() -> validate("important!", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessageContaining("Snapshot name contains illegal characters: important!");
+            assertThatThrownBy(() -> validate("backup*", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessageContaining("Snapshot name contains illegal characters: backup*");
+            assertThatThrownBy(() -> validate("o'snap", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessageContaining("Snapshot name contains illegal characters: o'snap");
+            assertThatThrownBy(() -> validate("snap(1)", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessageContaining("Snapshot name contains illegal characters: snap(1)");
+
+            // Other characters outside the allowed set must still be rejected.
+            assertThatThrownBy(() -> validate("a tag", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessageContaining("Snapshot name contains illegal characters: a tag");
+            assertThatThrownBy(() -> validate("a:tag", USER))
+            .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(() -> validate(".", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("Snapshot name '.' is reserved");
+
+            assertThatThrownBy(() -> validate("..", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("Snapshot name '..' is reserved");
+
+            // snapshot name contains "+" which might be present in a Cassandra version
+            // the usage of "+" is forbidden for USER snapshots
+            validate("this_is_system_snapshot-7.0.0+abc123", SnapshotType.UPGRADE);
+
+            assertThatThrownBy(() -> validate("this_is_snapshot-7.0.0+abc123", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessageContaining("Snapshot name contains illegal characters: this_is_snapshot-7.0.0+abc123");
+        }
+
+        try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false))
+        {
+            // The character check is bypassed entirely: space, ':', and the now-disallowed
+            // shell-significant characters (! * ' ( )) are all accepted.
+            assertThatCode(() -> validate("a tag", USER)).doesNotThrowAnyException();
+            assertThatCode(() -> validate("a:tag", USER)).doesNotThrowAnyException();
+            assertThatCode(() -> validate("important!", USER)).doesNotThrowAnyException();
+            assertThatCode(() -> validate("snap(1)", USER)).doesNotThrowAnyException();
+
+            assertThatCode(() -> validate("a".repeat(256), USER)).doesNotThrowAnyException();
+            assertThatCode(() -> validate("a".repeat(250), SnapshotType.UPGRADE)).doesNotThrowAnyException();
+
+            // Path separator and "." / ".." rejections are unconditional — they guard against
+            // traversal regardless of the toggle.
+            assertThatThrownBy(() -> validate('a' + sep + "tag", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("Snapshot name cannot contain " + sep);
+            assertThatThrownBy(() -> validate(".", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("Snapshot name '.' is reserved");
+            assertThatThrownBy(() -> validate("..", USER))
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("Snapshot name '..' is reserved");
+        }
+    }
+
+    private void validate(String tag, SnapshotType type)
+    {
+        new SnapshotOptions.Builder(tag, type, s -> true, "ks.tb").rateLimiter(RateLimiter.create(1)).build();
+    }
+
     @Test
     public void testSnapshotName()
     {
-        List<SnapshotType> sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, SnapshotType.USER);
+        List<SnapshotType> sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, USER);
 
         for (SnapshotType type : sameNameTypes)
         {
             SnapshotOptions options = SnapshotOptions.systemSnapshot("a_name", type, "ks.tb")
                                                      .rateLimiter(RateLimiter.create(5))
                                                      .build();
 
-            String snapshotName = options.getSnapshotName(Instant.now());
+            String snapshotName = SnapshotOptions.getSnapshotName(type, options.tag, Instant.now());
             assertEquals("a_name", snapshotName);
         }
 
@@ -57,7 +169,7 @@ public void testSnapshotName()
 
             Instant now = Instant.now();
 
-            String snapshotName = options.getSnapshotName(now);
+            String snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, now);
 
             assertEquals(format("%d-%s-%s", now.toEpochMilli(), type.label, "a_name"), snapshotName);
         }