Skip to content

Commit 816bf6d

Browse files
smiklosovicsmiklosovic
committed
harden snapshot names
1 parent 1656a9c commit 816bf6d

3 files changed

Lines changed: 158 additions & 2 deletions

File tree

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,10 @@ public enum CassandraRelevantProperties
511511
SNAPSHOT_CLEANUP_PERIOD_SECONDS("cassandra.snapshot.ttl_cleanup_period_seconds", "60"),
512512
/** minimum allowed TTL for snapshots */
513513
SNAPSHOT_MIN_ALLOWED_TTL_SECONDS("cassandra.snapshot.min_allowed_ttl_seconds", "60"),
514+
/**
515+
* Validates names of to-be-taken snapshots, defaults to true.
516+
*/
517+
SNAPSHOT_NAME_VALIDATION("cassandra.snapshot.validation", "false"),
514518
SSL_ENABLE("ssl.enable"),
515519
SSL_STORAGE_PORT("cassandra.ssl_storage_port"),
516520
START_GOSSIP("cassandra.start_gossip", "true"),

‎src/java/org/apache/cassandra/db/ColumnFamilyStore.java‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@
7878
import org.apache.cassandra.cache.RowCacheSentinel;
7979
import org.apache.cassandra.concurrent.ExecutorPlus;
8080
import org.apache.cassandra.concurrent.FutureTask;
81+
import org.apache.cassandra.config.CassandraRelevantProperties;
8182
import org.apache.cassandra.config.DatabaseDescriptor;
8283
import org.apache.cassandra.config.DurationSpec;
8384
import org.apache.cassandra.db.commitlog.CommitLog;
@@ -178,9 +179,11 @@
178179
import org.apache.cassandra.utils.concurrent.Refs;
179180
import org.apache.cassandra.utils.concurrent.UncheckedInterruptedException;
180181

182+
import static java.lang.String.format;
181183
import static org.apache.cassandra.concurrent.ExecutorFactory.Global.executorFactory;
182184
import static org.apache.cassandra.config.DatabaseDescriptor.getFlushWriters;
183185
import static org.apache.cassandra.db.commitlog.CommitLogPosition.NONE;
186+
import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH;
184187
import static org.apache.cassandra.utils.Clock.Global.currentTimeMillis;
185188
import static org.apache.cassandra.utils.Clock.Global.nanoTime;
186189
import static org.apache.cassandra.utils.FBUtilities.now;
@@ -2138,6 +2141,8 @@ public TableSnapshot snapshotWithoutMemtable(String snapshotName, Predicate<SSTa
21382141
throw new IllegalStateException(String.format("can not take ephemeral snapshot (%s) while ttl is specified too", snapshotName));
21392142
}
21402143

2144+
validateSnapshotName(snapshotName);
2145+
21412146
if (rateLimiter == null)
21422147
rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
21432148

@@ -2160,6 +2165,44 @@ public TableSnapshot snapshotWithoutMemtable(String snapshotName, Predicate<SSTa
21602165
return createSnapshot(snapshotName, ephemeral, ttl, snapshottedSSTables, creationTime);
21612166
}
21622167

