Through randomised testing we've stumbled on this problem. Creating a user through the ACL SETUSER command that contains a quote symbol is accepted if proper escaping is in place
ACL SETUSER 'test"user' on >redacted ~* +@all
AUTH 'test"user' redacted
OK
ACL SAVE however doesn't incorporate proper escaping when rewriting the acl file. Hence subsequent ACL LOAD commands will fail with:
/redacted/users.acl:10: unbalanced quotes in acl line. WARNING: ACL errors detected, no change to the previously active ACL rules was performed"
e.g. user.acl file
# Unbalanced quotes - ACL LOAD fails
user mXLGgUa"ls;1jW on #66992d25551e...
# Unbalanced quotes - ACL LOAD fails
user "mXLGgUa"ls;1jW" on #66992d25551e...
# ACL LOAD will work but strip out quotes at the beginning at the end of the username
user "mXLGgUa\"ls;1jW" on #66992d25551e...
Expectation would be that either username with single or double quotes are forbidden. Or some kind of escaping is in place to allow single and double quotes in usernames.
Comment From: bsergean
I can reproduce this.
(venv) sandbox$ redis-server redis.conf
80183:C 26 May 2020 20:56:24.284 # oO0OoO0OoO0Oo Redis is starting oO0OoO0OoO0Oo
80183:C 26 May 2020 20:56:24.284 # Redis version=6.0.3, bits=64, commit=7803b114, modified=0, pid=80183, just started
80183:C 26 May 2020 20:56:24.284 # Configuration loaded
80183:M 26 May 2020 20:56:24.285 * Increased maximum number of open files to 10032 (it was originally set to 1024).
_._
_.-``__ ''-._
_.-`` `. `_. ''-._ Redis 6.0.3 (7803b114/0) 64 bit
.-`` .-```. ```\/ _.,_ ''-._
( ' , .-` | `, ) Running in standalone mode
|`-._`-...-` __...-.``-._|'` _.-'| Port: 9999
| `-._ `._ / _.-' | PID: 80183
`-._ `-._ `-./ _.-' _.-'
|`-._`-._ `-.__.-' _.-'_.-'|
| `-._`-._ _.-'_.-' | http://redis.io
`-._ `-._`-.__.-'_.-' _.-'
|`-._`-._ `-.__.-' _.-'_.-'|
| `-._`-._ _.-'_.-' |
`-._ `-._`-.__.-'_.-' _.-'
`-._ `-.__.-' _.-'
`-._ _.-'
`-.__.-'
80183:M 26 May 2020 20:56:24.286 # Server initialized
80183:M 26 May 2020 20:56:24.286 # Aborting Redis startup because of ACL errors: users.acl:2: unbalanced quotes in acl line. WARNING: ACL errors detected, no change to the previously active ACL rules was performed
(venv) sandbox$ cat redis.conf
port 9999
aclfile users.acl
Client did
127.0.0.1:9999> ACL SETUSER 'test"user' on >redacted ~* +@all
OK
127.0.0.1:9999> ACL SAVE
OK
What I noticed which is interesting is that the redis-cli history (liblinenoise ?) didn't record the SETUSER command, as if it had failed. Actually I believe this is on purpose to keep 'secrets', and was changed by @itamarhaber.
Comment From: itamarhaber
Totally reproducible, even without testing :)
@bsergean - confirmed, the idea is to avoid saving passwords in the cli's history. This is done for AUTH and ACL SETUSER (although we're still missing similar treatment for MIGRATE).
Comment From: antirez
@ullumullu thanks! I tried to fix it in 1f8ea99b4. I would really appreciate if you could re-fuzz this commit.
Comment From: antirez
Reopening waiting for the OP to reply.
Comment From: ullumullu
Given the fact that spaces are not allowed in usernames and keys I'd say the commit makes sense to me.
Only question for my understanding: Don't think that the unbalanced quotes catch can be triggered after this commit, right? https://github.com/antirez/redis/commit/1f8ea99b4bc25083d909b3228601fb82efe1e67f#diff-5ab4602c36098a9b93707565ade6f721R1302
Comment From: oranagra
seems resolved (by the original commit), please comment if you think otherwise.