Skip to content

Commit f35bb7b

Browse files
smiklosovicsmiklosovic
committed
harden snapshot names
1 parent a1fc6c8 commit f35bb7b

3 files changed

Lines changed: 146 additions & 2 deletions

File tree

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,12 @@ public enum CassandraRelevantProperties
210210
/** This property indicates whether disable_mbean_registration is true */
211211
IS_DISABLED_MBEAN_REGISTRATION("org.apache.cassandra.disable_mbean_registration"),
212212

213+
/**
214+
* Validates names of to-be-taken snapshots, defaults to false.
215+
*/
216+
SNAPSHOT_NAME_VALIDATION("cassandra.snapshot.validation", "false"),
217+
218+
213219
/** what class to use for mbean registeration */
214220
MBEAN_REGISTRATION_CLASS("org.apache.cassandra.mbean_registration_class"),
215221

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@
9393

9494
import static java.util.concurrent.TimeUnit.NANOSECONDS;
9595

96+
import static java.lang.String.format;
97+
import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH;
9698
import static org.apache.cassandra.utils.Throwables.maybeFail;
9799
import static org.apache.cassandra.utils.Throwables.merge;
98100

@@ -1837,6 +1839,8 @@ public void snapshotWithoutFlush(String snapshotName)
18371839
*/
18381840
public Set<SSTableReader> snapshotWithoutFlush(String snapshotName, Predicate<SSTableReader> predicate, boolean ephemeral, RateLimiter rateLimiter)
18391841
{
1842+
validateSnapshotName(snapshotName);
1843+
18401844
if (rateLimiter == null)
18411845
rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
18421846

@@ -1868,6 +1872,41 @@ public Set<SSTableReader> snapshotWithoutFlush(String snapshotName, Predicate<SS
18681872
return snapshottedSSTables;
18691873
}
18701874

1875+
private void validateSnapshotName(String snapshotName)
1876+
{
1877+
if (snapshotName.contains(File.separator))
1878+
{
1879+
throw new IllegalArgumentException("Snapshot name cannot contain " + File.separator);
1880+
}
1881+
1882+
if (snapshotName.equals(".") || snapshotName.equals(".."))
1883+
{
1884+
throw new IllegalArgumentException("Snapshot name '" + snapshotName + "' is reserved");
1885+
}
1886+
1887+
if (!CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION.getBoolean())
1888+
return;
1889+
1890+
// the length of valid snapshot name has to be less than or equal to FILENAME_LENGTH - that is 255 -
1891+
// we are following the max length as it is in SchemaConstants for table name.
1892+
if (snapshotName.length() > SchemaConstants.FILENAME_LENGTH)
1893+
{
1894+
throw new IllegalArgumentException(format("Snapshot name must not be more than %d characters long for " +
1895+
"resolved snapshot name (got %d characters for \"%s\")",
1896+
FILENAME_LENGTH, snapshotName.length(), snapshotName));
1897+
}
1898+
1899+
// Allowed characters follow the AWS S3 "Safe characters" set documented at
1900+
// https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines :
1901+
// 0-9 a-z A-Z ! - _ . * ' ( )
1902+
// The path separator '/' is intentionally excluded,
1903+
// which is what blocks traversal attempts such as "../../mysnapshot"
1904+
if (!Pattern.compile("[a-zA-Z0-9!_.*'()-]+").matcher(snapshotName).matches())
1905+
{
1906+
throw new IllegalArgumentException("Snapshot name contains illegal characters: " + snapshotName);
1907+
}
1908+
}
1909+
18711910
private void writeSnapshotManifest(final JSONArray filesJSONArr, final String snapshotName)
18721911
{
18731912
final File manifestFile = getDirectories().getSnapshotManifestFile(snapshotName);

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

Lines changed: 101 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,19 +22,31 @@
2222
import java.nio.file.Files;
2323
import java.nio.file.StandardOpenOption;
2424

25+
import org.junit.Before;
2526
import org.junit.Test;
2627

28+
import org.apache.cassandra.config.CassandraRelevantProperties;
2729
import org.apache.cassandra.cql3.CQLTester;
30+
import org.apache.cassandra.distributed.shared.WithProperties;
2831
import org.apache.cassandra.io.sstable.Component;
2932
import org.apache.cassandra.io.sstable.format.SSTableReader;
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();
3951
for (SSTableReader sstable : getCurrentColumnFamilyStore().getLiveSSTables())
4052
{
@@ -43,4 +55,91 @@ 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.separator;
64+
65+
try (WithProperties p = new WithProperties())
66+
{
67+
p.set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, true);
68+
69+
// Previously-allowed alphanumerics, '-' and '_' must still be accepted.
70+
assertThatCode(() -> cfs.snapshot("atag")).doesNotThrowAnyException();
71+
assertThatCode(() -> cfs.snapshot("a-tag")).doesNotThrowAnyException();
72+
assertThatCode(() -> cfs.snapshot("a_tag")).doesNotThrowAnyException();
73+
assertThatCode(() -> cfs.snapshot("a_tag_1and_something2-more")).doesNotThrowAnyException();
74+
assertThatCode(() -> cfs.snapshot(repeat('a', SchemaConstants.FILENAME_LENGTH))).doesNotThrowAnyException();
75+
76+
// AWS S3 "Safe characters" accepted by the relaxed allowlist:
77+
// ! . * ' ( )
78+
// See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines
79+
assertThatCode(() -> cfs.snapshot("snap.2026-05-20")).doesNotThrowAnyException();
80+
assertThatCode(() -> cfs.snapshot("important!")).doesNotThrowAnyException();
81+
assertThatCode(() -> cfs.snapshot("backup*")).doesNotThrowAnyException();
82+
assertThatCode(() -> cfs.snapshot("o'snap")).doesNotThrowAnyException();
83+
assertThatCode(() -> cfs.snapshot("snap(1)")).doesNotThrowAnyException();
84+
assertThatCode(() -> cfs.snapshot("!._-*'()")).doesNotThrowAnyException();
85+
// Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory.
86+
assertThatCode(() -> cfs.snapshot("a..tag")).doesNotThrowAnyException();
87+
88+
String tooLong = repeat('a', SchemaConstants.FILENAME_LENGTH + 1);
89+
assertThatThrownBy(() -> cfs.snapshot(tooLong))
90+
.isInstanceOf(IllegalArgumentException.class)
91+
.hasMessage("Snapshot name must not be more than 255 characters long for " +
92+
"resolved snapshot name (got 256 characters for \"" + tooLong + "\")");
93+
94+
// '/' is not in the S3-safe set; this is what kills traversal attempts like "../../mysnapshot".
95+
assertThatThrownBy(() -> cfs.snapshot("a" + sep + "tag"))
96+
.isInstanceOf(IllegalArgumentException.class)
97+
.hasMessage("Snapshot name cannot contain " + sep);
98+
99+
// Other characters outside the S3-safe set must still be rejected.
100+
assertThatThrownBy(() -> cfs.snapshot("a tag"))
101+
.isInstanceOf(IllegalArgumentException.class)
102+
.hasMessage("Snapshot name contains illegal characters: a tag");
103+
assertThatThrownBy(() -> cfs.snapshot("a:tag"))
104+
.isInstanceOf(IllegalArgumentException.class)
105+
.hasMessage("Snapshot name contains illegal characters: a:tag");
106+
107+
// "." and ".." pass the charset check but resolve to the snapshots/ dir itself
108+
// and its parent (the live table dir) respectively, so they must be rejected as reserved.
109+
assertThatThrownBy(() -> cfs.snapshot("."))
110+
.isInstanceOf(IllegalArgumentException.class)
111+
.hasMessage("Snapshot name '.' is reserved");
112+
assertThatThrownBy(() -> cfs.snapshot(".."))
113+
.isInstanceOf(IllegalArgumentException.class)
114+
.hasMessage("Snapshot name '..' is reserved");
115+
}
116+
117+
try (WithProperties p = new WithProperties())
118+
{
119+
p.set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false);
120+
121+
// Previously-rejected characters are now accepted: space, ':', and other non-S3-safe chars.
122+
assertThatCode(() -> cfs.snapshot("a tag")).doesNotThrowAnyException();
123+
assertThatCode(() -> cfs.snapshot("a:tag")).doesNotThrowAnyException();
124+
125+
// Path separator and "." / ".." rejections are unconditional — they guard against
126+
// traversal regardless of the toggle.
127+
assertThatThrownBy(() -> cfs.snapshot("a" + sep + "tag"))
128+
.isInstanceOf(IllegalArgumentException.class)
129+
.hasMessage("Snapshot name cannot contain " + sep);
130+
assertThatThrownBy(() -> cfs.snapshot("."))
131+
.isInstanceOf(IllegalArgumentException.class)
132+
.hasMessage("Snapshot name '.' is reserved");
133+
assertThatThrownBy(() -> cfs.snapshot(".."))
134+
.isInstanceOf(IllegalArgumentException.class)
135+
.hasMessage("Snapshot name '..' is reserved");
136+
}
137+
}
138+
139+
private static String repeat(char c, int times)
140+
{
141+
char[] chars = new char[times];
142+
java.util.Arrays.fill(chars, c);
143+
return new String(chars);
144+
}
46145
}

0 commit comments

Comments
 (0)