Skip to content

Commit b23b207

Browse files
smiklosovicsmiklosovic
committed
Harden snapshot names on server side
1 parent c84f3d4 commit b23b207

4 files changed

Lines changed: 169 additions & 11 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
@@ -573,6 +573,10 @@ public enum CassandraRelevantProperties
573573
SNAPSHOT_MANIFEST_ENRICH_OR_CREATE_ENABLED("cassandra.snapshot.enrich.or.create.enabled", "true"),
574574
/** minimum allowed TTL for snapshots */
575575
SNAPSHOT_MIN_ALLOWED_TTL_SECONDS("cassandra.snapshot.min_allowed_ttl_seconds", "60"),
576+
/**
577+
* Validates names of to-be-taken snapshots, defaults to true.
578+
*/
579+
SNAPSHOT_NAME_VALIDATION("cassandra.snapshot.validation", "true"),
576580
SSL_ENABLE("ssl.enable"),
577581
SSL_STORAGE_PORT("cassandra.ssl_storage_port"),
578582
START_GOSSIP("cassandra.start_gossip", "true"),

‎src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java‎

Lines changed: 65 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.Collections;
2424
import java.util.Map;
2525
import java.util.function.Predicate;
26+
import java.util.regex.Pattern;
2627

2728
import com.google.common.util.concurrent.RateLimiter;
2829

@@ -33,13 +34,22 @@
3334
import org.apache.cassandra.config.DurationSpec;
3435
import org.apache.cassandra.db.ColumnFamilyStore;
3536
import org.apache.cassandra.io.sstable.format.SSTableReader;
37+
import org.apache.cassandra.io.util.File;
38+
import org.apache.cassandra.schema.SchemaConstants;
3639

3740
import static java.lang.String.format;
41+
import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH;
3842

3943
public class SnapshotOptions
4044
{
4145
public static final String SKIP_FLUSH = "skipFlush";
4246
public static final String TTL = "ttl";
47+
48+
// Follows AWS S3 "Safe characters" for object keys:
49+
// 0-9 a-z A-Z ! - _ . * ' ( )
50+
// See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines
51+
// Hyphen is placed last in the character class, so it stays literal and never becomes a range operator.
52+
private static final Pattern SAFE_SNAPSHOT_NAME = Pattern.compile("[a-zA-Z0-9!_.*'()-]+");
4353
public final SnapshotType type;
4454
public final String tag;
4555
public final DurationSpec.IntSecondsBound ttl;
@@ -88,7 +98,7 @@ public static SnapshotOptions userSnapshot(String tag, Map<String, String> optio
8898
return builder.build();
8999
}
90100

91-
public String getSnapshotName(Instant creationTime)
101+
public static String getSnapshotName(SnapshotType type, String tag, Instant creationTime)
92102
{
93103
// Diagnostic snapshots have very specific naming convention hence we are keeping it.
94104
// Repair snapshots rely on snapshots having name of their repair session ids
@@ -189,10 +199,63 @@ public Builder rateLimiter(RateLimiter rateLimiter)
189199
}
190200

191201
public SnapshotOptions build()
202+
{
203+
validateTag(tag);
204+
validateTTL(ephemeral, ttl);
205+
206+
if (rateLimiter == null)
207+
rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
208+
209+
return new SnapshotOptions(this);
210+
}
211+
212+
private void validateTag(String tag)
192213
{
193214
if (tag == null || tag.isEmpty())
194-
throw new RuntimeException("You must supply a snapshot name.");
215+
throw new IllegalArgumentException("You must supply a snapshot name.");
216+
217+
if (tag.contains(File.pathSeparator()))
218+
{
219+
throw new IllegalArgumentException("Snapshot name cannot contain " + File.pathSeparator());
220+
}
221+
222+
if (tag.equals(".") || tag.equals(".."))
223+
{
224+
throw new IllegalArgumentException("Snapshot name '" + tag + "' is reserved");
225+
}
195226

227+
if (!CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION.getBoolean())
228+
return;
229+
230+
// Pre-generate snapshot name for the sake of the validation.
231+
// getSnapshotName logic does not return raw "tag" as snapshot name every time,
232+
// it e.g. prepends timestamp and type for system snapshots, and we need to validate it as a whole.
233+
// If, for example, tag would be less than max allowed FILENAME_LENGTH,
234+
// we might in fact produce a snapshot name longer than FILENAME_LENGTH if we prepended a timestamp to it.
235+
String resolvedSnapshotname = SnapshotOptions.getSnapshotName(type, tag, Instant.now());
236+
237+
// the length of valid snapshot name has to be less than or equal to FILENAME_LEGTH - that is 255 -
238+
// we are following the max length as it is in SchemaConstants for table name.
239+
if (resolvedSnapshotname.length() > SchemaConstants.FILENAME_LENGTH)
240+
{
241+
throw new IllegalArgumentException(format("Snapshot name must not be more than %d characters long for " +
242+
"resolved snapshot name (got %d characters for \"%s\")",
243+
FILENAME_LENGTH, resolvedSnapshotname.length(), resolvedSnapshotname));
244+
}
245+
246+
// Allowed characters follow the AWS S3 "Safe characters" set documented at
247+
// https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines :
248+
// 0-9 a-z A-Z ! - _ . * ' ( )
249+
// The path separator '/' is intentionally excluded,
250+
// which is what blocks traversal attempts such as "../../mysnapshot"
251+
if (!SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotname).matches())
252+
{
253+
throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotname);
254+
}
255+
}
256+
257+
private void validateTTL(boolean ephemeral, DurationSpec.IntSecondsBound ttl)
258+
{
196259
if (ttl != null)
197260
{
198261
int minAllowedTtlSecs = CassandraRelevantProperties.SNAPSHOT_MIN_ALLOWED_TTL_SECONDS.getInt();
@@ -202,11 +265,6 @@ public SnapshotOptions build()
202265

203266
if (ephemeral && ttl != null)
204267
throw new IllegalStateException(format("can not take ephemeral snapshot (%s) while ttl is specified too", tag));
205-
206-
if (rateLimiter == null)
207-
rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
208-
209-
return new SnapshotOptions(this);
210268
}
211269
}
212270

