Skip to content

Commit ed74875

Browse files
committed
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
1 parent 7c41fe5 commit ed74875

File tree

8 files changed

+68
-16
lines changed

8 files changed

+68
-16
lines changed

cachedb/cachedb.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -658,7 +658,7 @@ cachedb_con* cachedb_do_init(str *url,void* (*new_connection)(struct cachedb_id
658658

659659
id = new_cachedb_id(url);
660660
if (!id) {
661-
LM_ERR("cannot parse url [%.*s]\n",url->len,url->s);
661+
LM_ERR("cannot parse url [%s]\n", db_url_escape(url));
662662
pkg_free(res);
663663
return 0;
664664
}

cachedb/cachedb_id.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,8 @@ struct cachedb_id* new_cachedb_id(const str* url)
348348
memset(ptr, 0, sizeof(struct cachedb_id));
349349

350350
if (parse_cachedb_url(ptr, url) < 0) {
351-
LM_ERR("error while parsing database URL: '%.*s' \n", url->len, url->s);
351+
LM_ERR("error while parsing database URL: '%s'\n",
352+
db_url_escape(url));
352353
goto err;
353354
}
354355

cachedb/test/test_cachedb.c

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -531,11 +531,11 @@ static void test_cachedb_url(void)
531531
db = new_cachedb_id(_str("redis:group1://:[email protected]:6379"));
532532
if (!ok(db != NULL))
533533
return;
534-
ok(str_match(_str(db->scheme), &str_init("redis")));
535-
ok(str_match(_str(db->group_name), &str_init("group1")));
536-
ok(str_match(_str(db->username), &str_init("")));
537-
ok(str_match(_str(db->password), &str_init("devxxxxxx")));
538-
ok(str_match(_str(db->host), &str_init("172.31.180.127")));
534+
ok(!strcmp(db->scheme, "redis"));
535+
ok(!strcmp(db->group_name, "group1"));
536+
ok(!strcmp(db->username, ""));
537+
ok(!strcmp(db->password, "devxxxxxx"));
538+
ok(!strcmp(db->host, "172.31.180.127"));
539539
ok(db->port == 6379);
540540
ok(!db->database);
541541
ok(!db->extra_options);
@@ -550,6 +550,6 @@ static void test_cachedb_url(void)
550550
db = new_cachedb_id(_str("redis:group1://:[email protected]:6379/d?x=1&q=2"));
551551
if (!ok(db != NULL))
552552
return;
553-
ok(str_match(_str(db->database), &str_init("d")));
554-
ok(str_match(_str(db->extra_options), &str_init("x=1&q=2")));
553+
ok(!strcmp(db->database, "d"));
554+
ok(!strcmp(db->extra_options, "x=1&q=2"));
555555
}

modules/cachedb_mongodb/cachedb_mongodb.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ static int child_init(int rank)
148148
cachedb_con *con;
149149

150150
for (it = mongodb_script_urls;it;it=it->next) {
151-
LM_DBG("iterating through conns - [%.*s]\n",it->url.len,it->url.s);
151+
LM_DBG("iterating through conns - [%s]\n", db_url_escape(&it->url));
152152
con = mongo_con_init(&it->url);
153153
if (con == NULL) {
154154
LM_ERR("failed to open connection\n");

modules/cachedb_mongodb/cachedb_mongodb_dbase.c

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -126,12 +126,12 @@ mongo_con* mongo_new_connection(struct cachedb_id* id)
126126

127127
snprintf(osips_appname, MONGOC_HANDSHAKE_APPNAME_MAX, "opensips-%d", my_pid());
128128

129-
LM_DBG("MongoDB conn for [%s]: %s:%s %s:%s |%s|:%u\n", osips_appname,
130-
id->scheme, id->group_name, id->username, id->password, id->host, id->port);
129+
LM_DBG("MongoDB conn for [%s]: %s:%s://%s:xxxxxx@%s:%u\n", osips_appname,
130+
id->scheme, id->group_name, id->username, id->host, id->port);
131131

132132
conn_str = build_mongodb_connect_string(id);
133133

134-
LM_DBG("cstr: %s\n", conn_str);
134+
LM_DBG("cstr: %s\n", _db_url_escape(conn_str));
135135

136136
con = pkg_malloc(sizeof *con);
137137
if (!con) {
@@ -144,7 +144,7 @@ mongo_con* mongo_new_connection(struct cachedb_id* id)
144144

145145
con->client = mongoc_client_new(conn_str);
146146
if (!con->client) {
147-
LM_ERR("failed to connect to Mongo (%s)\n", conn_str);
147+
LM_ERR("failed to connect to Mongo (%s)\n", _db_url_escape(conn_str));
148148
return NULL;
149149
}
150150

@@ -1673,7 +1673,7 @@ int mongo_truncate(cachedb_con *con)
16731673
start_expire_timer(start, mongo_exec_threshold);
16741674
if (!mongoc_collection_remove(MONGO_COLLECTION(con),
16751675
MONGOC_REMOVE_NONE, &empty_doc, NULL, &error)) {
1676-
LM_ERR("failed to truncate con %.*s!\n", con->url.len, con->url.s);
1676+
LM_ERR("failed to truncate collection '%s'!\n", MONGO_COL_STR(con));
16771677
ret = -1;
16781678
}
16791679
_stop_expire_timer(start, mongo_exec_threshold, "MongoDB truncate",

modules/cachedb_redis/cachedb_redis.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ static int child_init(int rank)
155155
cachedb_con *con;
156156

157157
for (it = redis_script_urls;it;it=it->next) {
158-
LM_DBG("iterating through conns - [%.*s]\n",it->url.len,it->url.s);
158+
LM_DBG("iterating through conns - [%s]\n", db_url_escape(&it->url));
159159
con = redis_init(&it->url);
160160
if (con == NULL) {
161161
LM_ERR("failed to open connection\n");

ut.c

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -735,3 +735,45 @@ int _base32decode(unsigned char *out, unsigned char *in, int len,
735735

736736
return out_len;
737737
}
738+
739+
char *db_url_escape(const str *url)
740+
{
741+
static str buf;
742+
char *at, *slash, *scn;
743+
str upw;
744+
745+
if (!url)
746+
return NULL;
747+
748+
if (pkg_str_extend(&buf, url->len + 6 + 1) < 0) {
749+
LM_ERR("oom\n");
750+
return NULL;
751+
}
752+
753+
/* if there's no '@' sign, the URL has no password */
754+
at = q_memchr(url->s, '@', url->len);
755+
if (!at)
756+
goto url_is_safe;
757+
758+
/* locate the end of the scheme (typical start for the user:password) */
759+
slash = q_memchr(url->s, '/', url->len);
760+
if (!slash || slash >= at)
761+
goto url_is_safe;
762+
763+
upw.s = slash;
764+
upw.len = at - slash;
765+
766+
/* if the semicolon is missing, the URL has no password (only username) */
767+
scn = q_memchr(upw.s, ':', upw.len);
768+
if (!scn)
769+
goto url_is_safe;
770+
771+
sprintf(buf.s, "%.*s:xxxxxx@%.*s", (int)(scn - url->s), url->s,
772+
(int)(url->len - (at - url->s) - 1), at + 1);
773+
774+
return buf.s;
775+
776+
url_is_safe:
777+
sprintf(buf.s, "%.*s", url->len, url->s);
778+
return buf.s;
779+
}

ut.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1417,6 +1417,15 @@ static inline void * l_memmem(const void *b1, const void *b2,
14171417
return NULL;
14181418
}
14191419

1420+
/**
1421+
* Make any database URL log-friendly by masking its password, if any
1422+
* Note: makes use of a single, static buffer -- use accordingly!
1423+
*/
1424+
char *db_url_escape(const str *url);
1425+
static inline char *_db_url_escape(char *url)
1426+
{
1427+
return db_url_escape(_str(url));
1428+
}
14201429

14211430
int user2uid(int* uid, int* gid, char* user);
14221431

0 commit comments

Comments
 (0)