diff --git a/src/acl.c b/src/acl.c index 6eb6c4bcf..de67b890d 100644 --- a/src/acl.c +++ b/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; } diff --git a/tests/unit/acl-v2.tcl b/tests/unit/acl-v2.tcl index 3e6d88f6f..866353d9a 100644 --- a/tests/unit/acl-v2.tcl +++ b/tests/unit/acl-v2.tcl @@ -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 }