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;
>
