Re: [patch] courier-imap-4.13 imapd patch replacing malloc, strcat and strcpy with asprintf

[Peter Malone]

That's it in a nutshell, essentially.

I'll take a stab at it, until I get frustrated.
Perhaps my time would be better suited to something else which could 
help OpenBSD. For the time being, however, I'll give this a shot.


On 04/20/14 02:18, Maxime Villard wrote:
Le 20/04/2014 02:38, Peter Malone a écrit :
Hi,

I'm using OpenBSD 5.5. courier-imap-4.13 is in the ports tree and it's
quite a mess. I started looking at it today with the hope of just
replacing some of the malloc,strcat & strcpy calls with asprintf, but it
became clear before long that there's lots more issues with this code.

Regardless, here's a patch which fixes some of the malloc,strcat &
strcpy spaghetti in imapd.c

I figure you guys are fairly busy with OpenSSL right now so if it's OK
with you I'll get working on the rest of this code.

I spoke with the courier-imap team and their response was less than
satisfactory - claiming asprintf code won't compile on non-linux
platforms.
courier-imap is *totally buggy*. Not only because there are big
programming mistakes - at a time I started to fix many issues like
double-free, use-after-free, leaks, etc., before letting down -,
but also because this "team" doesn't give a fuck at all.

Give a quick glance at the code: it is just a great mess, with bugs
everywhere, no indentation, and probably no review. Clearly, there
are poor - not to say *no* - considerations for security. That's the
idea I made myself of this project, and you seem to have come to the
same conclusion.

--- imapd_orig.c Sat Apr 19 19:38:18 2014
+++ imapd.c Sat Apr 19 20:09:48 2014
@@ -343,9 +343,8 @@

if (q)
{
- r=malloc(strlen(q)+sizeof("/."));
- if (!r) write_error_exit(0);
- strcat(strcpy(r, q), "/.");
+ if(asprintf(&r, "%s%s", q, "/.") == -1)
+ write_error_exit(0);
if (access(r, 0) == 0)
{
free(r);
@@ -1373,11 +1372,9 @@

static char *alloc_filename(const char *mbox, const char *name)
{
-char *p=malloc(strlen(mbox)+strlen(name)+sizeof("/cur/"));
-
- if (!p) write_error_exit(0);
-
- strcat(strcat(strcpy(p, mbox), "/cur/"), name);
+ char *p;
+ if(asprintf(&p, "%s%s%s", mbox, "/cur/", name) == -1)
+ write_error_exit(0);
return (p);
}

@@ -1812,14 +1809,12 @@
{
#if EXPLICITDIRSYNC

- char *p=malloc(strlen(folder)+sizeof("/new"));
+ char *p;
int fd;
-
- if (!p)
+
+ if(asprintf(&p, "%s%s", folder, "/new") == -1)
write_error_exit(0);

- p=strcat(strcpy(p, folder), "/new");
-
fd=open(p, O_RDONLY);

if (fd >= 0)
@@ -1828,7 +1823,8 @@
close(fd);
}

- p=strcat(strcpy(p, folder), "/cur");
+ if(asprintf(&p, "%s%s", folder, "/cur") == -1)
+                 write_error_exit(0);

fd=open(p, O_RDONLY);

@@ -2212,12 +2208,10 @@
{
struct imapscaninfo minfo;

- char *p=malloc(strlen(new_path)+sizeof("/" IMAPDB));
+ char *p;
+ if(asprintf(&p, "%s%s", new_path, "/" IMAPDB) == -1)
+                write_error_exit(0);

- if (!p)
- write_error_exit(0);
-
- strcat(strcpy(p, new_path), "/" IMAPDB);
unlink(p);
free(p);
imapscan_init(&minfo);
@@ -2448,14 +2442,12 @@
}
return 0;
}
- q=malloc(strlen(p)+sizeof("/new"));
- if (!q)
+ if(asprintf(&q, "%s%s", p, "/new") == -1)
{
free(p);
maildir_aclt_list_destroy(aclt_list);
return -1;
}
- strcat(strcpy(q, p), "/new");
free(p);

if (maildir_aclt_list_add(aclt_list,
@@ -4216,11 +4208,10 @@
if (p && (p=maildir_shareddir(".", p+1)) != NULL)
{
int rc;
- char *q=malloc(strlen(p)+sizeof("/shared"));
-
- if (!q) write_error_exit(0);
-
- strcat(strcpy(q, p), "/shared");
+ char *q;
+
+ if(asprintf(&q, "%s%s", p, "/shared") == -1)
+ write_error_exit(0);
free(p);
rc=append(tag, tok->tokenbuf, q);
free(q);
@@ -6041,10 +6032,9 @@
isshared=0;
if (is_sharedsubdir(cqinfo.destmailbox))
{
- char *p=malloc(strlen(cqinfo.destmailbox)+sizeof("/shared"));
-
- if (!p) write_error_exit(0);
- strcat(strcpy(p, cqinfo.destmailbox), "/shared");
+ char *p;
+ if(asprintf(&p, "%s%s", cqinfo.destmailbox, "/shared") == -1)
+ write_error_exit(0);

free(mailbox);
mailbox=cqinfo.destmailbox=p;