diff --git a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java index 4775a8425f9d..2be55fbe5c69 100644 --- a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java +++ b/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"), diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java index ce35c9ca1bd2..96b268a9e3d7 100644 --- a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java +++ b/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"; + + // Allowed snapshot-name characters: 0-9 a-z A-Z - _ . + + // See the validation site in validateTag for the full rationale (an S3 safe-set subset, plus '+'). + // 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 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,68 @@ 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 - _ . + // 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 (!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 +271,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); } } diff --git a/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java b/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java index 236a3ab783aa..2c69309d0b79 100644 --- a/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java +++ b/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java @@ -88,7 +88,7 @@ public Map getSnapshotsToCreate() else creationTime = options.creationTime; - snapshotName = options.getSnapshotName(creationTime); + snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, creationTime); Set entitiesForSnapshot = options.cfs == null ? parseEntitiesForSnapshot(options.entities) : Set.of(options.cfs); diff --git a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java index 30b94d32b1c5..6d5459e52bda 100644 --- a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java +++ b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java @@ -26,15 +26,126 @@ 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"); + + // "+" 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. + // The character check applies uniformly to all snapshot types, so "+" is accepted + // for system and user snapshots alike. + validate("this_is_system_snapshot-7.0.0+abc123", SnapshotType.UPGRADE); + validate("this_is_snapshot-7.0.0+abc123", USER); + } + + 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 sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, SnapshotType.USER); + List sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, USER); for (SnapshotType type : sameNameTypes) { @@ -42,7 +153,7 @@ public void testSnapshotName() .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 +168,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); }