Check the SORT destination that is actually written

[madolson]

When the server works out which keys a SORT touches, it re-examines the STORE destination as if that key name were an option keyword. A destination named by, get or limit therefore swallows the arguments after it and hides a later STORE clause, and one named store is read as another STORE clause and reports whatever follows it. Because SORT writes to the last STORE, the server ends up checking permissions against one key and writing to a different one, so a user can write to a key outside their allowed patterns. The fix advances the scan past the destination so a key name is never re-read as an option.

Details

Problem

sortGetKeys() (src/db.c:2879) walks SORT's arguments looking for STORE, because the destination is the one key that can appear anywhere in the argument list. Options that take arguments are stepped over using a table:

    } skiplist[] = {
        {"limit", 2}, {"get", 1}, {"by", 1}, {NULL, 0} /* End of elements. */
    };

When the walk found STORE it recorded the destination position and left the inner loop without moving the cursor (src/db.c:2884-2891):

            } else if (!strcasecmp(objectGetVal(argv[i]), "store") && i + 1 < argc) {
                /* Note: we don't increment "num" here and continue the loop
                 * to be sure to process the *last* "STORE" option if multiple
                 * ones are provided. This is same behavior as SORT. */
                found_store = 1;
                keys[num].pos = i + 1; /* <store-key> */
                keys[num].flags = CMD_KEY_OW | CMD_KEY_UPDATE;
                break;

i is still on the STORE token, so the next turn of for (i = 2; i < argc; i++) lands on the destination name and tests it against the skiplist and against "store". Two shapes follow:

• A destination named by or get consumes one further argument, limit consumes two. When a second STORE clause is inside that window, the walk never sees it.
• A destination named store matches the "store" test itself, so the next token is recorded as the destination instead. This needs no second STORE at all.

The executor has neither problem. sortCommandGeneric() (src/sort.c) steps past the destination, and it deliberately keeps the last STORE rather than the first:

        } else if (readonly == 0 && !strcasecmp(objectGetVal(c->argv[j]), "store") && leftargs >= 1) {
            storekey = c->argv[j + 1];
            j++;

So the key the server authorizes and the key it writes are two different keys.

Impact

Key permissions are applied to the reported key, not the real destination. Two shapes, both against an unpatched server:

> acl setuser u on >p ~allowed:* ~get +@all
  OK
> rpush allowed:src c b a
  3
> (as u) sort allowed:src ALPHA STORE get STORE forbidden:dst
  3
> exists forbidden:dst
  1
> lrange forbidden:dst 0 -1
  a
  b
  c

> acl setuser u on >p ~allowed:* ~alpha +@all
  OK
> (as u) sort allowed:src STORE store alpha
  3
> exists store
  1
> acl dryrun u sort allowed:src STORE store alpha
  OK

forbidden:dst and store are both outside ~allowed:*. The decoys (~get, ~alpha) are granted only so that the decoy itself passes, which is what leaves the real destination as the sole reason to reject the command. After the fix both sequences return NOPERM No permissions to access a key, EXISTS is 0, and ACL DRYRUN names the right key: User u has no permissions to access the 'store' key.

The same key list feeds getNodeByQuery() (src/cluster.c:984). When the decoy hashes to the source's slot the cross-slot check passes and the destination is added to the source's slot index instead of its own, leaving a key KEYS lists but EXISTS cannot find:

> cluster keyslot {get}src
  5175
> cluster keyslot get
  5175
> cluster keyslot {b}dst
  3300
> rpush {get}src c b a
  3
> command getkeys sort {get}src ALPHA STORE get STORE {b}dst
  {get}src
  get
> sort {get}src ALPHA STORE get STORE {b}dst
  3
> keys *
  {get}src
  {b}dst
> exists {b}dst
  0
> cluster countkeysinslot 3300
  0
> cluster countkeysinslot 5175
  2

With the fix that command is rejected with CROSSSLOT Keys in request don't hash to the same slot.

SORT_RO is unaffected: it rejects STORE at execution and sortROGetKeys() never scans for it.

Behavior

Destination reported by COMMAND GETKEYS sort src <args>, measured against both builds, next to the key SORT actually creates:

args after SORT src	reported before	reported after	actually written
STORE dst	dst	dst	dst
STORE a STORE dst	dst	dst	dst
STORE by STORE dst	by	dst	dst
STORE get STORE dst	get	dst	dst
STORE limit STORE dst	limit	dst	dst
STORE store alpha	alpha	store	store
STORE STORE alpha	alpha	STORE	STORE
STORE store by w_*	by	store	store
STORE by	by	by	by
BY w_* GET p_* STORE dst	dst	dst	dst
LIMIT 0 5 STORE dst	dst	dst	dst
STORE	none	none	syntax error

The destination names that derail the walk are exactly the tokens it recognizes: by, get, limit and store, in any case.

Why the existing tests missed it

tests/unit/sort.tcl:119 already covers repeated STORE:

    test "SORT extracts multiple STORE correctly" {
        r command getkeys sort abc store invalid store stillbad store def
    } {abc def}

invalid and stillbad are not tokens the walk recognizes, so the misplaced cursor lands on a word it ignores and the walk recovers on the next argument. The assertion only ever used destinations that cannot trigger the bug.

Fix

One statement, in the store branch:

                /* Skip the destination. It is a key name, so it must never be
                 * examined as an option: a key that spells one would hide the
                 * later STORE clause that SORT actually writes to. */
                i++;

num is still not incremented and found_store is untouched, so last-one-wins is preserved and the reported key count is unchanged. The branch is already guarded by i + 1 < argc, so the extra increment leaves i <= argc - 1, and keys[num].pos <= argc - 1 always. georadiusGetKeys() (src/db.c:2974) already does exactly this after recording its own STORE/STOREDIST destination.

The store branch of sortGetKeys() differs across release branches only in argv[i]->ptr (8.0 4bf1df644, 8.1 55a542671, 9.0 a100149d5) versus objectGetVal(argv[i]) (9.1 7f1dffedf, unstable da91ccd12), which the added line does not touch, so the hunk applies to all of them unchanged.

Alternative considered

Have sortGetKeys() share sortCommandGeneric()'s argument walk so the two cannot drift apart again. That is the right long-term shape, but it is a large change to a function that has to cherry-pick into four release branches, and the two walks legitimately differ: the key extractor must not validate LIMIT's integers and must not reject BY patterns in cluster mode, and it is called from inside the ACL check (ACLSelectorCheckCmd(), src/acl.c:1968) that sortCommandGeneric() itself depends on for its BY/GET decision. Worth doing separately on unstable.

Rejecting a repeated STORE as a syntax error would also close this and is arguably the cleaner command definition, but it is a user-visible behavior change to a shipped command and cannot be backported.

Breaking out of the outer loop on the first STORE is shorter than skipping the destination, but it reports the first destination instead of the last, which is the same bug mirrored: the server would authorize a key it does not write.

Decisions for a reviewer

1. Backport to 8.0, 8.1, 9.0 and 9.1. Proposal: yes, all four, unchanged.
2. Whether the cluster wrong-slot insert needs its own test. Proposal: no. Same root cause and same one-line fix, and it would cost a cluster topology for no additional coverage of the changed line.
3. Whether repeated STORE should become a syntax error. Proposal: separate issue, not this patch.

Testing

With src/db.c reverted to da91ccd12 and both new tests kept in the tree:

*** [err]: SORT extracts STORE correctly when the destination is named like an option in tests/unit/sort.tcl
Expected 'abc def' to be equal to 'abc by' (context: type eval line 6 cmd {assert_equal {abc def} [r command getkeys sort abc store $keyword store def]} proc ::test)

*** [err]: Test SORT STORE destination that is named like an option in tests/unit/acl-v2.tcl
Expected 'User test-sort-store has no permissions to access the 'forbidden:dst' key' to be equal to 'OK' (context: type eval line 14 cmd {assert_equal "User test-sort-store has no permissions to access the 'forbidden:dst' key"  [r ACL DRYRUN test-sort-store SORT allowed:src ALPHA STORE $keyword STORE forbidden:dst]} proc ::test)

Each assertion fails on its own, not just the first. On the unpatched build the store shapes report abc alpha, abc alpha and abc by where the test expects abc store, abc STORE and abc store, and SORT allowed:src STORE store alpha returns 3 with EXISTS store at 1.

Completeness is the part worth checking for this one, since a partial fix would leave a smaller version of the same hole. Enumerating every option sequence up to length 5 over {STORE, store, by, get, limit, alpha, dst, w_*, 0} against real servers, and comparing the reported destination to the key the server actually creates:

=== BASE, up to length 5 ===
commands executed OK : 4816
commands that wrote  : 3402
MISMATCHES           : 636
  SORT src STORE STORE alpha -> reported 'alpha', wrote 'STORE'
  SORT src STORE store alpha -> reported 'alpha', wrote 'store'
  SORT src store STORE alpha -> reported 'alpha', wrote 'STORE'
  SORT src store store alpha -> reported 'alpha', wrote 'store'
  SORT src STORE STORE by STORE -> reported 'by', wrote 'STORE'
  SORT src STORE STORE by store -> reported 'by', wrote 'STORE'
  SORT src STORE STORE by by -> reported 'by', wrote 'STORE'
  SORT src STORE STORE by get -> reported 'by', wrote 'STORE'

=== FIXED, up to length 5 ===
commands executed OK : 4816
commands that wrote  : 3402
MISMATCHES           : 0

4816 and 3402 being identical on both builds is the other half: the same commands succeed and write to the same keys, so this only changes what gets reported.

The ACL test lives in acl-v2.tcl beside the existing Test sort with ACL permissions, and uses its own client because the cleanup ACL deluser kills whichever client is authenticated as that user.

This was generated by AI but verified, with love, by a human.