[PATCH] Mail: fix STARTTLS misalloc

[Maxim Dounin]

Hello!

On Fri, Sep 20, 2013 at 10:48:12PM +0300, Markus Linnala wrote:

> # HG changeset patch
> # User Markus Linnala <Markus.Linnala at cybercom.com>
> # Date 1379691757 -10800
> #      Fri Sep 20 18:42:37 2013 +0300
> # Node ID 79cea900573997a74400dcef925de41ec6c150e7
> # Parent  4e7279d4c9c4181683373df3947749a7727b89a4
> Mail: fix STARTTLS misalloc

Trailing dot, please.

> Found by mail_imap.t from mdounin nginx-tests when running under valgrind.
> 
> ==10647== Invalid write of size 1
> ==10647==    at 0x4B1493: ngx_mail_smtp_merge_srv_conf (ngx_mail_smtp_module.c:280)
> ==10647==    by 0x4AB363: ngx_mail_block (ngx_mail.c:209)
> ==10647==    by 0x4303BE: ngx_conf_parse (ngx_conf_file.c:391)
> ==10647==    by 0x42DF03: ngx_init_cycle (ngx_cycle.c:265)
> ==10647==    by 0x4206A9: main (nginx.c:333)
> ==10647==  Address 0x550fb84 is 0 bytes after a block of size 68 alloc'd
> ==10647==    at 0x4A074CD: malloc (vg_replace_malloc.c:236)
> ==10647==    by 0x43B251: ngx_alloc (ngx_alloc.c:22)
> ==10647==    by 0x421B0D: ngx_malloc (ngx_palloc.c:119)
> ==10647==    by 0x421B65: ngx_pnalloc (ngx_palloc.c:147)
> ==10647==    by 0x4B1447: ngx_mail_smtp_merge_srv_conf (ngx_mail_smtp_module.c:269)
> ==10647==    by 0x4AB363: ngx_mail_block (ngx_mail.c:209)
> ==10647==    by 0x4303BE: ngx_conf_parse (ngx_conf_file.c:391)
> ==10647==    by 0x42DF03: ngx_init_cycle (ngx_cycle.c:265)
> ==10647==    by 0x4206A9: main (nginx.c:333)
> ==10647==
> 
> I choose to retain extra CRLF as I could not test protocol change easily.
> As per RFC 2487 there is no extra CRLF. But it was not obvious why it was
> there from history.

As I already said in the ticket, you patch looks wrong for me.

It doesn't retain extra CRLF but rather adds one to the output - 
previously, size (and hence conf->starttls_capability.len) was 
correct, and the problem was unneeded overrun of unallocated 
memory.  With your patch, size becomes wrong - and the output 
changes.

> 
> This fixes ticket #411

Just a "... (ticket #411)." in a summary line, please.

> 
> diff -r 4e7279d4c9c4 -r 79cea9005739 src/mail/ngx_mail_smtp_module.c
> --- a/src/mail/ngx_mail_smtp_module.c	Fri Sep 20 17:57:21 2013 +0300
> +++ b/src/mail/ngx_mail_smtp_module.c	Fri Sep 20 18:42:37 2013 +0300
> @@ -264,7 +264,7 @@
>          last[3] = ' ';
>      }
>  
> -    size += sizeof("250 STARTTLS" CRLF) - 1;
> +    size += sizeof("250 STARTTLS" CRLF CRLF) - 1;
>  
>      p = ngx_pnalloc(cf->pool, size);
>      if (p == NULL) {
> @@ -276,8 +276,7 @@
>  
>      p = ngx_cpymem(p, conf->capability.data, conf->capability.len);
>  
> -    p = ngx_cpymem(p, "250 STARTTLS" CRLF, sizeof("250 STARTTLS" CRLF) - 1);
> -    *p++ = CR; *p = LF;
> +    p = ngx_cpymem(p, "250 STARTTLS" CRLF CRLF, sizeof("250 STARTTLS" CRLF CRLF) - 1);
>  
>      p = conf->starttls_capability.data
>          + (last - conf->capability.data) + 3;

See above.  As I already suggested, correct patch seems to be:

--- a/src/mail/ngx_mail_smtp_module.c
+++ b/src/mail/ngx_mail_smtp_module.c
@@ -277,7 +277,6 @@ ngx_mail_smtp_merge_srv_conf(ngx_conf_t
     p = ngx_cpymem(p, conf->capability.data, conf->capability.len);
 
     p = ngx_cpymem(p, "250 STARTTLS" CRLF, sizeof("250 STARTTLS" CRLF) - 1);
-    *p++ = CR; *p = LF;
 
     p = conf->starttls_capability.data
         + (last - conf->capability.data) + 3;


-- 
Maxim Dounin
http://nginx.org/en/donation.html