Skip to content

Commit 8f89b26

Browse files
tobiemhtobiemh
authored
fix(sec): stop array element-level SELECT permissions leaking elements (#7356) (#347)
1 parent 11430e2 commit 8f89b26

5 files changed

Lines changed: 152 additions & 16 deletions

File tree

‎language-tests/tests/reproductions/7356_array_element_select_permission_leak.surql‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/**
2+
[test]
3+
reason = "Array element-level SELECT permissions (items[*]) must filter elements by identity. reduce_document / apply_select_field_permissions / filter_fields_by_permission expanded items[*] via Value::each (ascending indices) and cut each denied element with Value::cut, which does Vec::remove(i) — shifting later indices down. Cutting forward therefore invalidated the pending indices: a deny-all leaked the odd-indexed elements ([1,3,5]) and a predicate kept/dropped the wrong elements. Fixed by cutting in reverse (descending) index order at every site."
4+
issue = 7356
5+
6+
[[test.results]]
7+
value = "'OK'"
8+
9+
[[test.results]]
10+
value = "{ id: doc:1, items: [] }"
11+
12+
[[test.results]]
13+
value = "{ id: docp:1, items: [{ n: 0 }, { n: 2 }, { n: 4 }] }"
14+
15+
[env]
16+
imports = ["reproductions/7356_array_element_select_permission_leak_import.surql"]
17+
auth = { namespace = "test", database = "test", access = "user", rid = "person:guest" }
18+
19+
*/
20+
21+
-- Authenticated as record user person:guest.
22+
RETURN 'OK';
23+
24+
-- Deny-all element permission: the whole array must be empty (pre-fix it
25+
-- leaked the odd-indexed elements [{n:1},{n:3},{n:5}]).
26+
SELECT * FROM ONLY doc:1;
27+
28+
-- Per-element predicate: only the even-n elements survive, by identity
29+
-- (pre-fix the index-shift kept {n:3},{n:5} and dropped {n:4}).
30+
SELECT * FROM ONLY docp:1;

‎language-tests/tests/reproductions/7356_array_element_select_permission_leak_import.surql‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
/**
2+
[test]
3+
4+
[[test.results]]
5+
value = "NONE"
6+
7+
[[test.results]]
8+
value = "NONE"
9+
10+
[[test.results]]
11+
value = "NONE"
12+
13+
[[test.results]]
14+
value = "NONE"
15+
16+
[[test.results]]
17+
value = "NONE"
18+
19+
[[test.results]]
20+
value = "NONE"
21+
22+
[[test.results]]
23+
value = "NONE"
24+
25+
[[test.results]]
26+
value = "NONE"
27+
28+
[[test.results]]
29+
value = "NONE"
30+
31+
[[test.results]]
32+
value = "NONE"
33+
34+
[[test.results]]
35+
value = "[{ id: person:guest }]"
36+
37+
[[test.results]]
38+
value = "[{ id: doc:1, items: [{ n: 0 }, { n: 1 }, { n: 2 }, { n: 3 }, { n: 4 }, { n: 5 }] }]"
39+
40+
[[test.results]]
41+
value = "[{ id: docp:1, items: [{ n: 0 }, { n: 1 }, { n: 2 }, { n: 3 }, { n: 4 }, { n: 5 }] }]"
42+
*/
43+
44+
DEFINE ACCESS user ON DATABASE TYPE RECORD
45+
SIGNIN ( SELECT * FROM type::record('person', $id) )
46+
DURATION FOR TOKEN 1h, FOR SESSION 1h;
47+
48+
DEFINE TABLE person SCHEMALESS PERMISSIONS FULL;
49+
50+
-- Table A: element-level SELECT permission that always denies. Every
51+
-- element of `items` must be hidden, leaving an empty array.
52+
DEFINE TABLE doc SCHEMAFULL PERMISSIONS FOR select FULL;
53+
DEFINE FIELD items ON doc TYPE array<object>;
54+
DEFINE FIELD OVERWRITE items[*] ON doc TYPE object PERMISSIONS FOR select WHERE false;
55+
DEFINE FIELD items[*].n ON doc TYPE int;
56+
57+
-- Table B: a genuine per-element predicate. Only even `n` survive, by
58+
-- element identity — NOT by (post-removal, shifted) index.
59+
DEFINE TABLE docp SCHEMAFULL PERMISSIONS FOR select FULL;
60+
DEFINE FIELD items ON docp TYPE array<object>;
61+
DEFINE FIELD OVERWRITE items[*] ON docp TYPE object PERMISSIONS FOR select WHERE $value.n % 2 == 0;
62+
DEFINE FIELD items[*].n ON docp TYPE int;
63+
64+
CREATE person:guest;
65+
CREATE doc:1 SET items = [{ n: 0 }, { n: 1 }, { n: 2 }, { n: 3 }, { n: 4 }, { n: 5 }];
66+
CREATE docp:1 SET items = [{ n: 0 }, { n: 1 }, { n: 2 }, { n: 3 }, { n: 4 }, { n: 5 }];

‎surrealdb/core/src/doc/output.rs‎

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -323,24 +323,41 @@ impl Document {
323323
if !ctx.check_perms(opt, Action::View)? {
324324
return Ok(());
325325
}
326-
// Loop through all field statements
326+
// Apply each field's select permission to the PROJECTED output.
327+
// `reduce_document` already filtered the stored fields, so this
328+
// post-projection pass must be IDEMPOTENT. SECURITY: read `each`
329+
// and the `$value` pick from a stable snapshot of `out` (not the
330+
// un-reduced `self.current`) so array-element paths stay aligned
331+
// with the array we cut — otherwise a denied element is removed a
332+
// second time at a shifted index, dropping a permitted sibling
333+
// (issue #7356). Cuts accumulate on `out`; the snapshot is cloned
334+
// lazily so tables with no element-bearing permissions pay nothing.
335+
let mut snapshot: Option<Value> = None;
327336
for fd in self.doc_ctx.fd()?.iter() {
328337
// SECURITY: apply the field's AUTH LIMIT before evaluating
329338
// PERMISSIONS FOR select so the predicate runs under the
330339
// definer's downgraded auth, not the caller's. Mirrors the
331340
// pluck.rs / field.rs paths.
332341
let opt = AuthLimit::try_from(&fd.auth_limit)?.limit_opt(opt);
333-
// Loop over each field in document
334-
for k in out.each(&fd.name).iter() {
335-
// Process the field permissions
336-
match &fd.select_permission {
337-
Permission::Full => (),
338-
Permission::None => out.cut(k),
339-
Permission::Specific(e) => {
342+
// Process the field permissions
343+
match &fd.select_permission {
344+
Permission::Full => (),
345+
Permission::None => {
346+
let original = snapshot.get_or_insert_with(|| out.clone());
347+
// SECURITY: iterate in reverse so `Value::cut`'s `Vec::remove`
348+
// shift doesn't invalidate the pending array indices (#7356).
349+
for k in original.each(&fd.name).iter().rev() {
350+
out.cut(k);
351+
}
352+
}
353+
Permission::Specific(e) => {
354+
let original = snapshot.get_or_insert_with(|| out.clone());
355+
// SECURITY: iterate in reverse (see the None arm above, #7356).
356+
for k in original.each(&fd.name).iter().rev() {
340357
// Disable permissions
341358
let opt = &opt.new_with_perms(false);
342-
// Get the current value
343-
let val = Arc::new(self.current.doc.as_ref().pick(k));
359+
// Get the projected value from the snapshot
360+
let val = Arc::new(original.pick(k));
344361
// Configure the context
345362
let mut child_ctx = Context::new_child(ctx);
346363
child_ctx.add_value("value", val);

‎surrealdb/core/src/doc/reduce.rs‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -168,15 +168,23 @@ impl Document {
168168
// PERMISSIONS FOR select so the predicate runs under the
169169
// definer's downgraded auth, not the caller's.
170170
let opt = AuthLimit::try_from(&fd.auth_limit)?.limit_opt(opt);
171+
// SECURITY: `each` yields paths in ascending array-index order, and
172+
// `Value::cut` removes an element via `Vec::remove`, which shifts
173+
// every later index down. Cutting in forward order would make each
174+
// removal invalidate the remaining indices, leaking the odd-indexed
175+
// elements (issue #7356). Iterate in reverse so a higher index is
176+
// always removed before any lower index that is still pending.
177+
// Predicates read from `original` (immutable), so evaluation order
178+
// is irrelevant.
171179
match &fd.select_permission {
172180
Permission::Full => (),
173181
Permission::None => {
174-
for k in original.doc.as_ref().each(&fd.name).iter() {
182+
for k in original.doc.as_ref().each(&fd.name).iter().rev() {
175183
doc.doc.to_mut().cut(k);
176184
}
177185
}
178186
Permission::Specific(e) => {
179-
for k in original.doc.as_ref().each(&fd.name).iter() {
187+
for k in original.doc.as_ref().each(&fd.name).iter().rev() {
180188
// Disable permissions
181189
let opt = &opt.new_with_perms(false);
182190
// Get the computed value
@@ -222,8 +230,14 @@ impl Document {
222230
// definer's downgraded auth, not the caller's. Mirrors the
223231
// pluck.rs / field.rs paths.
224232
let opt = AuthLimit::try_from(&fd.auth_limit)?.limit_opt(opt);
225-
// Loop over each field in document
226-
for k in reduced.as_ref().each(&fd.name).iter() {
233+
// Loop over each field in document. SECURITY: iterate in reverse —
234+
// `each` yields ascending array indices and `Value::cut` removes via
235+
// `Vec::remove` (shifting later indices down), so a forward pass
236+
// would let each removal invalidate the pending indices and leak the
237+
// odd-indexed elements (issue #7356). Removing higher indices first
238+
// keeps the not-yet-processed lower indices valid; predicates read
239+
// from the immutable `full`, so evaluation order is irrelevant.
240+
for k in reduced.as_ref().each(&fd.name).iter().rev() {
227241
// Process the field permissions
228242
match &fd.select_permission {
229243
Permission::Full => (),

‎surrealdb/core/src/exec/operators/scan/pipeline.rs‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -898,13 +898,22 @@ pub(crate) async fn filter_fields_by_permission(
898898
PhysicalPermission::Allow => continue,
899899
PhysicalPermission::Deny => {
900900
let original = snapshot.get_or_insert_with(|| value.clone());
901-
for path in original.each(&idiom.0) {
901+
// SECURITY: iterate in reverse. `each` yields ascending
902+
// array indices and `Value::cut` removes via `Vec::remove`
903+
// (shifting later indices down), so a forward pass would
904+
// let each removal invalidate the pending indices and leak
905+
// the odd-indexed elements (issue #7356). Removing higher
906+
// indices first keeps the pending lower indices valid.
907+
for path in original.each(&idiom.0).into_iter().rev() {
902908
value.cut(&path.0);
903909
}
904910
}
905911
PhysicalPermission::Conditional(_) => {
906912
let original = snapshot.get_or_insert_with(|| value.clone());
907-
for path in original.each(&idiom.0) {
913+
// SECURITY: iterate in reverse (see the Deny arm above and
914+
// issue #7356). Predicates read from `original` (immutable),
915+
// so evaluation order is irrelevant.
916+
for path in original.each(&idiom.0).into_iter().rev() {
908917
let field_value = original.pick(&path.0);
909918
let allowed =
910919
check_permission_for_value(perm, original, Some(&field_value), ctx)

0 commit comments

Comments
 (0)