From 0038591339e76da3f165a2c3b3a99e5f190b2e62 Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Tue, 19 May 2026 13:34:29 +0200 Subject: [PATCH 1/2] Harden snapshot names on server side --- .../service/snapshot/SnapshotOptions.java | 48 ++++++++++++++++--- .../service/snapshot/TakeSnapshotTask.java | 2 +- .../service/snapshot/SnapshotOptionsTest.java | 44 +++++++++++++++-- 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java index ce35c9ca1bd2..9a83f6ae3bc6 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,8 +34,10 @@ import org.apache.cassandra.config.DurationSpec; import org.apache.cassandra.db.ColumnFamilyStore; import org.apache.cassandra.io.sstable.format.SSTableReader; +import org.apache.cassandra.schema.SchemaConstants; import static java.lang.String.format; +import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH; public class SnapshotOptions { @@ -88,7 +91,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 +192,47 @@ 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."); + // 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, Instant.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)); + } + + // "-" is important, we are already generating system snapshots with "-" in them so this has to stay + // \\w are just "words" consisting of [a-zA-Z0-9_] + if (!Pattern.compile("[\\w-]+").matcher(resolvedSnapshotname).matches()) + { + throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotname); + } + } + + private void validateTTL(boolean ephemeral, DurationSpec.IntSecondsBound ttl) + { if (ttl != null) { int minAllowedTtlSecs = CassandraRelevantProperties.SNAPSHOT_MIN_ALLOWED_TTL_SECONDS.getInt(); @@ -202,12 +242,8 @@ 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); } + } @Override 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..a6ae7d894278 100644 --- a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java +++ b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java @@ -26,15 +26,53 @@ import org.junit.Test; +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.assertThatThrownBy; import static org.junit.Assert.assertEquals; public class SnapshotOptionsTest { + @Test + public void testSnapshotNameValidation() + { + 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); + + 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"); + + assertThatThrownBy(() -> validate('a' + File.pathSeparator() + "tag", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: a/tag"); + + assertThatThrownBy(() -> validate("a..tag", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: a..tag"); + } + + 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 +80,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 +95,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); } From 828d1eff5c35bf2765462194550b3cb8eb4e8dea Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Wed, 20 May 2026 14:48:39 +0200 Subject: [PATCH 2/2] introduce safe characters per AWS guidelines --- .../service/snapshot/SnapshotOptions.java | 24 +++++++++++--- .../service/snapshot/SnapshotOptionsTest.java | 32 +++++++++++++++++-- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java index 9a83f6ae3bc6..fc82ed8bf826 100644 --- a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java +++ b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java @@ -43,6 +43,12 @@ public class SnapshotOptions { public static final String SKIP_FLUSH = "skipFlush"; public static final String TTL = "ttl"; + + // AWS S3 "Safe characters" for object keys: + // 0-9 a-z A-Z ! - _ . * ' ( ) + // See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines + // 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; @@ -223,12 +229,23 @@ private void validateTag(String tag) FILENAME_LENGTH, resolvedSnapshotname.length(), resolvedSnapshotname)); } - // "-" is important, we are already generating system snapshots with "-" in them so this has to stay - // \\w are just "words" consisting of [a-zA-Z0-9_] - if (!Pattern.compile("[\\w-]+").matcher(resolvedSnapshotname).matches()) + // 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 (!SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotname).matches()) { throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotname); } + + // Because '.' is part of the safe set, the names "." and ".." would pass the charset check + // but resolve to the snapshots/ directory itself or its parent (the live table directory) + // when used as a path component. Reject them explicitly. + if (resolvedSnapshotname.equals(".") || resolvedSnapshotname.equals("..")) + { + throw new IllegalArgumentException("Snapshot name '" + resolvedSnapshotname + "' is reserved"); + } } private void validateTTL(boolean ephemeral, DurationSpec.IntSecondsBound ttl) @@ -243,7 +260,6 @@ private void validateTTL(boolean ephemeral, DurationSpec.IntSecondsBound ttl) if (ephemeral && ttl != null) throw new IllegalStateException(format("can not take ephemeral snapshot (%s) while ttl is specified too", tag)); } - } @Override diff --git a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java index a6ae7d894278..4ad201c14f15 100644 --- a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java +++ b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java @@ -38,6 +38,7 @@ public class SnapshotOptionsTest @Test public void testSnapshotNameValidation() { + // Previously-allowed alphanumerics, '-' and '_' must still be accepted. validate("atag", USER); validate("a-tag", USER); validate("a_tag", USER); @@ -45,6 +46,18 @@ public void testSnapshotNameValidation() validate("a_tag_1and_something2-more", USER); validate("a".repeat(255), USER); + // AWS S3 "Safe characters" newly accepted by the relaxed allowlist: + // ! . * ' ( ) + // See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines + validate("snap.2026-05-20", USER); + validate("important!", USER); + validate("backup*", USER); + validate("o'snap", USER); + validate("snap(1)", USER); + validate("!._-*'()", 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 " + @@ -55,13 +68,28 @@ public void testSnapshotNameValidation() .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("Snapshot name must not be more than 255 characters long"); + // '/' is not in the S3-safe set; this is what kills traversal attempts like "../../mysnapshot". assertThatThrownBy(() -> validate('a' + File.pathSeparator() + "tag", USER)) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Snapshot name contains illegal characters: a/tag"); - assertThatThrownBy(() -> validate("a..tag", USER)) + // Other characters outside the S3-safe set must still be rejected. + assertThatThrownBy(() -> validate("a tag", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: a tag"); + assertThatThrownBy(() -> validate("a:tag", USER)) + .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(() -> validate(".", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '.' is reserved"); + + assertThatThrownBy(() -> validate("..", USER)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Snapshot name contains illegal characters: a..tag"); + .hasMessage("Snapshot name '..' is reserved"); } private void validate(String tag, SnapshotType type)