imap deadlock bug in 0.7.65

[Maxim Dounin]

Hello!

On Fri, Apr 23, 2010 at 06:46:02AM +0400, Maxim Dounin wrote:

> Hello!
> 
> On Thu, Apr 22, 2010 at 06:12:57PM -0700, Alan Batie wrote:
> 
> > 
> > At line 729 of ngx_mail_proxy_module.c, there is this check for how much
> > data was received from an imap server response:
> > 
> >     if (b->last - b->pos < 5) {
> >         return NGX_AGAIN;
> >     }
> > 
> > Our zimbra server, oddly enough, running nginx itself, returns "+ \r\n"
> > in response to the initial phase of a login.  As this is only 4
> > characters, nginx goes back for more, only there isn't any more coming,
> > resulting in a timeout.  Changing 5 to 4 fixes the problem, though
> > probably a "MIN_IMAP_RESPONSE" define would probably be better.
> 
> As far as I understand RFC 3501 the only situation where "+ " CRLF 
> form is permitted is server challenage in AUTHENTICATE command.  
> And nginx doesn't use AUTHENTICATE command while talking to backends, 
> it uses LOGIN command instead.
> 
> Could you please provide something like tcpdump -xXs0 of such 
> a connection and some more details about your backend?
> 
> I'm ok with relaxing the above check, just curious.

Just for the record: Alan pointed off-list that he uses Zimbra 6 
as a backend.  Looking int source code indeed reveals that Zimbra 
uses nginx 0.5.30 with local patches which causes it to return 
"+ " CRLF as literal continuation response.

I believe the following patch would be appropriate:

--- a/src/mail/ngx_mail_proxy_module.c
+++ b/src/mail/ngx_mail_proxy_module.c
@@ -726,7 +726,12 @@ ngx_mail_proxy_read_response(ngx_mail_se

     b->last += n;

-    if (b->last - b->pos < 5) {
+    /*
+     * check up to 4 chars unconditionally
+     * minimal expected reply is "+ " CRLF
+     */
+
+    if (b->last - b->pos < 4) {
         return NGX_AGAIN;
     }


Alternatively, the attached one may be used.  It bumps limit to 2 
(just CRLF) and adds appropriate length checking to individual reply 
checks.

Maxim Dounin
-------------- next part --------------
# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1272242475 -14400
# Node ID 54a94faecee92b6638a35b3dafe401efc461c647
# Parent  19b134bf21c0e34a17590df5d83813cbe62735be
Mail: only wait for CRLF from backend.

Backend reply shorter that one we expect (at least 3 chars + CRLF) currently
results in timeout.  Wait only for CRLF and reject too short replies instead.

diff --git a/src/mail/ngx_mail_proxy_module.c b/src/mail/ngx_mail_proxy_module.c
--- a/src/mail/ngx_mail_proxy_module.c
+++ b/src/mail/ngx_mail_proxy_module.c
@@ -726,7 +726,7 @@ ngx_mail_proxy_read_response(ngx_mail_se
 
     b->last += n;
 
-    if (b->last - b->pos < 5) {
+    if (b->last - b->pos < 2) {
         return NGX_AGAIN;
     }
 
@@ -743,11 +743,12 @@ ngx_mail_proxy_read_response(ngx_mail_se
     }
 
     p = b->pos;
+    n = b->last - b->pos - 2;
 
     switch (s->protocol) {
 
     case NGX_MAIL_POP3_PROTOCOL:
-        if (p[0] == '+' && p[1] == 'O' && p[2] == 'K') {
+        if (n > 2 && p[0] == '+' && p[1] == 'O' && p[2] == 'K') {
             return NGX_OK;
         }
         break;
@@ -756,7 +757,9 @@ ngx_mail_proxy_read_response(ngx_mail_se
         switch (state) {
 
         case ngx_imap_start:
-            if (p[0] == '*' && p[1] == ' ' && p[2] == 'O' && p[3] == 'K') {
+            if (n > 3 && p[0] == '*' && p[1] == ' ' && p[2] == 'O'
+                && p[3] == 'K')
+            {
                 return NGX_OK;
             }
             break;
@@ -781,6 +784,10 @@ ngx_mail_proxy_read_response(ngx_mail_se
         break;
 
     default: /* NGX_MAIL_SMTP_PROTOCOL */
+        if (n < 3) {
+            break;
+        }
+
         switch (state) {
 
         case ngx_smtp_start: