diff --git a/src/acl.c b/src/acl.c index 2cd729e77..4c43add14 100644 --- a/src/acl.c +++ b/src/acl.c @@ -94,6 +94,9 @@ void ACLResetSubcommandsForCommand(user *u, unsigned long id); void ACLResetSubcommands(user *u); void ACLAddAllowedSubcommand(user *u, unsigned long id, const char *sub); +/* The length of the string representation of a hashed password. */ +#define HASH_PASSWORD_LEN SHA256_BLOCK_SIZE*2 + /* ============================================================================= * Helper functions for the rest of the ACL implementation * ==========================================================================*/ @@ -145,7 +148,7 @@ int time_independent_strcmp(char *a, char *b) { sds ACLHashPassword(unsigned char *cleartext, size_t len) { SHA256_CTX ctx; unsigned char hash[SHA256_BLOCK_SIZE]; - char hex[SHA256_BLOCK_SIZE*2]; + char hex[HASH_PASSWORD_LEN]; char *cset = "0123456789abcdef"; sha256_init(&ctx); @@ -156,7 +159,7 @@ sds ACLHashPassword(unsigned char *cleartext, size_t len) { hex[j*2] = cset[((hash[j]&0xF0)>>4)]; hex[j*2+1] = cset[(hash[j]&0xF)]; } - return sdsnewlen(hex,SHA256_BLOCK_SIZE*2); + return sdsnewlen(hex,HASH_PASSWORD_LEN); } /* ============================================================================= @@ -522,7 +525,7 @@ sds ACLDescribeUser(user *u) { listRewind(u->passwords,&li); while((ln = listNext(&li))) { sds thispass = listNodeValue(ln); - res = sdscatlen(res,">",1); + res = sdscatlen(res,"#",1); res = sdscatsds(res,thispass); res = sdscatlen(res," ",1); } @@ -649,7 +652,14 @@ void ACLAddAllowedSubcommand(user *u, unsigned long id, const char *sub) { * > Add this password to the list of valid password for the user. * For example >mypass will add "mypass" to the list. * This directive clears the "nopass" flag (see later). + * # Add this password hash to the list of valid hashes for + * the user. This is useful if you have previously computed + * the hash, and don't want to store it in plaintext. + * This directive clears the "nopass" flag (see later). * < Remove this password from the list of valid passwords. + * ! Remove this hashed password from the list of valid passwords. + * This is useful when you want to remove a password just by + * hash without knowing its plaintext version at all. * nopass All the set passwords of the user are removed, and the user * is flagged as requiring no password: it means that every * password will work against this user. If this directive is @@ -685,6 +695,7 @@ void ACLAddAllowedSubcommand(user *u, unsigned long id, const char *sub) { * EEXIST: You are adding a key pattern after "*" was already added. This is * 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. */ int ACLSetUser(user *u, const char *op, ssize_t oplen) { if (oplen == -1) oplen = strlen(op); @@ -720,8 +731,30 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen) { } else if (!strcasecmp(op,"resetpass")) { u->flags &= ~USER_FLAG_NOPASS; listEmpty(u->passwords); - } else if (op[0] == '>') { - sds newpass = ACLHashPassword((unsigned char*)op+1,oplen-1); + } else if (op[0] == '>' || op[0] == '#') { + sds newpass; + if (op[0] == '>') { + newpass = ACLHashPassword((unsigned char*)op+1,oplen-1); + } else { + if (oplen != HASH_PASSWORD_LEN + 1) { + errno = EBADMSG; + return C_ERR; + } + + /* Password hashes can only be characters that represent + * hexadecimal values, which are numbers and lowercase + * characters 'a' through 'f'. + */ + for(int i = 1; i < HASH_PASSWORD_LEN + 1; i++) { + char c = op[i]; + if ((c < 'a' || c > 'f') && (c < '0' || c > '9')) { + errno = EBADMSG; + return C_ERR; + } + } + newpass = sdsnewlen(op+1,oplen-1); + } + listNode *ln = listSearchKey(u->passwords,newpass); /* Avoid re-adding the same password multiple times. */ if (ln == NULL) @@ -729,8 +762,17 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen) { else sdsfree(newpass); u->flags &= ~USER_FLAG_NOPASS; - } else if (op[0] == '<') { - sds delpass = ACLHashPassword((unsigned char*)op+1,oplen-1); + } else if (op[0] == '<' || op[0] == '!') { + sds delpass; + if (op[0] == '<') { + delpass = ACLHashPassword((unsigned char*)op+1,oplen-1); + } else { + if (oplen != HASH_PASSWORD_LEN + 1) { + errno = EBADMSG; + return C_ERR; + } + delpass = sdsnewlen(op+1,oplen-1); + } listNode *ln = listSearchKey(u->passwords,delpass); sdsfree(delpass); if (ln) { @@ -848,6 +890,9 @@ char *ACLSetUserStringError(void) { else if (errno == ENODEV) errmsg = "The password you are trying to remove from the user does " "not exist"; + else if (errno == EBADMSG) + errmsg = "The password hash must be exactly 64 characters and contain " + "only lowercase hexadecimal characters"; return errmsg; } diff --git a/tests/support/test.tcl b/tests/support/test.tcl index 6f02f2f12..2646acecd 100644 --- a/tests/support/test.tcl +++ b/tests/support/test.tcl @@ -15,6 +15,12 @@ proc assert {condition} { } } +proc assert_no_match {pattern value} { + if {[string match $pattern $value]} { + error "assertion:Expected '$value' to not match '$pattern'" + } +} + proc assert_match {pattern value} { if {![string match $pattern $value]} { error "assertion:Expected '$value' to match '$pattern'" diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index 058441433..2205d2d86 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -35,6 +35,32 @@ start_server {tags {"acl"}} { set e } {*WRONGPASS*} + test {Test password hashes can be added} { + r ACL setuser newuser #34344e4d60c2b6d639b7bd22e18f2b0b91bc34bf0ac5f9952744435093cfb4e6 + catch {r AUTH newuser passwd4} e + assert {$e eq "OK"} + } + + test {Test password hashes validate input} { + # Validate Length + catch {r ACL setuser newuser #34344e4d60c2b6d639b7bd22e18f2b0b91bc34bf0ac5f9952744435093cfb4e} e + # Validate character outside set + catch {r ACL setuser newuser #34344e4d60c2b6d639b7bd22e18f2b0b91bc34bf0ac5f9952744435093cfb4eq} e + set e + } {*Error in ACL SETUSER modifier*} + + test {ACL GETUSER returns the password hash instead of the actual password} { + set passstr [dict get [r ACL getuser newuser] passwords] + assert_match {*34344e4d60c2b6d639b7bd22e18f2b0b91bc34bf0ac5f9952744435093cfb4e6*} $passstr + assert_no_match {*passwd4*} $passstr + } + + test {Test hashed passwords removal} { + r ACL setuser newuser !34344e4d60c2b6d639b7bd22e18f2b0b91bc34bf0ac5f9952744435093cfb4e6 + set passstr [dict get [r ACL getuser newuser] passwords] + assert_no_match {*34344e4d60c2b6d639b7bd22e18f2b0b91bc34bf0ac5f9952744435093cfb4e6*} $passstr + } + test {By default users are not able to access any command} { catch {r SET foo bar} e set e @@ -67,7 +93,7 @@ start_server {tags {"acl"}} { set e } {*NOPERM*} - test {ACLs can include or excluse whole classes of commands} { + test {ACLs can include or exclude whole classes of commands} { r ACL setuser newuser -@all +@set +acl r SADD myset a b c; # Should not raise an error r ACL setuser newuser +@all -@string