[PATCH] Mail: added support for SSL client certificate

[Filipe Da Silva]

Hi,

My answer are below.

2014-04-27 9:54 GMT+02:00 Christian Felsing <pug at 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 at 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