Hi, My answer are below.
2014-04-27 9:54 GMT+02:00 Christian Felsing <[email protected]>: > 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 <[email protected]> # 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 [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
