Hi,

My answer are below.

2014-04-27 9:54 GMT+02:00 Christian Felsing <pug@felsing.net>:
> Hello,
>
> this patch has an buffer length calculation issue in
> src/mail/ngx_mail_auth_http_module.c, in case of multiple login - logout
> sequences sometimes I got signal 11 errors in log which are caused by
> memory access outside that buffer. This may also a security issue.
>
> len = sizeof("GET ") - 1 + ahcf->uri.len + sizeof(" HTTP/1.0" CRLF) - 1
> + sizeof("Host: ") - 1 + ahcf->host_header.len + sizeof(CRLF) - 1
> + sizeof("Auth-Method: ") - 1
> + ngx_mail_auth_http_method[s->auth_method].len
> + sizeof(CRLF) - 1
> + sizeof("Auth-User: ") - 1 + login.len + sizeof(CRLF) - 1
> + sizeof("Auth-Pass: ") - 1 + passwd.len + sizeof(CRLF) - 1
> + sizeof("Auth-Salt: ") - 1 + s->salt.len
> #if (NGX_MAIL_SSL)
> + sizeof("Auth-Certificate: ") - 1 + cert.len + sizeof(CRLF) - 1
> + sizeof("Auth-Verify: ") - 1 + verify.len + sizeof(CRLF) - 1
> + sizeof("Auth-Issuer-DN: ") - 1 + issuer.len + sizeof(CRLF) - 1
> + sizeof("Auth-Subject-DN: ") - 1 + subject.len + sizeof(CRLF) - 1
> + sizeof("Auth-Subject-Serial: ") - 1 + serial.len
> + sizeof(CRLF) - 1
> #endif
> + sizeof("Auth-Protocol: ") - 1 + cscf->protocol->name.len
> + sizeof(CRLF) - 1
> + sizeof("Auth-Login-Attempt: ") - 1 + NGX_INT_T_LEN
> + sizeof(CRLF) - 1
> + sizeof("Client-IP: ") - 1 + s->connection->addr_text.len
> + sizeof(CRLF) - 1
> + sizeof("Client-Host: ") - 1 + s->host.len + sizeof(CRLF) - 1
> + sizeof("Auth-SMTP-Helo: ") - 1 + s->smtp_helo.len
> + sizeof("Auth-SMTP-From: ") - 1 + s->smtp_from.len
> + sizeof("Auth-SMTP-To: ") - 1 + s->smtp_to.len
> + ahcf->header.len
> + sizeof(CRLF) - 1
> + 20; // <======================
>
>
> To verify that I added 20 extra byte to buffer which fixed that problem
> in this special case - this dirty patch was only for verification. I
> think buffer length calculation is wrong. Further questions arises:
>

I'm not sure about this, as this buffer already has an oversized allocation:
The SMTP header len is added whatever the current mail protocol, for example.


> * What happens if subjects (issuer or other attribute) contains 0x00? Do
> functions like ngx_ssl_get_issuer_dn care about that?
>
> * All Attributes may contain UTF-8 characters, that means a character
> may consist of 1 to 4 bytes. Does length calculation care about that?
>

The code of ngx_ssl_get_issuer_dn contains a loop that recalc the
exact text len :

for (len = 0; p[len]; len++) { /* void */ }

Same about the certificate subject.
C strings are zero terminated, whatever its contains : UTF-8 or not.

> before doing something like
> b->last = ngx_cpymem(b->last, "GET ", sizeof("GET ") - 1);
> it should be checked
>
> if ((ngx_buf_t *)(b->last + (sizeof("GET ") - 1)) > (b + len)) {
> ngx_log_error(NGX_LOG_ALERT, s->connection->log, 0,
> "buffer size exceeded");
> return NULL;
> }

Please try this patch :

====
# HG changeset patch
# User Filipe da Silva <fdasilvayy@gmail.com>
# Date 1398595855 -7200
# Sun Apr 27 12:50:55 2014 +0200
# Node ID d20159ce27d7f13813178c820c4c74da14b38e11
# Parent 741297fe4dc26d04deccd7bbcb1dc53c18f56614
Mail: http request buffer overflow check

diff -r 741297fe4dc2 -r d20159ce27d7 src/mail/ngx_mail_auth_http_module.c
--- a/src/mail/ngx_mail_auth_http_module.c Fri Jan 24 16:26:16 2014 +0100
+++ b/src/mail/ngx_mail_auth_http_module.c Sun Apr 27 12:50:55 2014 +0200
@@ -1381,6 +1381,13 @@ ngx_mail_auth_http_create_request(ngx_ma
/* add "\r\n" at the header end */
*b->last++ = CR; *b->last++ = LF;

+ if ( b->last > b->end )
+ {
+ ngx_log_error(NGX_LOG_EMERG, s->connection->log, 0,
+ "Buffer overflow while creating http request: "
+ "%uz used instead of %uz", b->last - b->start, len);
+ }
+
#if (NGX_DEBUG_MAIL_PASSWD)
{
ngx_str_t l;

====

It will directly check if there is any buffer overflow.



> best regards
> Christian Felsing
>

Best,
Filipe da Silva

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel