Protected configs and sensitive commands (#9920)

Block sensitive configs and commands by default.

* `enable-protected-configs` - block modification of configs with the new `PROTECTED_CONFIG` flag.
   Currently we add this flag to `dbfilename`, and `dir` configs,
   all of which are non-mutable configs that can set a file redis will write to.
* `enable-debug-command` - block the `DEBUG` command
* `enable-module-command` - block the `MODULE` command

These have a default value set to `no`, so that these features are not
exposed by default to client connections, and can only be set by modifying the config file.

Users can change each of these to either `yes` (allow all access), or `local` (allow access from
local TCP connections and unix domain connections)

Note that this is a **breaking change** (specifically the part about MODULE command being disabled by default).
I.e. we don't consider DEBUG command being blocked as an issue (people shouldn't have been using it),
and the few configs we protected are unlikely to have been set at runtime anyway.
On the other hand, it's likely to assume some users who use modules, load them from the config file anyway.
Note that's the whole point of this PR, for redis to be more secure by default and reduce the attack surface on
innocent users, so secure defaults will necessarily mean a breaking change.
This commit is contained in:
YaacovHazan 2021-12-19 10:46:16 +02:00 committed by GitHub
parent 5df070ba39
commit ae2f5b7b2e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 148 additions and 31 deletions

View File

@ -15,7 +15,7 @@ jobs:
- name: Build
run: make REDIS_CFLAGS=-Werror
- name: Start redis-server
run: ./src/redis-server --daemonize yes --logfile external-redis.log
run: ./src/redis-server --daemonize yes --logfile external-redis.log --enable-protected-configs yes --enable-debug-command yes --enable-module-command yes
- name: Run external test
run: |
./runtest \
@ -36,7 +36,7 @@ jobs:
- name: Build
run: make REDIS_CFLAGS=-Werror
- name: Start redis-server
run: ./src/redis-server --cluster-enabled yes --daemonize yes --logfile external-redis.log
run: ./src/redis-server --cluster-enabled yes --daemonize yes --logfile external-redis.log --enable-protected-configs yes --enable-debug-command yes --enable-module-command yes
- name: Create a single node cluster
run: ./src/redis-cli cluster addslots $(for slot in {0..16383}; do echo $slot; done); sleep 5
- name: Run external test
@ -51,4 +51,3 @@ jobs:
with:
name: test-external-cluster-log
path: external-redis.log

View File

@ -110,6 +110,29 @@ bind 127.0.0.1 -::1
# even if no authentication is configured.
protected-mode yes
# Redis uses default hardened security configuration directives to reduce the
# attack surface on innocent users. Therefore, several sensitive configuration
# directives are immutable, and some potentially-dangerous commands are blocked.
#
# Configuration directives that control files that Redis writes to (e.g., 'dir'
# and 'dbfilename') and that aren't usually modified during runtime
# are protected by making them immutable.
#
# Commands that can increase the attack surface of Redis and that aren't usually
# called by users are blocked by default.
#
# These can be exposed to either all connections or just local ones by setting
# each of the configs listed below to either of these values:
#
# no - Block for any connection (remain immutable)
# yes - Allow for any connection (no protection)
# local - Allow only for local local connections. Ones originating from the
# IPv4 address (127.0.0.1), IPv6 address (::1) or Unix domain sockets.
#
# enable-protected-configs no
# enable-debug-command no
# enable-module-command no
# Accept connections on the specified port, default is 6379 (IANA #815344).
# If port 0 is specified Redis will not listen on a TCP socket.
port 6379

View File

