Skip to content

Commit

Permalink
MIGTATE with AUTH that contains "keys" is getting wrong key names in …
Browse files Browse the repository at this point in the history
…migrateGetKeys, leads to ACL errors (redis#11253)

When using the MIGRATE, with a destination Redis that has the user name or password set to the string "keys",
Redis would have determine the wrong set of key names the command is gonna access.
This lead to ACL returning wrong authentication result.

Destination instance:
```
127.0.0.1:6380> acl setuser default >keys
OK
127.0.0.1:6380> acl setuser keys on nopass ~* &* +@ALL
OK
```

Source instance:
```
127.0.0.1:6379> set a 123
OK
127.0.0.1:6379> acl setuser cc on nopass ~a* +@ALL
OK
127.0.0.1:6379> auth cc 1
OK
127.0.0.1:6379> migrate 127.0.0.1 6380 "" 0 1000 auth keys keys a
(error) NOPERM this user has no permissions to access one of the keys used as arguments
127.0.0.1:6379> migrate 127.0.0.1 6380 "" 0 1000 auth2 keys pswd keys a
(error) NOPERM this user has no permissions to access one of the keys used as arguments
```

Using `acl dryrun` we know that the parameters of `auth` and `auth2` are mistaken for the `keys` option.
```
127.0.0.1:6379> acl dryrun cc migrate whatever whatever "" 0 1000 auth keys keys a
"This user has no permissions to access the 'keys' key"
127.0.0.1:6379> acl dryrun cc migrate whatever whatever "" 0 1000 auth2 keys pswd keys a
"This user has no permissions to access the 'pswd' key"
```

Fix the bug by editing db.c/migrateGetKeys function, which finds the `keys` option and all the keys following.
  • Loading branch information
CharlesChen888 authored Oct 13, 2022
1 parent dd60c6c commit 9ab873d
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 6 deletions.
32 changes: 26 additions & 6 deletions src/db.c
Original file line number Diff line number Diff line change
Expand Up @@ -2227,7 +2227,7 @@ int sortGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *

/* This command declares incomplete keys, so the flags are correctly set for this function */
int migrateGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result) {
int i, num, first;
int i, j, num, first;
keyReference *keys;
UNUSED(cmd);

Expand All @@ -2236,15 +2236,35 @@ int migrateGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResul
num = 1;

/* But check for the extended one with the KEYS option. */
struct {
char* name;
int skip;
} skip_keywords[] = {
{"copy", 0},
{"replace", 0},
{"auth", 1},
{"auth2", 2},
{NULL, 0}
};
if (argc > 6) {
for (i = 6; i < argc; i++) {
if (!strcasecmp(argv[i]->ptr,"keys") &&
sdslen(argv[3]->ptr) == 0)
{
first = i+1;
num = argc-first;
if (!strcasecmp(argv[i]->ptr, "keys")) {
if (sdslen(argv[3]->ptr) > 0) {
/* This is a syntax error. So ignore the keys and leave
* the syntax error to be handled by migrateCommand. */
num = 0;
} else {
first = i + 1;
num = argc - first;
}
break;
}
for (j = 0; skip_keywords[j].name != NULL; j++) {
if (!strcasecmp(argv[i]->ptr, skip_keywords[j].name)) {
i += skip_keywords[j].skip;
break;
}
}
}
}

Expand Down
9 changes: 9 additions & 0 deletions tests/unit/acl-v2.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,15 @@ start_server {tags {"acl external:skip"}} {
assert_equal "OK" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 KEYS rw]
assert_equal "This user has no permissions to access the 'read' key" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 KEYS read]
assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 KEYS write]
assert_equal "OK" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 AUTH KEYS KEYS rw]
assert_equal "This user has no permissions to access the 'read' key" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 AUTH KEYS KEYS read]
assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 AUTH KEYS KEYS write]
assert_equal "OK" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 AUTH2 KEYS 123 KEYS rw]
assert_equal "This user has no permissions to access the 'read' key" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 AUTH2 KEYS 123 KEYS read]
assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 AUTH2 KEYS 123 KEYS write]
assert_equal "OK" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 AUTH2 USER KEYS KEYS rw]
assert_equal "This user has no permissions to access the 'read' key" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 AUTH2 USER KEYS KEYS read]
assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN command-test MIGRATE whatever whatever "" 0 5000 AUTH2 USER KEYS KEYS write]

# Test SORT, which is marked with incomplete keys
assert_equal "OK" [r ACL DRYRUN command-test SORT read STORE write]
Expand Down

0 comments on commit 9ab873d

Please sign in to comment.