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:

* 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?

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

best regards
Christian Felsing

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