mirror of
https://codeberg.org/redict/redict.git
synced 2025-01-21 23:58:51 -05:00
Fix Read/Write key pattern selector (CVE-2024-51741)
The explanation on the original commit was wrong. Key based access must have a `~` in order to correctly configure whey key prefixes to apply the selector to. If this is missing, a server assert will be triggered later. Cherry-picked and squashed from relevant Valkey changes. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Drew DeVault <sir@cmpwn.com> Co-authored-by: YaacovHazan <yaacov.hazan@redis.com>
This commit is contained in:
parent
a8edd3f6ac
commit
ba5dcb3b16
11
src/acl.c
11
src/acl.c
@ -1068,19 +1068,24 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) {
|
||||
int flags = 0;
|
||||
size_t offset = 1;
|
||||
if (op[0] == '%') {
|
||||
int perm_ok = 1;
|
||||
for (; offset < oplen; offset++) {
|
||||
if (toupper(op[offset]) == 'R' && !(flags & ACL_READ_PERMISSION)) {
|
||||
flags |= ACL_READ_PERMISSION;
|
||||
} else if (toupper(op[offset]) == 'W' && !(flags & ACL_WRITE_PERMISSION)) {
|
||||
flags |= ACL_WRITE_PERMISSION;
|
||||
} else if (op[offset] == '~' && flags) {
|
||||
} else if (op[offset] == '~') {
|
||||
offset++;
|
||||
break;
|
||||
} else {
|
||||
errno = EINVAL;
|
||||
return C_ERR;
|
||||
perm_ok = 0;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (!flags || !perm_ok) {
|
||||
errno = EINVAL;
|
||||
return C_ERR;
|
||||
}
|
||||
} else {
|
||||
flags = ACL_ALL_PERMISSION;
|
||||
}
|
||||
|
@ -123,9 +123,23 @@ start_server {tags {"acl external:skip"}} {
|
||||
}
|
||||
|
||||
test {Validate read and write permissions format} {
|
||||
catch {r ACL SETUSER key-permission-RW %~} err
|
||||
set err
|
||||
} {ERR Error in ACL SETUSER modifier '%~': Syntax error}
|
||||
# Regression tests for CVE-2024-51741
|
||||
assert_error "ERR Error in ACL SETUSER modifier '%~': Syntax error" {r ACL SETUSER invalid %~}
|
||||
assert_error "ERR Error in ACL SETUSER modifier '%': Syntax error" {r ACL SETUSER invalid %}
|
||||
}
|
||||
|
||||
test {Validate key permissions format - empty and omitted pattern} {
|
||||
# Empty pattern results with access to only the empty key
|
||||
r ACL SETUSER key-permission-no-key on nopass %RW~ +@all
|
||||
assert_equal "User key-permission-no-key has no permissions to access the 'x' key" [r ACL DRYRUN key-permission-no-key GET x]
|
||||
assert_equal "OK" [r ACL DRYRUN key-permission-no-key GET ""]
|
||||
# This is incorrect syntax, it should have `~`, but we'll allow it for compatibility since it does something
|
||||
r ACL SETUSER key-permission-omit on nopass %RW +@all
|
||||
assert_equal "User key-permission-omit has no permissions to access the 'x' key" [r ACL DRYRUN key-permission-omit GET x]
|
||||
assert_equal "OK" [r ACL DRYRUN key-permission-omit GET ""]
|
||||
# Assert these two are equivalent
|
||||
assert_equal [r ACL GETUSER key-permission-omit] [r ACL GETUSER key-permission-no-key]
|
||||
}
|
||||
|
||||
test {Test separate read and write permissions on different selectors are not additive} {
|
||||
r ACL SETUSER key-permission-RW-selector on nopass "(%R~read* +@all)" "(%W~write* +@all)"
|
||||
@ -467,43 +481,43 @@ start_server {tags {"acl external:skip"}} {
|
||||
test {Test sort with ACL permissions} {
|
||||
r set v1 1
|
||||
r lpush mylist 1
|
||||
|
||||
r ACL setuser test-sort-acl on nopass (+sort ~mylist)
|
||||
|
||||
r ACL setuser test-sort-acl on nopass (+sort ~mylist)
|
||||
$r2 auth test-sort-acl nopass
|
||||
|
||||
catch {$r2 sort mylist by v*} e
|
||||
assert_equal "ERR BY option of SORT denied due to insufficient ACL permissions." $e
|
||||
catch {$r2 sort mylist get v*} e
|
||||
assert_equal "ERR GET option of SORT denied due to insufficient ACL permissions." $e
|
||||
|
||||
r ACL setuser test-sort-acl (+sort ~mylist ~v*)
|
||||
catch {$r2 sort mylist by v*} e
|
||||
assert_equal "ERR BY option of SORT denied due to insufficient ACL permissions." $e
|
||||
catch {$r2 sort mylist get v*} e
|
||||
assert_equal "ERR GET option of SORT denied due to insufficient ACL permissions." $e
|
||||
|
||||
r ACL setuser test-sort-acl (+sort ~mylist %W~*)
|
||||
|
||||
catch {$r2 sort mylist by v*} e
|
||||
assert_equal "ERR BY option of SORT denied due to insufficient ACL permissions." $e
|
||||
catch {$r2 sort mylist get v*} e
|
||||
assert_equal "ERR GET option of SORT denied due to insufficient ACL permissions." $e
|
||||
|
||||
r ACL setuser test-sort-acl (+sort ~mylist %R~*)
|
||||
assert_equal "1" [$r2 sort mylist by v*]
|
||||
|
||||
|
||||
r ACL setuser test-sort-acl (+sort ~mylist ~v*)
|
||||
catch {$r2 sort mylist by v*} e
|
||||
assert_equal "ERR BY option of SORT denied due to insufficient ACL permissions." $e
|
||||
catch {$r2 sort mylist get v*} e
|
||||
assert_equal "ERR GET option of SORT denied due to insufficient ACL permissions." $e
|
||||
|
||||
r ACL setuser test-sort-acl (+sort ~mylist %W~*)
|
||||
catch {$r2 sort mylist by v*} e
|
||||
assert_equal "ERR BY option of SORT denied due to insufficient ACL permissions." $e
|
||||
catch {$r2 sort mylist get v*} e
|
||||
assert_equal "ERR GET option of SORT denied due to insufficient ACL permissions." $e
|
||||
|
||||
r ACL setuser test-sort-acl (+sort ~mylist %R~*)
|
||||
assert_equal "1" [$r2 sort mylist by v*]
|
||||
|
||||
# cleanup
|
||||
r ACL deluser test-sort-acl
|
||||
r del v1 mylist
|
||||
}
|
||||
|
||||
|
||||
test {Test DRYRUN with wrong number of arguments} {
|
||||
r ACL setuser test-dry-run +@all ~v*
|
||||
|
||||
|
||||
assert_equal "OK" [r ACL DRYRUN test-dry-run SET v v]
|
||||
|
||||
|
||||
catch {r ACL DRYRUN test-dry-run SET v} e
|
||||
assert_equal "ERR wrong number of arguments for 'set' command" $e
|
||||
|
||||
|
||||
catch {r ACL DRYRUN test-dry-run SET} e
|
||||
assert_equal "ERR wrong number of arguments for 'set' command" $e
|
||||
}
|
||||
|
Loading…
Reference in New Issue
Block a user