Thu 16 May 2013 04:37:18 AM UTC, original submission:
Hi,
there is an issue with memory handling in function mu_url_parse() in mailbox/url.c (mailutils 2.2)
Here is the code excerpt with the problem:
if (u.secret)
{
/* Obfuscate the password */
#define PASS_REPL "***"
#define PASS_REPL_LEN (sizeof (PASS_REPL) - 1)
size_t plen = mu_secret_length (u.secret);
size_t nlen = strlen (url->name);
size_t len = nlen - plen + PASS_REPL_LEN + 1;
char *newname;
memset (url->name + pstart, 0, plen);
newname = realloc (url->name, len);
if (!newname)
goto CLEANUP;
memmove (newname + pstart + PASS_REPL_LEN, newname + pstart + plen,
nlen - (pstart + plen) + 1);
memcpy (newname + pstart, PASS_REPL, PASS_REPL_LEN);
url->name = newname;
}
You can not use memory past len bytes after realloc() call, as this memory is already free to use to any other thread or process, so the memomove() call after this point is using alredy free memory (please note the value of len is less that nlen in the case when plen is greater than PASS_REPL_LEN + 1). On most system this trick with reallocation works but it's an invalid practise and can cause a problem for an application with aggressive memory allocations.
The proposed modification for this code is:
if (u.secret)
{
/* Obfuscate the password */
#define PASS_REPL "***"
#define PASS_REPL_LEN (sizeof (PASS_REPL) - 1)
size_t plen = mu_secret_length (u.secret);
size_t nlen = strlen (url->name);
size_t len = nlen + PASS_REPL_LEN + 1;
char *newname;
memset (url->name + pstart, 0, plen);
newname = realloc (url->name, len);
if (!newname)
goto CLEANUP;
memmove (newname + pstart + PASS_REPL_LEN, newname + pstart + plen,
nlen - (pstart + plen) + 1);
memcpy (newname + pstart, PASS_REPL, PASS_REPL_LEN);
newname = realloc (newname, len - plen);
if (!newname)
goto CLEANUP;
url->name = newname;
}
|