Fix ACL OOB for wrong-arity KEYNUM commands#14847
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
sundb
added
the
release-notes
| /* Validate arity before key/channel extraction to avoid touching argv out of bounds. */ | ||
| if (!commandCheckArity(cmd, argc, NULL)) { | ||
| if (idxptr) *idxptr = 0; | ||
| return ACL_DENIED_CMD; |
There was a problem hiding this comment.
do you mean there is also a similar issue here? but i don't see the crash whey i ran the scipting test.
make SANITIZER=address
./runtest --single unit/scripting
There was a problem hiding this comment.
Ah yeah good catch! I think it's because the scripting path has a growing argv buffer so earlier test commands created a larger buffer masking ASan error. Updated the test to use a new server.
The fix in db.c alone is not enough because after the fix, getKeysUsingKeySpecs correctly returns -1, but then getKeysFromCommandWithSpecs will invoke evalGetKeys via cmd->getkeys_proc, causing oob read in genericGetKeys. So the acl.c check is actually preventing the crash. The db.c makes the argv check logic correct.
Without the fix in db.c:
==2279681==ERROR: AddressSanitizer: heap-use-after-free on address 0x503000026ef8 at pc 0x55db5d73d7c3 bp 0x7ffc6f5dc420 sp 0x7ffc6f5dc410
READ of size 8 at 0x503000026ef8 thread T0
#0 0x55db5d73d7c2 in getKeysUsingKeySpecs .../redis/src/db.c:3149
#1 0x55db5d73d7c2 in getKeysUsingKeySpecs .../redis/src/db.c:3091
#2 0x55db5d73dddc in getKeysFromCommandWithSpecs .../redis/src/db.c:3230
#3 0x55db5dabf1db in ACLSelectorCheckCmd .../redis/src/acl.c:1726
#4 0x55db5dac003a in ACLCheckAllUserCommandPerm .../redis/src/acl.c:1894
#5 0x55db5d9fdd1d in RM_ACLCheckCommandPermissions .../redis/src/module.c:10425
With the fix in db.c:
==2273397==ERROR: AddressSanitizer: heap-use-after-free on address 0x503000026ef8 at pc 0x5598e4fb8c0f bp 0x7ffc94ab48e0 sp 0x7ffc94ab48d0
READ of size 8 at 0x503000026ef8 thread T0
#0 0x5598e4fb8c0e in genericGetKeys .../redis/src/db.c:3459
#1 0x5598e4fb8c0e in evalGetKeys .../redis/src/db.c:3501
#2 0x5598e4fbce64 in getKeysFromCommandWithSpecs .../redis/src/db.c:3244
#3 0x5598e533e12b in ACLSelectorCheckCmd .../redis/src/acl.c:1726
#4 0x5598e533ef8a in ACLCheckAllUserCommandPerm .../redis/src/acl.c:1894
#5 0x5598e527cc6d in RM_ACLCheckCommandPermissions .../redis/src/module.c:10425
Thanks for checking!
There was a problem hiding this comment.
not sure this is the right way, my concern is that whether ethere are other paths where an invalid command could be passed into genericGetKeys().
another alternative is to add if (keyCountOfs >= argc) return 0; to genericGetKeys() before touch the argv.
@oranagra please share your thoughts.
There was a problem hiding this comment.
i think it's a good idea to have both.
i.e. if a command arity is wrong, abort early (not sure if ACL_DENIED_CMD is the right return value).
this will reduce many risks due to missing range checks.
but also, it's a good idea to have boundary checks like we did in the other places (which is mostly needed in case some command spec or arity is wrongly defined)
There was a problem hiding this comment.
seem that there isn't a return value better than ACL_DENIED_CMD.
/* Return values for ACLCheckAllPerm(). */
#define ACL_OK 0
#define ACL_DENIED_CMD 1
#define ACL_DENIED_KEY 2
#define ACL_DENIED_AUTH 3 /* Only used for ACL LOG entries. */
#define ACL_DENIED_CHANNEL 4 /* Only used for pub/sub commands */
#define ACL_INVALID_TLS_CERT_AUTH 5 /* Only used for TLS Auto-authentication */
@zzjas PTAL
There was a problem hiding this comment.
Thanks for the suggestion! I added the check in genericGetKeys() as well. Now the tests won't crash due ASan even if the acl.c fix is removed -- but still kept it as suggested for defense in depth.
zzjas
force-pushed
the
fix-acl-arity-check
branch
2 times, most recently
from
b72c311 to
503d152
Compare
zzjas
force-pushed
the
fix-acl-arity-check
branch
from
503d152 to
5187494
Compare
|
@zzjas please also add a similar check in extractSlotFromKeysResult() diff --git a/src/cluster.c b/src/cluster.c
index ea0c67636..4bb2412c8 100644
--- a/src/cluster.c
+++ b/src/cluster.c
@@ -1130,13 +1130,16 @@ void clusterCommand(client *c) {
* - The slot number if all keys belong to the same slot
* - INVALID_CLUSTER_SLOT if there are no keys or cluster is disabled
* - CLUSTER_CROSSSLOT if keys belong to different slots (cross-slot error) */
-int extractSlotFromKeysResult(robj **argv, getKeysResult *keys_result) {
+int extractSlotFromKeysResult(robj **argv, int argc, getKeysResult *keys_result) {
if (keys_result->numkeys == 0 || !server.cluster_enabled)
return INVALID_CLUSTER_SLOT;
int first_slot = INVALID_CLUSTER_SLOT;
for (int j = 0; j < keys_result->numkeys; j++) {
- robj *this_key = argv[keys_result->keys[j].pos];
+ int pos = keys_result->keys[j].pos;
+ if (pos >= argc || pos < 0)
+ continue;
+ robj *this_key = argv[pos];
int this_slot = (int)keyHashSlot((char*)this_key->ptr, sdslen(this_key->ptr));
if (first_slot == INVALID_CLUSTER_SLOT)@tezc PTAL, not sure if it's needed. |
|
@sundb I'm not sure if there in an actual issue leading to extractSlotFromKeysResult() but perhaps we can have that boundary check just to be defensive? |
zzjas
force-pushed
the
fix-acl-arity-check
branch
from
5187494 to
f332363
Compare
|
@sundb added the check in |
zzjas
force-pushed
the
fix-acl-arity-check
branch
2 times, most recently
from
b222cc9 to
468a980
Compare
|
Returning Maybe we should check command arity before calling but |
Add commandCheckArity() in luaRedisAclCheckCmdPermissionsCommand and RM_ACLCheckCommandPermissions before calling ACLCheckAllUserCommandPerm, matching processCommand, scriptCall, and RM_Call. Without this, KEYNUM keyspec commands like EVAL with wrong arity cause out-of-bounds argv access during key extraction. Also fix KEYNUM index calculation (`first + keynumidx`) and add a bounds check in genericGetKeys().
zzjas
force-pushed
the
fix-acl-arity-check
branch
from
468a980 to
d3d1f31
Compare
@ShooterIT I think this makes more sense then making |
ShooterIT
left a comment
ShooterIT
left a comment
There was a problem hiding this comment.
LGTM, please update the top comment
|
@ShooterIT Thanks for the review!
Just to make sure, do you mean the PR description or the comments for |
now we validate command arity before ACL checks
Yes, these are user-aware APIs and should be mentioned when we modify their behavior |
|
Thanks! Updated the descirption. |
sundb
changed the title
sundb
changed the title
|
This bug has existed for a long time, so we only backport it to the latest version. |
`luaRedisAclCheckCmdPermissionsCommand` and `RM_ACLCheckCommandPermissions` now call `commandCheckArity()` to check command arity before calling `ACLCheckAllUserCommandPerm`, matching the behavior of `processCommand`, `scriptCall`, and `RM_Call`. Without this, KEYNUM keyspec commands like EVAL with wrong arity cause out-of-bounds argv access during key extraction. Also fix KEYNUM index calculation (`first + keynumidx`) and add a bounds check in genericGetKeys(). Add scripting and module ACL tests for wrong-arity `EVAL` to lock in the non-crashing behavior. Fixes redis#14843
`luaRedisAclCheckCmdPermissionsCommand` and `RM_ACLCheckCommandPermissions` now call `commandCheckArity()` to check command arity before calling `ACLCheckAllUserCommandPerm`, matching the behavior of `processCommand`, `scriptCall`, and `RM_Call`. Without this, KEYNUM keyspec commands like EVAL with wrong arity cause out-of-bounds argv access during key extraction. Also fix KEYNUM index calculation (`first + keynumidx`) and add a bounds check in genericGetKeys(). Add scripting and module ACL tests for wrong-arity `EVAL` to lock in the non-crashing behavior. Fixes #14843
7 participants
luaRedisAclCheckCmdPermissionsCommandandRM_ACLCheckCommandPermissionsnow callcommandCheckArity()to check command arity before callingACLCheckAllUserCommandPerm, matching the behavior ofprocessCommand,scriptCall, andRM_Call. Without this, KEYNUM keyspec commands like EVAL with wrong arity cause out-of-bounds argv access during key extraction.Also fix KEYNUM index calculation (
first + keynumidx) and add a bounds check in genericGetKeys().Add scripting and module ACL tests for wrong-arity
EVALto lock in the non-crashing behavior.Fixes #14843
Note
Medium Risk
Touches command key-extraction and ACL-check paths used by modules and Lua scripting; mistakes could change authorization behavior or error handling for affected commands.
Overview
Prevents crashes during ACL checks on wrong-arity commands by validating command arity before ACL key/channel extraction in both the module API (
RM_ACLCheckCommandPermissions) and Lua (redis.acl_check_cmd).Hardens key discovery for
KEYNUMkey-specs by computing the key-count index relative to the resolvedfirstargument and guarding against negative/out-of-rangeargvaccess, plus an early bounds check ingenericGetKeyswhen the key-count argument is missing.Adds regression tests ensuring wrong-arity
EVALfails safely (returns an error/denial) in module ACL checks and inredis.acl_check_cmd, rather than crashing.Written by Cursor Bugbot for commit d3d1f31. This will update automatically on new commits. Configure here.