From 08fa2783794a1b59a405a951a7efb30d0127a313 Mon Sep 17 00:00:00 2001
From: Madelyn Olson <madelyneolson@gmail.com>
Date: Mon, 6 Jan 2025 14:02:16 -0800
Subject: [PATCH] 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 <madelyneolson@gmail.com>
Co-authored-by: YaacovHazan <yaacov.hazan@redis.com>
---
 src/acl.c             | 11 ++++++++---
 tests/unit/acl-v2.tcl | 23 ++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/acl.c b/src/acl.c
index 04e8c7d1e..d0795f389 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -1074,19 +1074,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 a509bc0ff..d1f7ba1b7 100644
--- 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