Fix Read/Write key pattern selector (CVE-2024-51741) (#1514)

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. Signed-off-by: Madelyn Olson <[email protected]> Co-authored-by: YaacovHazan <[email protected]>
valkey-io · Jan 8, 2025 · bc1680d · bc1680d
1 parent 6101248
commit bc1680d
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 4 deletions.
diff --git a/src/acl.c b/src/acl.c
@@ -1046,19 +1046,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
@@ -116,11 +116,32 @@ start_server {tags {"acl external:skip"}} {
         assert_match "*NOPERM*key*" $err
     }
 
-    test {Validate read and write permissions format} {
+    test {Validate read and write permissions format - empty permission} {
         catch {r ACL SETUSER key-permission-RW %~} err
         set err
     } {ERR Error in ACL SETUSER modifier '%~': Syntax error}
 
+    test {Validate read and write permissions format - empty selector} {
+        catch {r ACL SETUSER key-permission-RW %} err
+        set err
+    } {ERR Error in ACL SETUSER modifier '%': Syntax error}
+
+    test {Validate read and write permissions format - empty pattern} {
+        # Empty pattern results with R/W access to no key
+        r ACL SETUSER key-permission-RW on nopass %RW~ +@all
+        $r2 auth key-permission-RW password
+        catch {$r2 SET x 5} err
+        set err
+    } {NOPERM No permissions to access a key}
+
+    test {Validate read and write permissions format - no pattern} {
+        # No pattern results with R/W access to no key (currently we accept this syntax error)
+        r ACL SETUSER key-permission-RW on nopass %RW +@all
+        $r2 auth key-permission-RW password
+        catch {$r2 SET x 5} err
+        set err
+    } {NOPERM No permissions to access a 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)"
         $r2 auth key-permission-RW-selector password