From 064d807db3ab0f9d8ba6a29d1c950720de9d3485 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Mon, 15 Feb 2021 17:08:53 +0200 Subject: [PATCH] Escape unsafe field name characters in INFO. (#8492) Fixes #8489 --- src/module.c | 7 +++++-- src/server.c | 27 +++++++++++++++++++++++++-- src/server.h | 1 + src/util.c | 27 +++++++++++++++++++++++++++ src/util.h | 2 ++ tests/integration/redis-cli.tcl | 2 +- tests/modules/infotest.c | 5 +++++ tests/unit/info.tcl | 8 ++++++++ tests/unit/moduleapi/infotest.tcl | 5 +++++ 9 files changed, 79 insertions(+), 5 deletions(-) diff --git a/src/module.c b/src/module.c index f8d3e3170..5282bd742 100644 --- a/src/module.c +++ b/src/module.c @@ -6759,10 +6759,13 @@ int RM_InfoBeginDictField(RedisModuleInfoCtx *ctx, char *name) { /* Implicitly end dicts, instead of returning an error which is likely un checked. */ if (ctx->in_dict_field) RM_InfoEndDictField(ctx); + char *tmpmodname, *tmpname; ctx->info = sdscatfmt(ctx->info, "%s_%s:", - ctx->module->name, - name); + getSafeInfoString(ctx->module->name, strlen(ctx->module->name), &tmpmodname), + getSafeInfoString(name, strlen(name), &tmpname)); + if (tmpmodname != NULL) zfree(tmpmodname); + if (tmpname != NULL) zfree(tmpname); ctx->in_dict_field = 1; return REDISMODULE_OK; } diff --git a/src/server.c b/src/server.c index bc0eb8255..3f3ac3517 100644 --- a/src/server.c +++ b/src/server.c @@ -4501,6 +4501,25 @@ void bytesToHuman(char *s, unsigned long long n) { } } +/* Characters we sanitize on INFO output to maintain expected format. */ +static char unsafe_info_chars[] = "#:\n\r"; +static char unsafe_info_chars_substs[] = "____"; /* Must be same length as above */ + +/* Returns a sanitized version of s that contains no unsafe info string chars. + * If no unsafe characters are found, simply returns s. Caller needs to + * free tmp if it is non-null on return. + */ +const char *getSafeInfoString(const char *s, size_t len, char **tmp) { + *tmp = NULL; + if (mempbrk(s, len, unsafe_info_chars,sizeof(unsafe_info_chars)-1) + == NULL) return s; + char *new = *tmp = zmalloc(len + 1); + memcpy(new, s, len); + new[len] = '\0'; + return memmapchars(new, len, unsafe_info_chars, unsafe_info_chars_substs, + sizeof(unsafe_info_chars)-1); +} + /* Create the string returned by the INFO command. This is decoupled * by the INFO command itself as we need to report the same information * on memory corruption problems. */ @@ -5126,15 +5145,17 @@ sds genRedisInfoString(const char *section) { dictIterator *di; di = dictGetSafeIterator(server.commands); while((de = dictNext(di)) != NULL) { + char *tmpsafe; c = (struct redisCommand *) dictGetVal(de); if (!c->calls && !c->failed_calls && !c->rejected_calls) continue; info = sdscatprintf(info, "cmdstat_%s:calls=%lld,usec=%lld,usec_per_call=%.2f" ",rejected_calls=%lld,failed_calls=%lld\r\n", - c->name, c->calls, c->microseconds, + getSafeInfoString(c->name, strlen(c->name), &tmpsafe), c->calls, c->microseconds, (c->calls == 0) ? 0 : ((float)c->microseconds/c->calls), c->rejected_calls, c->failed_calls); + if (tmpsafe != NULL) zfree(tmpsafe); } dictReleaseIterator(di); } @@ -5147,10 +5168,12 @@ sds genRedisInfoString(const char *section) { raxSeek(&ri,"^",NULL,0); struct redisError *e; while(raxNext(&ri)) { + char *tmpsafe; e = (struct redisError *) ri.data; info = sdscatprintf(info, "errorstat_%.*s:count=%lld\r\n", - (int)ri.key_len, ri.key, e->count); + (int)ri.key_len, getSafeInfoString((char *) ri.key, ri.key_len, &tmpsafe), e->count); + if (tmpsafe != NULL) zfree(tmpsafe); } raxStop(&ri); } diff --git a/src/server.h b/src/server.h index 7140c4571..a9841886d 100644 --- a/src/server.h +++ b/src/server.h @@ -2685,6 +2685,7 @@ void _serverPanic(const char *file, int line, const char *msg, ...); #endif void serverLogObjectDebugInfo(const robj *o); void sigsegvHandler(int sig, siginfo_t *info, void *secret); +const char *getSafeInfoString(const char *s, size_t len, char **tmp); sds genRedisInfoString(const char *section); sds genModulesInfoString(sds info); void enableWatchdog(int period); diff --git a/src/util.c b/src/util.c index 3243fa51e..8087c8b7a 100644 --- a/src/util.c +++ b/src/util.c @@ -244,6 +244,33 @@ long long memtoll(const char *p, int *err) { return val*mul; } +/* Search a memory buffer for any set of bytes, like strpbrk(). + * Returns pointer to first found char or NULL. + */ +const char *mempbrk(const char *s, size_t len, const char *chars, size_t charslen) { + for (size_t j = 0; j < len; j++) { + for (size_t n = 0; n < charslen; n++) + if (s[j] == chars[n]) return &s[j]; + } + + return NULL; +} + +/* Modify the buffer replacing all occurrences of chars from the 'from' + * set with the corresponding char in the 'to' set. Always returns s. + */ +char *memmapchars(char *s, size_t len, const char *from, const char *to, size_t setlen) { + for (size_t j = 0; j < len; j++) { + for (size_t i = 0; i < setlen; i++) { + if (s[j] == from[i]) { + s[j] = to[i]; + break; + } + } + } + return s; +} + /* Return the number of digits of 'v' when converted to string in radix 10. * See ll2string() for more information. */ uint32_t digits10(uint64_t v) { diff --git a/src/util.h b/src/util.h index feaa82924..3a15c793e 100644 --- a/src/util.h +++ b/src/util.h @@ -49,6 +49,8 @@ int stringmatchlen(const char *p, int plen, const char *s, int slen, int nocase) int stringmatch(const char *p, const char *s, int nocase); int stringmatchlen_fuzz_test(void); long long memtoll(const char *p, int *err); +const char *mempbrk(const char *s, size_t len, const char *chars, size_t charslen); +char *memmapchars(char *s, size_t len, const char *from, const char *to, size_t setlen); uint32_t digits10(uint64_t v); uint32_t sdigits10(int64_t v); int ll2string(char *s, size_t len, long long value); diff --git a/tests/integration/redis-cli.tcl b/tests/integration/redis-cli.tcl index 1e346a9a5..d877542ed 100644 --- a/tests/integration/redis-cli.tcl +++ b/tests/integration/redis-cli.tcl @@ -109,7 +109,7 @@ start_server {tags {"cli"}} { test_interactive_cli "INFO response should be printed raw" { set lines [split [run_command $fd info] "\n"] foreach line $lines { - assert [regexp {^$|^#|^[a-z0-9_]+:.+} $line] + assert [regexp {^$|^#|^[^#:]+:} $line] } } diff --git a/tests/modules/infotest.c b/tests/modules/infotest.c index 4cb77ee87..87a89dcb1 100644 --- a/tests/modules/infotest.c +++ b/tests/modules/infotest.c @@ -21,6 +21,11 @@ void InfoFunc(RedisModuleInfoCtx *ctx, int for_crash_report) { RedisModule_InfoAddFieldLongLong(ctx, "expires", 1); RedisModule_InfoEndDictField(ctx); + RedisModule_InfoAddSection(ctx, "unsafe"); + RedisModule_InfoBeginDictField(ctx, "unsafe:field"); + RedisModule_InfoAddFieldLongLong(ctx, "value", 1); + RedisModule_InfoEndDictField(ctx); + if (for_crash_report) { RedisModule_InfoAddSection(ctx, "Klingon"); RedisModule_InfoAddFieldCString(ctx, "one", "wa’"); diff --git a/tests/unit/info.tcl b/tests/unit/info.tcl index 5a44c0647..08171fff9 100644 --- a/tests/unit/info.tcl +++ b/tests/unit/info.tcl @@ -150,4 +150,12 @@ start_server {tags {"info"}} { assert_match {} [errorstat NOPERM] } } + + start_server {} { + test {Unsafe command names are sanitized in INFO output} { + catch {r host:} e + set info [r info commandstats] + assert_match {*cmdstat_host_:calls=1*} $info + } + } } diff --git a/tests/unit/moduleapi/infotest.tcl b/tests/unit/moduleapi/infotest.tcl index 80a28656c..1ad2ee6fc 100644 --- a/tests/unit/moduleapi/infotest.tcl +++ b/tests/unit/moduleapi/infotest.tcl @@ -85,5 +85,10 @@ start_server {tags {"modules"}} { set keys [scan [regexp -inline {keys\=([\d]*)} $keyspace] keys=%d] } {3} + test {module info unsafe fields} { + set info [r info infotest_unsafe] + assert_match {*infotest_unsafe_field:value=1*} $info + } + # TODO: test crash report. }