diff --git a/src/acl.c b/src/acl.c index d4fa2c6c8e..24a9e35b41 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1110,13 +1110,20 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) { struct redisCommand *cmd = ACLLookupCommand(copy); /* Check if the command exists. We can't check the - * subcommand to see if it is valid. */ + * first-arg to see if it is valid. */ if (cmd == NULL) { zfree(copy); errno = ENOENT; return C_ERR; } + /* We do not support allowing first-arg of a subcommand */ + if (cmd->parent) { + zfree(copy); + errno = ECHILD; + return C_ERR; + } + /* The subcommand cannot be empty, so things like DEBUG| * are syntax errors of course. */ if (strlen(sub) == 0) { @@ -1127,7 +1134,7 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) { if (cmd->subcommands_dict) { /* If user is trying to allow a valid subcommand we can just add its unique ID */ - struct redisCommand *cmd = ACLLookupCommand(op+1); + cmd = ACLLookupCommand(op+1); if (cmd == NULL) { zfree(copy); errno = ENOENT; @@ -1138,13 +1145,11 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) { /* If user is trying to use the ACL mech to block SELECT except SELECT 0 or * block DEBUG except DEBUG OBJECT (DEBUG subcommands are not considered * subcommands for now) we use the allowed_firstargs mechanism. */ - struct redisCommand *cmd = ACLLookupCommand(copy); - if (cmd == NULL) { - zfree(copy); - errno = ENOENT; - return C_ERR; - } + /* Add the first-arg to the list of valid ones. */ + serverLog(LL_WARNING, "Deprecation warning: Allowing a first arg of an otherwise " + "blocked command is a misuse of ACL and may get disabled " + "in the future (offender: +%s)", op+1); ACLAddAllowedFirstArg(selector,cmd->id,sub); } @@ -1236,6 +1241,7 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) { * almost surely an error on the user side. * ENODEV: The password you are trying to remove from the user does not exist. * EBADMSG: The hash you are trying to add is not a valid hash. + * ECHILD: Attempt to allow a specific first argument of a subcommand */ int ACLSetUser(user *u, const char *op, ssize_t oplen) { if (oplen == -1) oplen = strlen(op); @@ -1360,6 +1366,8 @@ const char *ACLSetUserStringError(void) { else if (errno == EALREADY) errmsg = "Duplicate user found. A user can only be defined once in " "config files"; + else if (errno == ECHILD) + errmsg = "Allowing first-arg of a subcommand is not supported"; return errmsg; } diff --git a/src/server.c b/src/server.c index 5de6d61c5e..50fc7a9574 100644 --- a/src/server.c +++ b/src/server.c @@ -2812,20 +2812,32 @@ int isContainerCommandBySds(sds s) { return has_subcommands; } -struct redisCommand *lookupCommandLogic(dict *commands, robj **argv, int argc) { +/* Look up a command by argv and argc + * + * If `strict` is not 0 we expect argc to be exact (i.e. argc==2 + * for a subcommand and argc==1 for a top-level command) + * `strict` should be used every time we want to look up a command + * name (e.g. in COMMAND INFO) rather than to find the command + * a user requested to execute (in processCommand). + */ +struct redisCommand *lookupCommandLogic(dict *commands, robj **argv, int argc, int strict) { struct redisCommand *base_cmd = dictFetchValue(commands, argv[0]->ptr); int has_subcommands = base_cmd && base_cmd->subcommands_dict; if (argc == 1 || !has_subcommands) { + if (strict && argc != 1) + return NULL; /* Note: It is possible that base_cmd->proc==NULL (e.g. CONFIG) */ return base_cmd; - } else { + } else { /* argc > 1 && has_subcommands */ + if (strict && argc != 2) + return NULL; /* Note: Currently we support just one level of subcommands */ return dictFetchValue(base_cmd->subcommands_dict, argv[1]->ptr); } } struct redisCommand *lookupCommand(robj **argv, int argc) { - return lookupCommandLogic(server.commands,argv,argc); + return lookupCommandLogic(server.commands,argv,argc,0); } struct redisCommand *lookupCommandBySdsLogic(dict *commands, sds s) { @@ -2846,7 +2858,7 @@ struct redisCommand *lookupCommandBySdsLogic(dict *commands, sds s) { argv[j] = &objects[j]; } - struct redisCommand *cmd = lookupCommandLogic(commands,argv,argc); + struct redisCommand *cmd = lookupCommandLogic(commands,argv,argc,1); sdsfreesplitres(strings,argc); return cmd; } @@ -2876,9 +2888,9 @@ struct redisCommand *lookupCommandByCString(const char *s) { * rewriteClientCommandVector() in order to set client->cmd pointer * correctly even if the command was renamed. */ struct redisCommand *lookupCommandOrOriginal(robj **argv ,int argc) { - struct redisCommand *cmd = lookupCommandLogic(server.commands, argv, argc); + struct redisCommand *cmd = lookupCommandLogic(server.commands, argv, argc, 0); - if (!cmd) cmd = lookupCommandLogic(server.orig_commands, argv, argc); + if (!cmd) cmd = lookupCommandLogic(server.orig_commands, argv, argc, 0); return cmd; } diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index 872bade905..5c55921085 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -327,16 +327,27 @@ start_server {tags {"acl external:skip"}} { set e } {*NOPERM*config|set*} - test {ACLs can include a subcommand with a specific arg} { + test {ACLs cannot include a subcommand with a specific arg} { r ACL setuser newuser +@all -config|get - r ACL setuser newuser +config|get|appendonly - set cmdstr [dict get [r ACL getuser newuser] commands] - assert_match {*-config|get*} $cmdstr - assert_match {*+config|get|appendonly*} $cmdstr - r CONFIG GET appendonly; # Should not fail - catch {r CONFIG GET loglevel} e + catch { r ACL setuser newuser +config|get|appendonly} e set e - } {*NOPERM*config|get*} + } {*Allowing first-arg of a subcommand is not supported*} + + test {ACLs cannot exclude or include a container commands with a specific arg} { + r ACL setuser newuser +@all +config|get + catch { r ACL setuser newuser +@all +config|asdf} e + assert_match "*Unknown command or category name in ACL*" $e + catch { r ACL setuser newuser +@all -config|asdf} e + assert_match "*Unknown command or category name in ACL*" $e + } {} + + test {ACLs cannot exclude or include a container command with two args} { + r ACL setuser newuser +@all +config|get + catch { r ACL setuser newuser +@all +get|key1|key2} e + assert_match "*Unknown command or category name in ACL*" $e + catch { r ACL setuser newuser +@all -get|key1|key2} e + assert_match "*Unknown command or category name in ACL*" $e + } {} test {ACLs including of a type includes also subcommands} { r ACL setuser newuser -@all +acl +@stream diff --git a/tests/unit/introspection-2.tcl b/tests/unit/introspection-2.tcl index 437826f764..457ac6bc4a 100644 --- a/tests/unit/introspection-2.tcl +++ b/tests/unit/introspection-2.tcl @@ -110,4 +110,9 @@ start_server {tags {"introspection"}} { set reply [r command list filterby pattern pf*] assert_equal [lsort $reply] {pfadd pfcount pfdebug pfmerge pfselftest} } + + test {COMMAND INFO of invalid subcommands} { + assert_equal {{}} [r command info get|key] + assert_equal {{}} [r command info config|get|key] + } }