@ -4148,8 +4148,8 @@ struct redisCommandArg MODULE_UNLOAD_Args[] = {
struct redisCommand MODULE_Subcommands[] = {
{"help","Show helpful text about the different subcommands","O(1)","5.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,MODULE_HELP_History,MODULE_HELP_Hints,moduleCommand,2,CMD_LOADING|CMD_STALE,0},
{"list","List all modules loaded by the server","O(N) where N is the number of loaded modules.","4.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,MODULE_LIST_History,MODULE_LIST_Hints,moduleCommand,2,CMD_ADMIN|CMD_NOSCRIPT,0},
{"load","Load a module","O(1)","4.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,MODULE_LOAD_History,MODULE_LOAD_Hints,moduleCommand,-3,CMD_ADMIN|CMD_NOSCRIPT,0,.args=MODULE_LOAD_Args},
{"unload","Unload a module","O(1)","4.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,MODULE_UNLOAD_History,MODULE_UNLOAD_Hints,moduleCommand,3,CMD_ADMIN|CMD_NOSCRIPT,0,.args=MODULE_UNLOAD_Args},
{"load","Load a module","O(1)","4.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,MODULE_LOAD_History,MODULE_LOAD_Hints,moduleCommand,-3,CMD_ADMIN|CMD_NOSCRIPT|CMD_PROTECTED,0,.args=MODULE_LOAD_Args},
{"unload","Unload a module","O(1)","4.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,MODULE_UNLOAD_History,MODULE_UNLOAD_Hints,moduleCommand,3,CMD_ADMIN|CMD_NOSCRIPT|CMD_PROTECTED,0,.args=MODULE_UNLOAD_Args},
{0}
};
@ -6431,7 +6431,7 @@ struct redisCommand redisCommandTable[] = {
{"command","Get array of Redis command details","O(N) where N is the total number of Redis commands","2.8.13",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,COMMAND_History,COMMAND_Hints,commandCommand,-1,CMD_RANDOM|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_CONNECTION,.subcommands=COMMAND_Subcommands},
{"config","A container for server configuration commands","Depends on subcommand.","2.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,CONFIG_History,CONFIG_Hints,NULL,-2,0,0,.subcommands=CONFIG_Subcommands},
{"dbsize","Return the number of keys in the selected database","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,DBSIZE_History,DBSIZE_Hints,dbsizeCommand,1,CMD_READONLY|CMD_FAST,ACL_CATEGORY_KEYSPACE},
{"debug","A container for debugging commands","Depends on subcommand.","1.0.0",CMD_DOC_SYSCMD,NULL,NULL,COMMAND_GROUP_SERVER,DEBUG_History,DEBUG_Hints,debugCommand,-2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE,0},
{"debug","A container for debugging commands","Depends on subcommand.","1.0.0",CMD_DOC_SYSCMD,NULL,NULL,COMMAND_GROUP_SERVER,DEBUG_History,DEBUG_Hints,debugCommand,-2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_PROTECTED,0},
{"failover","Start a coordinated failover between this server and one of its replicas.","O(1)","6.2.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,FAILOVER_History,FAILOVER_Hints,failoverCommand,-1,CMD_ADMIN|CMD_NOSCRIPT|CMD_STALE,0,.args=FAILOVER_Args},
{"flushall","Remove all keys from all databases","O(N) where N is the total number of keys in all databases","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,FLUSHALL_History,FLUSHALL_Hints,flushallCommand,-1,CMD_WRITE,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_DANGEROUS,.args=FLUSHALL_Args},
{"flushdb","Remove all keys from the current database","O(N) where N is the number of keys in the selected database","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,FLUSHDB_History,FLUSHDB_Hints,flushdbCommand,-1,CMD_WRITE,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_DANGEROUS,.args=FLUSHDB_Args},

View File

@ -13,7 +13,8 @@
"ADMIN",
"NOSCRIPT",
"LOADING",
"STALE"
"STALE",
"PROTECTED"
]
}
}

View File

@ -9,7 +9,8 @@
"function": "moduleCommand",
"command_flags": [
"ADMIN",
"NOSCRIPT"
"NOSCRIPT",
"PROTECTED"
],
"arguments": [
{

View File

@ -9,7 +9,8 @@
"function": "moduleCommand",
"command_flags": [
"ADMIN",
"NOSCRIPT"
"NOSCRIPT",
"PROTECTED"
],
"arguments": [
{

View File

@ -128,6 +128,13 @@ configEnum sanitize_dump_payload_enum[] = {
{NULL, 0}
};
configEnum protected_action_enum[] = {
{"no", PROTECTED_ACTION_ALLOWED_NO},
{"yes", PROTECTED_ACTION_ALLOWED_YES},
{"local", PROTECTED_ACTION_ALLOWED_LOCAL},
{NULL, 0}
};
/* Output buffer limits presets. */
clientBufferLimitsConfig clientBufferLimitsDefaults[CLIENT_TYPE_OBUF_COUNT] = {
{0, 0, 0}, /* normal */
@ -255,6 +262,7 @@ typedef struct standardConfig {
#define DEBUG_CONFIG (1ULL<<2) /* Values that are useful for debugging. */
#define MULTI_ARG_CONFIG (1ULL<<3) /* This config receives multiple arguments. */
#define HIDDEN_CONFIG (1ULL<<4) /* This config is hidden in `config get <pattern>` (used for tests/debugging) */
#define PROTECTED_CONFIG (1ULL<<5) /* Becomes immutable if enable-protected-configs is enabled. */
standardConfig configs[];
@ -711,9 +719,11 @@ void configSetCommand(client *c) {
}
if (!invalid_args) {
if (config->flags & IMMUTABLE_CONFIG) {
if (config->flags & IMMUTABLE_CONFIG ||
(config->flags & PROTECTED_CONFIG && !allowProtectedAction(server.enable_protected_configs, c)))
{
/* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */
errstr = "can't set immutable config";
errstr = (config->flags & IMMUTABLE_CONFIG) ? "can't set immutable config" : "can't set protected config";
err_arg_name = c->argv[2+i*2]->ptr;
invalid_args = 1;
}
@ -2552,6 +2562,11 @@ static sds getConfigReplicaOfOption(typeData data) {
return sdsnew(buf);
}
int allowProtectedAction(int config, client *c) {
return (config == PROTECTED_ACTION_ALLOWED_YES) ||
(config == PROTECTED_ACTION_ALLOWED_LOCAL && islocalClient(c));
}
standardConfig configs[] = {
/* Bool configs */
createBoolConfig("rdbchecksum", NULL, IMMUTABLE_CONFIG, server.rdb_checksum, 1, NULL, NULL),
@ -2607,7 +2622,7 @@ standardConfig configs[] = {
createStringConfig("cluster-announce-ip", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.cluster_announce_ip, NULL, NULL, updateClusterIp),
createStringConfig("cluster-config-file", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.cluster_configfile, "nodes.conf", NULL, NULL),
createStringConfig("syslog-ident", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.syslog_ident, "redis", NULL, NULL),
createStringConfig("dbfilename", NULL, MODIFIABLE_CONFIG, ALLOW_EMPTY_STRING, server.rdb_filename, "dump.rdb", isValidDBfilename, NULL),
createStringConfig("dbfilename", NULL, MODIFIABLE_CONFIG | PROTECTED_CONFIG, ALLOW_EMPTY_STRING, server.rdb_filename, "dump.rdb", isValidDBfilename, NULL),
createStringConfig("appendfilename", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.aof_filename, "appendonly.aof", isValidAOFfilename, NULL),
createStringConfig("server_cpulist", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.server_cpulist, NULL, NULL, NULL),
createStringConfig("bio_cpulist", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.bio_cpulist, NULL, NULL, NULL),
@ -2632,6 +2647,9 @@ standardConfig configs[] = {
createEnumConfig("oom-score-adj", NULL, MODIFIABLE_CONFIG, oom_score_adj_enum, server.oom_score_adj, OOM_SCORE_ADJ_NO, NULL, updateOOMScoreAdj),
createEnumConfig("acl-pubsub-default", NULL, MODIFIABLE_CONFIG, acl_pubsub_default_enum, server.acl_pubsub_default, USER_FLAG_ALLCHANNELS, NULL, NULL),
createEnumConfig("sanitize-dump-payload", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, sanitize_dump_payload_enum, server.sanitize_dump_payload, SANITIZE_DUMP_NO, NULL, NULL),
createEnumConfig("enable-protected-configs", NULL, IMMUTABLE_CONFIG, protected_action_enum, server.enable_protected_configs, PROTECTED_ACTION_ALLOWED_NO, NULL, NULL),
createEnumConfig("enable-debug-command", NULL, IMMUTABLE_CONFIG, protected_action_enum, server.enable_debug_cmd, PROTECTED_ACTION_ALLOWED_NO, NULL, NULL),
createEnumConfig("enable-module-command", NULL, IMMUTABLE_CONFIG, protected_action_enum, server.enable_module_cmd, PROTECTED_ACTION_ALLOWED_NO, NULL, NULL),
/* Integer configs */
createIntConfig("databases", NULL, IMMUTABLE_CONFIG, 1, INT_MAX, server.dbnum, 16, INTEGER_CONFIG, NULL, NULL),
@ -2734,7 +2752,7 @@ standardConfig configs[] = {
#endif
/* Special configs */
createSpecialConfig("dir", NULL, MODIFIABLE_CONFIG, setConfigDirOption, getConfigDirOption, rewriteConfigDirOption, NULL),
createSpecialConfig("dir", NULL, MODIFIABLE_CONFIG | PROTECTED_CONFIG, setConfigDirOption, getConfigDirOption, rewriteConfigDirOption, NULL),
createSpecialConfig("save", NULL, MODIFIABLE_CONFIG | MULTI_ARG_CONFIG, setConfigSaveOption, getConfigSaveOption, rewriteConfigSaveOption, NULL),
createSpecialConfig("client-output-buffer-limit", NULL, MODIFIABLE_CONFIG | MULTI_ARG_CONFIG, setConfigClientOutputBufferLimitOption, getConfigClientOutputBufferLimitOption, rewriteConfigClientOutputBufferLimitOption, NULL),
createSpecialConfig("oom-score-adj-values", NULL, MODIFIABLE_CONFIG | MULTI_ARG_CONFIG, setConfigOOMScoreAdjValuesOption, getConfigOOMScoreAdjValuesOption, rewriteConfigOOMScoreAdjValuesOption, updateOOMScoreAdj),

View File

@ -1026,6 +1026,18 @@ int clientHasPendingReplies(client *c) {
}
}
/* Return true if client connected from loopback interface */
int islocalClient(client *c) {
/* unix-socket */
if (c->flags & CLIENT_UNIX_SOCKET) return 1;
/* tcp */
char cip[NET_IP_STR_LEN+1] = { 0 };
connPeerToString(c->conn, cip, sizeof(cip)-1, NULL);
return !strcmp(cip,"127.0.0.1") || !strcmp(cip,"::1");
}
void clientAcceptHandler(connection *conn) {
client *c = connGetPrivateData(conn);
@ -1042,13 +1054,9 @@ void clientAcceptHandler(connection *conn) {
* requests from non loopback interfaces. Instead we try to explain the
* user what to do to fix it if needed. */
if (server.protected_mode &&
DefaultUser->flags & USER_FLAG_NOPASS &&
!(c->flags & CLIENT_UNIX_SOCKET))
DefaultUser->flags & USER_FLAG_NOPASS)
{
char cip[NET_IP_STR_LEN+1] = { 0 };
connPeerToString(conn, cip, sizeof(cip)-1, NULL);
if (strcmp(cip,"127.0.0.1") && strcmp(cip,"::1")) {
if (!islocalClient(c)) {
char *err =
"-DENIED Redis is running in protected mode because protected "
"mode is enabled and no password is set for the default user. "

View File

@ -3210,6 +3210,21 @@ int processCommand(client *c) {
return C_OK;
}
/* Check if the command is marked as protected and the relevant configuration allows it */
if (c->cmd->flags & CMD_PROTECTED) {
if ((c->cmd->proc == debugCommand && !allowProtectedAction(server.enable_debug_cmd, c)) ||
(c->cmd->proc == moduleCommand && !allowProtectedAction(server.enable_module_cmd, c)))
{
rejectCommandFormat(c,"%s command not allowed. If the %s option is set to \"local\","
"you can run it from a local connection, otherwise you need to set this option "
"in the configuration file, and then restart the server.",
c->cmd->proc == debugCommand ? "DEBUG" : "MODULE",
c->cmd->proc == debugCommand ? "enable-debug-command" : "enable-module-command");
return C_OK;
}
}
int is_read_command = (c->cmd->flags & CMD_READONLY) ||
(c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_READONLY));
int is_write_command = (c->cmd->flags & CMD_WRITE) ||

View File

@ -203,9 +203,10 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT];
#define CMD_SENTINEL (1ULL<<17)
#define CMD_ONLY_SENTINEL (1ULL<<18)
#define CMD_NO_MANDATORY_KEYS (1ULL<<19)
#define CMD_PROTECTED (1ULL<<20)
/* Command flags used by the module system. */
#define CMD_MODULE_GETKEYS (1ULL<<20) /* Use the modules getkeys interface. */
#define CMD_MODULE_NO_CLUSTER (1ULL<<21) /* Deny on Redis Cluster. */
#define CMD_MODULE_GETKEYS (1ULL<<21) /* Use the modules getkeys interface. */
#define CMD_MODULE_NO_CLUSTER (1ULL<<22) /* Deny on Redis Cluster. */
/* Command flags that describe ACLs categories. */
#define ACL_CATEGORY_KEYSPACE (1ULL<<0)
@ -437,6 +438,11 @@ typedef enum {
#define SANITIZE_DUMP_YES 1
#define SANITIZE_DUMP_CLIENTS 2
/* Enable protected config/command */
#define PROTECTED_ACTION_ALLOWED_NO 0
#define PROTECTED_ACTION_ALLOWED_YES 1
#define PROTECTED_ACTION_ALLOWED_LOCAL 2
/* Sets operations codes */
#define SET_OP_UNION 0
#define SET_OP_DIFF 1
@ -1408,6 +1414,9 @@ struct redisServer {
int io_threads_do_reads; /* Read and parse from IO threads? */
int io_threads_active; /* Is IO threads currently active? */
long long events_processed_while_blocked; /* processEventsWhileBlocked() */
int enable_protected_configs; /* Enable the modification of protected configs, see PROTECTED_ACTION_ALLOWED_* */
int enable_debug_cmd; /* Enable DEBUG commands, see PROTECTED_ACTION_ALLOWED_* */
int enable_module_cmd; /* Enable MODULE commands, see PROTECTED_ACTION_ALLOWED_* */
/* RDB / AOF loading information */
volatile sig_atomic_t loading; /* We are loading data from disk if true */
@ -2339,6 +2348,7 @@ int handleClientsWithPendingWritesUsingThreads(void);
int handleClientsWithPendingReadsUsingThreads(void);
int stopThreadedIOIfNeeded(void);
int clientHasPendingReplies(client *c);
int islocalClient(client *c);
int updateClientMemUsage(client *c);
void updateClientMemUsageBucket(client *c);
void unlinkClient(client *c);
@ -2799,6 +2809,7 @@ void rewriteConfigMarkAsProcessed(struct rewriteConfigState *state, const char *
int rewriteConfig(char *path, int force_all);
void initConfigValues();
sds getConfigDebugInfo();
int allowProtectedAction(int config, client *c);
/* db.c -- Keyspace access API */
int removeExpire(redisDb *db, robj *key);

View File

@ -25,3 +25,7 @@ appendonly no
appendfsync everysec
no-appendfsync-on-rewrite no
activerehashing yes
enable-protected-configs yes
enable-debug-command yes
enable-module-command yes

View File

@ -908,6 +908,16 @@ proc delete_lines_with_pattern {filename tmpfilename pattern} {
file rename -force $tmpfilename $filename
}
proc get_nonloopback_addr {} {
set addrlist [list {}]
catch { set addrlist [exec hostname -I] }
return [lindex $addrlist 0]
}
proc get_nonloopback_client {} {
return [redis [get_nonloopback_addr] [srv 0 "port"] 0 $::tls]
}
# The following functions and variables are used only when running large-memory
# tests. We avoid defining them when not running large-memory tests because the
# global variables takes up lots of memory.

View File

@ -201,6 +201,12 @@ start_server {tags {"introspection"}} {
cluster-port
oom-score-adj
oom-score-adj-values
enable-protected-configs
enable-debug-command
enable-module-command
dbfilename
logfile
dir
}
if {!$::tls} {
@ -431,3 +437,27 @@ start_server {tags {"introspection"}} {
# Config file at this point is at a weird state, and includes all
# known keywords. Might be a good idea to avoid adding tests here.
}
start_server {tags {"introspection external:skip"} overrides {enable-protected-configs {no} enable-debug-command {no}}} {
test {cannot modify protected configuration - no} {
assert_error "ERR*protected*" {r config set dir somedir}
assert_error "ERR*DEBUG command not allowed*" {r DEBUG HELP}
} {} {needs:debug}
}
start_server {config "minimal.conf" tags {"introspection external:skip"} overrides {protected-mode {no} enable-protected-configs {local} enable-debug-command {local}}} {
test {cannot modify protected configuration - local} {
# verify that for local connection it doesn't error
r config set dbfilename somename
r DEBUG HELP
# Get a non-loopback address of this instance for this test.
set myaddr [get_nonloopback_addr]
if {$myaddr != "" && ![string match {127.*} $myaddr]} {
# Non-loopback client should fail
set r2 [get_nonloopback_client]
assert_error "ERR*protected*" {$r2 config set dir somedir}
assert_error "ERR*DEBUG command not allowed*" {$r2 DEBUG HELP}
}
} {} {needs:debug}
}

View File

@ -32,3 +32,9 @@ start_server {tags {"modules"}} {
r module unload test
}
start_server {tags {"modules external:skip"} overrides {enable-module-command no}} {
test {module command disabled} {
assert_error "ERR*MODULE command not allowed*" {r module load $testmodule}
}
}

View File

@ -130,16 +130,6 @@ start_server {config "minimal.conf" tags {"external:skip"}} {
r ping
} {PONG}
proc get_nonloopback_addr {} {
set addrlist [list {}]
catch { set addrlist [exec hostname -I] }
return [lindex $addrlist 0]
}
proc get_nonloopback_client {} {
return [redis [get_nonloopback_addr] [srv 0 "port"] 0 $::tls]
}
test {Protected mode works as expected} {
# Get a non-loopback address of this instance for this test.
set myaddr [get_nonloopback_addr]