diff --git a/CHANGES.txt b/CHANGES.txt index c764952023..274e2dcecf 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.0.21 + * 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 19d89f1e48..7bf72a3fb4 100644 --- a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java +++ b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java @@ -210,6 +210,11 @@ public enum CassandraRelevantProperties /** This property indicates whether disable_mbean_registration is true */ IS_DISABLED_MBEAN_REGISTRATION("org.apache.cassandra.disable_mbean_registration"), + /** + * 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 571b86e908..4ff6e096e7 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -93,6 +93,8 @@ import org.json.simple.JSONObject; import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.lang.String.format; +import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH; import static org.apache.cassandra.utils.Throwables.maybeFail; import static org.apache.cassandra.utils.Throwables.merge; @@ -1837,6 +1839,8 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean */ public Set snapshotWithoutFlush(String snapshotName, Predicate predicate, boolean ephemeral, RateLimiter rateLimiter) { + validateSnapshotName(snapshotName); + if (rateLimiter == null) rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter(); @@ -1868,6 +1872,46 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean return snapshottedSSTables; } + private void validateSnapshotName(String snapshotName) + { + if (snapshotName.contains(File.separator)) + { + throw new IllegalArgumentException("Snapshot name cannot contain " + File.separator); + } + + 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_LENGTH - 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."); + } + } + private void writeSnapshotManifest(final JSONArray filesJSONArr, final String snapshotName) { final File manifestFile = getDirectories().getSnapshotManifestFile(snapshotName); diff --git a/test/unit/org/apache/cassandra/db/SnapshotTest.java b/test/unit/org/apache/cassandra/db/SnapshotTest.java index a406048739..544a131dec 100644 --- a/test/unit/org/apache/cassandra/db/SnapshotTest.java +++ b/test/unit/org/apache/cassandra/db/SnapshotTest.java @@ -22,19 +22,31 @@ import java.io.File; 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.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(); 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.separator; + + 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); + } }