diff --git a/CHANGES.txt b/CHANGES.txt index be3a765b0f..d9be2ff393 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -2,6 +2,7 @@ * Add Paxos v2 option and informatin in cassandra.yaml (CASSANDRA-21316) * Harden data resurrection startup check with atomic heartbeat file write with fallback (CASSANDRA-21290) Merged from 4.0: + * 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 dcd777fc03..12aea18917 100644 --- a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java +++ b/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"), diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index da859a548c..2d23522280 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -79,6 +79,7 @@ import org.apache.cassandra.cache.RowCacheKey; 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.Refs; 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 class ColumnFamilyStore implements ColumnFamilyStoreMBean, Memtable.Owner 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,46 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean, Memtable.Owner 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 - _ . + // 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."); + } + } + protected TableSnapshot createSnapshot(String tag, boolean ephemeral, DurationSpec.IntSecondsBound ttl, Set sstables, Instant creationTime) { Set snapshotDirs = sstables.stream() .map(s -> Directories.getSnapshotDirectory(s.descriptor, tag).toAbsolute()) diff --git a/test/unit/org/apache/cassandra/db/SnapshotTest.java b/test/unit/org/apache/cassandra/db/SnapshotTest.java index aa726ec3e0..092ac4d96d 100644 --- a/test/unit/org/apache/cassandra/db/SnapshotTest.java +++ b/test/unit/org/apache/cassandra/db/SnapshotTest.java @@ -21,20 +21,32 @@ package org.apache.cassandra.db; 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,105 @@ public class SnapshotTest extends CQLTester } 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(); + + // "+" 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); + } }