2168+
private void validateSnapshotName(String snapshotName)
2169+
{
2170+
if (snapshotName.contains(File.pathSeparator()))
2171+
{
2172+
throw new IllegalArgumentException("Snapshot name cannot contain " + File.pathSeparator());
2173+
}
2174+
2175+
if (snapshotName.equals(".") || snapshotName.equals(".."))
2176+
{
2177+
throw new IllegalArgumentException("Snapshot name '" + snapshotName + "' is reserved");
2178+
}
2179+
2180+
if (!CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION.getBoolean())
2181+
return;
2182+
2183+
// the length of valid snapshot name has to be less than or equal to FILENAME_LEGTH - that is 255 -
2184+
// we are following the max length as it is in SchemaConstants for table name.
2185+
if (snapshotName.length() > SchemaConstants.FILENAME_LENGTH)
2186+
{
2187+
throw new IllegalArgumentException(format("Snapshot name must not be more than %d characters long for " +
2188+
"resolved snapshot name (got %d characters for \"%s\")",
2189+
FILENAME_LENGTH, snapshotName.length(), snapshotName));
2190+
}
2191+
2192+
// Allowed characters are a conservative subset of the AWS S3 "Safe characters" set
2193+
// (https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines):
2194+
// 0-9 a-z A-Z - _ .
2195+
// plus '+', which is not an S3 "Safe character" but can legitimately appear in system
2196+
// snapshot names via version build metadata (e.g. an upgrade snapshot "<millis>-upgrade-5.0.4+build-...").
2197+
// The remaining S3-safe characters (! * ' ( )) are intentionally excluded as they are
2198+
// shell-significant and error-prone in paths, and the path separator '/' is excluded too,
2199+
// which is what blocks traversal attempts such as "../../mysnapshot"
2200+
if (!Pattern.compile("[a-zA-Z0-9_.+-]+").matcher(snapshotName).matches())
2201+
{
2202+
throw new IllegalArgumentException("Snapshot name contains illegal characters: " + snapshotName);
2203+
}
2204+
}
2205+
21632206
protected TableSnapshot createSnapshot(String tag, boolean ephemeral, DurationSpec.IntSecondsBound ttl, Set<SSTableReader> sstables, Instant creationTime) {
21642207
Set<File> snapshotDirs = sstables.stream()
21652208
.map(s -> Directories.getSnapshotDirectory(s.descriptor, tag).toAbsolute())

‎test/unit/org/apache/cassandra/db/SnapshotTest.java‎

Lines changed: 111 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,20 +21,32 @@
2121
import java.nio.file.Files;
2222
import java.nio.file.StandardOpenOption;
2323

24+
import org.junit.Before;
2425
import org.junit.Test;
2526

27+
import org.apache.cassandra.config.CassandraRelevantProperties;
2628
import org.apache.cassandra.cql3.CQLTester;
29+
import org.apache.cassandra.distributed.shared.WithProperties;
2730
import org.apache.cassandra.io.sstable.format.SSTableFormat.Components;
2831
import org.apache.cassandra.io.sstable.format.SSTableReader;
2932
import org.apache.cassandra.io.util.File;
33+
import org.apache.cassandra.schema.SchemaConstants;
34+
35+
import static org.assertj.core.api.Assertions.assertThatCode;
36+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
3037

3138
public class SnapshotTest extends CQLTester
3239
{
33-
@Test
34-
public void testEmptyTOC() throws Throwable
40+
@Before
41+
public void setUpTable() throws Throwable
3542
{
3643
createTable("create table %s (id int primary key, k int)");
3744
execute("insert into %s (id, k) values (1,1)");
45+
}
46+
47+
@Test
48+
public void testEmptyTOC() throws Throwable
49+
{
3850
getCurrentColumnFamilyStore().forceBlockingFlush(ColumnFamilyStore.FlushReason.UNIT_TESTS);
3951
for (SSTableReader sstable : getCurrentColumnFamilyStore().getLiveSSTables())
4052
{
@@ -43,4 +55,101 @@ public void testEmptyTOC() throws Throwable
4355
}
4456
getCurrentColumnFamilyStore().snapshot("hello");
4557
}
58+
59+
@Test
60+
public void testSnapshotNameValidation()
61+
{
62+
ColumnFamilyStore cfs = getCurrentColumnFamilyStore();
63+
String sep = File.pathSeparator();
64+
65+
try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, true))
66+
{
67+
// Previously-allowed alphanumerics, '-' and '_' must still be accepted.
68+
assertThatCode(() -> cfs.snapshot("atag")).doesNotThrowAnyException();
69+
assertThatCode(() -> cfs.snapshot("a-tag")).doesNotThrowAnyException();
70+
assertThatCode(() -> cfs.snapshot("a_tag")).doesNotThrowAnyException();
71+
assertThatCode(() -> cfs.snapshot("a_tag_1and_something2-more")).doesNotThrowAnyException();
72+
assertThatCode(() -> cfs.snapshot(repeat('a', SchemaConstants.FILENAME_LENGTH))).doesNotThrowAnyException();
73+
74+
// Only alphanumerics, '-', '_' and '.' are accepted.
75+
assertThatCode(() -> cfs.snapshot("snap.2026-05-20")).doesNotThrowAnyException();
76+
// Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory.
77+
assertThatCode(() -> cfs.snapshot("a..tag")).doesNotThrowAnyException();
78+
79+
// "+" is part of the allowed set because it can appear in a Cassandra version
80+
// (build metadata, e.g. "7.0.0+abc123"), which ends up in system snapshot names.
81+
assertThatCode(() -> cfs.snapshot("this_is_snapshot-7.0.0+abc123")).doesNotThrowAnyException();
82+
83+
String tooLong = repeat('a', SchemaConstants.FILENAME_LENGTH + 1);
84+
assertThatThrownBy(() -> cfs.snapshot(tooLong))
85+
.isInstanceOf(IllegalArgumentException.class)
86+
.hasMessage("Snapshot name must not be more than 255 characters long for " +
87+
"resolved snapshot name (got 256 characters for \"" + tooLong + "\")");
88+
89+
// '/' is not in the allowed set; this is what kills traversal attempts like "../../mysnapshot".
90+
assertThatThrownBy(() -> cfs.snapshot("a" + sep + "tag"))
91+
.isInstanceOf(IllegalArgumentException.class)
92+
.hasMessage("Snapshot name cannot contain " + sep);
93+
94+
// The shell-significant S3 "safe" characters (! * ' ( )) are deliberately NOT allowed.
95+
assertThatThrownBy(() -> cfs.snapshot("important!"))
96+
.isInstanceOf(IllegalArgumentException.class)
97+
.hasMessage("Snapshot name contains illegal characters: important!");
98+
assertThatThrownBy(() -> cfs.snapshot("backup*"))
99+
.isInstanceOf(IllegalArgumentException.class)
100+
.hasMessage("Snapshot name contains illegal characters: backup*");
101+
assertThatThrownBy(() -> cfs.snapshot("o'snap"))
102+
.isInstanceOf(IllegalArgumentException.class)
103+
.hasMessage("Snapshot name contains illegal characters: o'snap");
104+
assertThatThrownBy(() -> cfs.snapshot("snap(1)"))
105+
.isInstanceOf(IllegalArgumentException.class)
106+
.hasMessage("Snapshot name contains illegal characters: snap(1)");
107+
108+
// Other characters outside the allowed set must still be rejected.
109+
assertThatThrownBy(() -> cfs.snapshot("a tag"))
110+
.isInstanceOf(IllegalArgumentException.class)
111+
.hasMessage("Snapshot name contains illegal characters: a tag");
112+
assertThatThrownBy(() -> cfs.snapshot("a:tag"))
113+
.isInstanceOf(IllegalArgumentException.class)
114+
.hasMessage("Snapshot name contains illegal characters: a:tag");
115+
116+
// "." and ".." pass the charset check but resolve to the snapshots/ dir itself
117+
// and its parent (the live table dir) respectively, so they must be rejected as reserved.
118+
assertThatThrownBy(() -> cfs.snapshot("."))
119+
.isInstanceOf(IllegalArgumentException.class)
120+
.hasMessage("Snapshot name '.' is reserved");
121+
assertThatThrownBy(() -> cfs.snapshot(".."))
122+
.isInstanceOf(IllegalArgumentException.class)
123+
.hasMessage("Snapshot name '..' is reserved");
124+
}
125+
126+
try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false))
127+
{
128+
// The character check is bypassed entirely: space, ':', and the now-disallowed
129+
// shell-significant characters (! * ' ( )) are all accepted.
130+
assertThatCode(() -> cfs.snapshot("a tag")).doesNotThrowAnyException();
131+
assertThatCode(() -> cfs.snapshot("a:tag")).doesNotThrowAnyException();
132+
assertThatCode(() -> cfs.snapshot("important!")).doesNotThrowAnyException();
133+
assertThatCode(() -> cfs.snapshot("snap(1)")).doesNotThrowAnyException();
134+
135+
// Path separator and "." / ".." rejections are unconditional — they guard against
136+
// traversal regardless of the toggle.
137+
assertThatThrownBy(() -> cfs.snapshot("a" + sep + "tag"))
138+
.isInstanceOf(IllegalArgumentException.class)
139+
.hasMessage("Snapshot name cannot contain " + sep);
140+
assertThatThrownBy(() -> cfs.snapshot("."))
141+
.isInstanceOf(IllegalArgumentException.class)
142+
.hasMessage("Snapshot name '.' is reserved");
143+
assertThatThrownBy(() -> cfs.snapshot(".."))
144+
.isInstanceOf(IllegalArgumentException.class)
145+
.hasMessage("Snapshot name '..' is reserved");
146+
}
147+
}
148+
149+
private static String repeat(char c, int times)
150+
{
151+
char[] chars = new char[times];
152+
java.util.Arrays.fill(chars, c);
153+
return new String(chars);
154+
}
46155
}

0 commit comments

Comments
 (0)