gvsafronov/futriix
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

Fixed a bug where sequential matching ACL rules weren't compressed (#12472)

Browse Source 
When adding a new ACL rule was added, an attempt was made to remove
any "overlapping" rules. However, there when a match was found, the search
was not resumed at the right location, but instead after the original position of
the original command.

For example, if the current rules were `-config +config|get` and a rule `+config`
was added. It would identify that `-config` was matched, but it would skip over
`+config|get`, leaving the compacted rule `-config +config`. This would be evaluated
safely, but looks weird.

This bug can only be triggered with subcommands, since that is the only way to
have sequential matching rules. Resolves #12470. This is also only present in 7.2.
I think there was also a minor risk of removing another valid rule, since it would start
the search of the next command at an arbitrary point. I couldn't find a valid offset that
would have cause a match using any of the existing commands that have subcommands
with another command.
This commit is contained in:
Madelyn Olson 2023-08-09 23:58:53 -07:00 committed by GitHub
parent 6abfda54c3
commit 7c179f9bf4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 7 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/acl.c
View File

@ -563,7 +563,7 @@ void ACLSelectorRemoveCommandRule(aclSelector *selector, sds new_rule) {
* as well if the command is removed. */ * as well if the command is removed. */
char *rule_end = strchr(existing_rule, ' '); char *rule_end = strchr(existing_rule, ' ');
if (!rule_end) { if (!rule_end) {
/* This is the last rule, so it it to the end of the string. */ /* This is the last rule, so move it to the end of the string. */
rule_end = existing_rule + strlen(existing_rule); rule_end = existing_rule + strlen(existing_rule);
/* This approach can leave a trailing space if the last rule is removed, /* This approach can leave a trailing space if the last rule is removed,
@ -580,6 +580,8 @@ void ACLSelectorRemoveCommandRule(aclSelector *selector, sds new_rule) {
/* Copy the remaining rules starting at the next rule to replace the rule to be /* Copy the remaining rules starting at the next rule to replace the rule to be
* deleted, including the terminating NULL character. */ * deleted, including the terminating NULL character. */
memmove(copy_position, copy_end, strlen(copy_end) + 1); memmove(copy_position, copy_end, strlen(copy_end) + 1);
existing_rule = copy_position;
continue;
} }
} }
existing_rule = copy_end; existing_rule = copy_end;

4
tests/unit/acl.tcl
View File

@ -615,6 +615,10 @@ start_server {tags {"acl external:skip"}} {
# Unnecessary categories are retained for potentional future compatibility # Unnecessary categories are retained for potentional future compatibility
r ACL SETUSER adv-test -@all -@dangerous r ACL SETUSER adv-test -@all -@dangerous
assert_equal "-@all -@dangerous" [dict get [r ACL getuser adv-test] commands] assert_equal "-@all -@dangerous" [dict get [r ACL getuser adv-test] commands]
# Duplicate categories are compressed, regression test for #12470
r ACL SETUSER adv-test -@all +config +config|get -config|set +config
assert_equal "-@all +config" [dict get [r ACL getuser adv-test] commands]
} }
test "ACL CAT with illegal arguments" { test "ACL CAT with illegal arguments" {