‎src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ public Map<ColumnFamilyStore, TableSnapshot> getSnapshotsToCreate()
8888
else
8989
creationTime = options.creationTime;
9090

91-
snapshotName = options.getSnapshotName(creationTime);
91+
snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, creationTime);
9292

9393
Set<ColumnFamilyStore> entitiesForSnapshot = options.cfs == null ? parseEntitiesForSnapshot(options.entities) : Set.of(options.cfs);
9494

‎test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java‎

Lines changed: 99 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,119 @@
2626

2727
import org.junit.Test;
2828

29+
import org.apache.cassandra.config.CassandraRelevantProperties;
30+
import org.apache.cassandra.distributed.shared.WithProperties;
31+
import org.apache.cassandra.io.util.File;
32+
2933
import static java.lang.String.format;
34+
import static org.apache.cassandra.service.snapshot.SnapshotType.USER;
35+
import static org.assertj.core.api.Assertions.assertThatCode;
36+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
3037
import static org.junit.Assert.assertEquals;
3138

3239
public class SnapshotOptionsTest
3340
{
41+
@Test
42+
public void testSnapshotNameValidation()
43+
{
44+
String sep = File.pathSeparator();
45+
46+
try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, true))
47+
{
48+
// Previously-allowed alphanumerics, '-' and '_' must still be accepted.
49+
validate("atag", USER);
50+
validate("a-tag", USER);
51+
validate("a_tag", USER);
52+
validate("a_tag" + Instant.now().toEpochMilli(), USER);
53+
validate("a_tag_1and_something2-more", USER);
54+
validate("a".repeat(255), USER);
55+
56+
// AWS S3 "Safe characters" newly accepted by the relaxed allowlist:
57+
// ! . * ' ( )
58+
// See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines
59+
validate("snap.2026-05-20", USER);
60+
validate("important!", USER);
61+
validate("backup*", USER);
62+
validate("o'snap", USER);
63+
validate("snap(1)", USER);
64+
validate("!._-*'()", USER);
65+
// Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory.
66+
validate("a..tag", USER);
67+
68+
assertThatThrownBy(() -> validate("a".repeat(256), USER))
69+
.isInstanceOf(IllegalArgumentException.class)
70+
.hasMessage("Snapshot name must not be more than 255 characters long for " +
71+
"resolved snapshot name (got 256 characters for \"" + "a".repeat(256) + "\")");
72+
73+
// this would append timestamp + type which would violate < 255 when tag is 250 chars long only
74+
assertThatThrownBy(() -> validate("a".repeat(250), SnapshotType.UPGRADE))
75+
.isInstanceOf(IllegalArgumentException.class)
76+
.hasMessageContaining("Snapshot name must not be more than 255 characters long");
77+
78+
// '/' is not in the S3-safe set; this is what kills traversal attempts like "../../mysnapshot".
79+
assertThatThrownBy(() -> validate('a' + sep + "tag", USER))
80+
.isInstanceOf(IllegalArgumentException.class)
81+
.hasMessage("Snapshot name cannot contain " + sep);
82+
83+
// Other characters outside the S3-safe set must still be rejected.
84+
assertThatThrownBy(() -> validate("a tag", USER))
85+
.isInstanceOf(IllegalArgumentException.class)
86+
.hasMessage("Snapshot name contains illegal characters: a tag");
87+
assertThatThrownBy(() -> validate("a:tag", USER))
88+
.isInstanceOf(IllegalArgumentException.class)
89+
.hasMessage("Snapshot name contains illegal characters: a:tag");
90+
91+
// "." and ".." pass the charset check but resolve to the snapshots/ dir itself
92+
// and its parent (the live table dir) respectively, so they must be rejected as reserved.
93+
assertThatThrownBy(() -> validate(".", USER))
94+
.isInstanceOf(IllegalArgumentException.class)
95+
.hasMessage("Snapshot name '.' is reserved");
96+
97+
assertThatThrownBy(() -> validate("..", USER))
98+
.isInstanceOf(IllegalArgumentException.class)
99+
.hasMessage("Snapshot name '..' is reserved");
100+
}
101+
102+
try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false))
103+
{
104+
// Previously-rejected characters are now accepted: space, ':', and other non-S3-safe chars.
105+
assertThatCode(() -> validate("a tag", USER)).doesNotThrowAnyException();
106+
assertThatCode(() -> validate("a:tag", USER)).doesNotThrowAnyException();
107+
108+
assertThatCode(() -> validate("a".repeat(256), USER)).doesNotThrowAnyException();
109+
assertThatCode(() -> validate("a".repeat(250), SnapshotType.UPGRADE)).doesNotThrowAnyException();
110+
111+
// Path separator and "." / ".." rejections are unconditional — they guard against
112+
// traversal regardless of the toggle.
113+
assertThatThrownBy(() -> validate('a' + sep + "tag", USER))
114+
.isInstanceOf(IllegalArgumentException.class)
115+
.hasMessage("Snapshot name cannot contain " + sep);
116+
assertThatThrownBy(() -> validate(".", USER))
117+
.isInstanceOf(IllegalArgumentException.class)
118+
.hasMessage("Snapshot name '.' is reserved");
119+
assertThatThrownBy(() -> validate("..", USER))
120+
.isInstanceOf(IllegalArgumentException.class)
121+
.hasMessage("Snapshot name '..' is reserved");
122+
}
123+
}
124+
125+
private void validate(String tag, SnapshotType type)
126+
{
127+
new SnapshotOptions.Builder(tag, type, s -> true, "ks.tb").rateLimiter(RateLimiter.create(1)).build();
128+
}
129+
34130
@Test
35131
public void testSnapshotName()
36132
{
37-
List<SnapshotType> sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, SnapshotType.USER);
133+
List<SnapshotType> sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, USER);
38134

39135
for (SnapshotType type : sameNameTypes)
40136
{
41137
SnapshotOptions options = SnapshotOptions.systemSnapshot("a_name", type, "ks.tb")
42138
.rateLimiter(RateLimiter.create(5))
43139
.build();
44140

45-
String snapshotName = options.getSnapshotName(Instant.now());
141+
String snapshotName = SnapshotOptions.getSnapshotName(type, options.tag, Instant.now());
46142
assertEquals("a_name", snapshotName);
47143
}
48144

@@ -57,7 +153,7 @@ public void testSnapshotName()
57153

58154
Instant now = Instant.now();
59155

60-
String snapshotName = options.getSnapshotName(now);
156+
String snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, now);
61157

62158
assertEquals(format("%d-%s-%s", now.toEpochMilli(), type.label, "a_name"), snapshotName);
63159
}

0 commit comments

Comments
 (0)