From c09336094846ae720f3828560abdebc4758e8a4c Mon Sep 17 00:00:00 2001 From: Liviu Chircu Date: Wed, 24 Feb 2021 16:44:04 +0200 Subject: [PATCH] Cache DB: Escape all logged URL passwords Additionally: * fix similar security issues in the Mongo and Redis clients * rewrite some cachedb unit tests Suggested by @ChrisZhangJin Related to #2416 (cherry picked from commit ed74875df66219e6e59a65f3f2f9a8a26dfb2e95) --- cachedb/cachedb.c | 2 +- cachedb/cachedb_id.c | 3 +- cachedb/test/test_cachedb.c | 14 +++---- modules/cachedb_mongodb/cachedb_mongodb.c | 2 +- .../cachedb_mongodb/cachedb_mongodb_dbase.c | 10 ++--- modules/cachedb_redis/cachedb_redis.c | 2 +- ut.c | 42 +++++++++++++++++++ ut.h | 9 ++++ 8 files changed, 68 insertions(+), 16 deletions(-) diff --git a/cachedb/cachedb.c b/cachedb/cachedb.c index 72d3554672c..57845b1cde7 100644 --- a/cachedb/cachedb.c +++ b/cachedb/cachedb.c @@ -650,7 +650,7 @@ cachedb_con* cachedb_do_init(str *url,void* (*new_connection)(struct cachedb_id id = new_cachedb_id(url); if (!id) { - LM_ERR("cannot parse url [%.*s]\n",url->len,url->s); + LM_ERR("cannot parse url [%s]\n", db_url_escape(url)); pkg_free(res); return 0; } diff --git a/cachedb/cachedb_id.c b/cachedb/cachedb_id.c index 7c72caff353..4dd3656ffcd 100644 --- a/cachedb/cachedb_id.c +++ b/cachedb/cachedb_id.c @@ -346,7 +346,8 @@ struct cachedb_id* new_cachedb_id(const str* url) memset(ptr, 0, sizeof(struct cachedb_id)); if (parse_cachedb_url(ptr, url) < 0) { - LM_ERR("error while parsing database URL: '%.*s' \n", url->len, url->s); + LM_ERR("error while parsing database URL: '%s'\n", + db_url_escape(url)); goto err; } diff --git a/cachedb/test/test_cachedb.c b/cachedb/test/test_cachedb.c index 64fbadc4cae..029018748ee 100644 --- a/cachedb/test/test_cachedb.c +++ b/cachedb/test/test_cachedb.c @@ -444,11 +444,11 @@ static void test_cachedb_url(void) db = new_cachedb_id(_str("redis:group1://:devxxxxxx@172.31.180.127:6379")); if (!ok(db != NULL)) return; - ok(str_match(_str(db->scheme), &str_init("redis"))); - ok(str_match(_str(db->group_name), &str_init("group1"))); - ok(str_match(_str(db->username), &str_init(""))); - ok(str_match(_str(db->password), &str_init("devxxxxxx"))); - ok(str_match(_str(db->host), &str_init("172.31.180.127"))); + ok(!strcmp(db->scheme, "redis")); + ok(!strcmp(db->group_name, "group1")); + ok(!strcmp(db->username, "")); + ok(!strcmp(db->password, "devxxxxxx")); + ok(!strcmp(db->host, "172.31.180.127")); ok(db->port == 6379); ok(!db->database); ok(!db->extra_options); @@ -463,6 +463,6 @@ static void test_cachedb_url(void) db = new_cachedb_id(_str("redis:group1://:devxxxxxx@172.31.180.127:6379/d?x=1&q=2")); if (!ok(db != NULL)) return; - ok(str_match(_str(db->database), &str_init("d"))); - ok(str_match(_str(db->extra_options), &str_init("x=1&q=2"))); + ok(!strcmp(db->database, "d")); + ok(!strcmp(db->extra_options, "x=1&q=2")); } diff --git a/modules/cachedb_mongodb/cachedb_mongodb.c b/modules/cachedb_mongodb/cachedb_mongodb.c index 61d26e4e1bc..4373cdc2473 100644 --- a/modules/cachedb_mongodb/cachedb_mongodb.c +++ b/modules/cachedb_mongodb/cachedb_mongodb.c @@ -148,7 +148,7 @@ static int child_init(int rank) cachedb_con *con; for (it = mongodb_script_urls;it;it=it->next) { - LM_DBG("iterating through conns - [%.*s]\n",it->url.len,it->url.s); + LM_DBG("iterating through conns - [%s]\n", db_url_escape(&it->url)); con = mongo_con_init(&it->url); if (con == NULL) { LM_ERR("failed to open connection\n"); diff --git a/modules/cachedb_mongodb/cachedb_mongodb_dbase.c b/modules/cachedb_mongodb/cachedb_mongodb_dbase.c index 6c3b0ca9aa9..9c7ca917eee 100644 --- a/modules/cachedb_mongodb/cachedb_mongodb_dbase.c +++ b/modules/cachedb_mongodb/cachedb_mongodb_dbase.c @@ -126,12 +126,12 @@ mongo_con* mongo_new_connection(struct cachedb_id* id) snprintf(osips_appname, MONGOC_HANDSHAKE_APPNAME_MAX, "opensips-%d", my_pid()); - LM_DBG("MongoDB conn for [%s]: %s:%s %s:%s |%s|:%u\n", osips_appname, - id->scheme, id->group_name, id->username, id->password, id->host, id->port); + LM_DBG("MongoDB conn for [%s]: %s:%s://%s:xxxxxx@%s:%u\n", osips_appname, + id->scheme, id->group_name, id->username, id->host, id->port); conn_str = build_mongodb_connect_string(id); - LM_DBG("cstr: %s\n", conn_str); + LM_DBG("cstr: %s\n", _db_url_escape(conn_str)); con = pkg_malloc(sizeof *con); if (!con) { @@ -144,7 +144,7 @@ mongo_con* mongo_new_connection(struct cachedb_id* id) con->client = mongoc_client_new(conn_str); if (!con->client) { - LM_ERR("failed to connect to Mongo (%s)\n", conn_str); + LM_ERR("failed to connect to Mongo (%s)\n", _db_url_escape(conn_str)); return NULL; } @@ -1673,7 +1673,7 @@ int mongo_truncate(cachedb_con *con) start_expire_timer(start, mongo_exec_threshold); if (!mongoc_collection_remove(MONGO_COLLECTION(con), MONGOC_REMOVE_NONE, &empty_doc, NULL, &error)) { - LM_ERR("failed to truncate con %.*s!\n", con->url.len, con->url.s); + LM_ERR("failed to truncate collection '%s'!\n", MONGO_COL_STR(con)); ret = -1; } stop_expire_timer(start, mongo_exec_threshold, "MongoDB truncate", diff --git a/modules/cachedb_redis/cachedb_redis.c b/modules/cachedb_redis/cachedb_redis.c index eb6da490f72..3370e8f3baa 100644 --- a/modules/cachedb_redis/cachedb_redis.c +++ b/modules/cachedb_redis/cachedb_redis.c @@ -123,7 +123,7 @@ static int child_init(int rank) cachedb_con *con; for (it = redis_script_urls;it;it=it->next) { - LM_DBG("iterating through conns - [%.*s]\n",it->url.len,it->url.s); + LM_DBG("iterating through conns - [%s]\n", db_url_escape(&it->url)); con = redis_init(&it->url); if (con == NULL) { LM_ERR("failed to open connection\n"); diff --git a/ut.c b/ut.c index 908518a0462..0790ce435d2 100644 --- a/ut.c +++ b/ut.c @@ -421,3 +421,45 @@ int word64decode(unsigned char *out, unsigned char *in, int len) return out_len; } + +char *db_url_escape(const str *url) +{ + static str buf; + char *at, *slash, *scn; + str upw; + + if (!url) + return NULL; + + if (pkg_str_extend(&buf, url->len + 6 + 1) < 0) { + LM_ERR("oom\n"); + return NULL; + } + + /* if there's no '@' sign, the URL has no password */ + at = q_memchr(url->s, '@', url->len); + if (!at) + goto url_is_safe; + + /* locate the end of the scheme (typical start for the user:password) */ + slash = q_memchr(url->s, '/', url->len); + if (!slash || slash >= at) + goto url_is_safe; + + upw.s = slash; + upw.len = at - slash; + + /* if the semicolon is missing, the URL has no password (only username) */ + scn = q_memchr(upw.s, ':', upw.len); + if (!scn) + goto url_is_safe; + + sprintf(buf.s, "%.*s:xxxxxx@%.*s", (int)(scn - url->s), url->s, + (int)(url->len - (at - url->s) - 1), at + 1); + + return buf.s; + +url_is_safe: + sprintf(buf.s, "%.*s", url->len, url->s); + return buf.s; +} diff --git a/ut.h b/ut.h index ad61d3190f2..4c999a3ff9b 100644 --- a/ut.h +++ b/ut.h @@ -1288,6 +1288,15 @@ static inline void * l_memmem(const void *b1, const void *b2, return NULL; } +/** + * Make any database URL log-friendly by masking its password, if any + * Note: makes use of a single, static buffer -- use accordingly! + */ +char *db_url_escape(const str *url); +static inline char *_db_url_escape(char *url) +{ + return db_url_escape(_str(url)); +} int user2uid(int* uid, int* gid, char* user);