Bug #3214: "helperHandleRead: unexpected read from ssl_crtd" errors.

Squid would read the beginning of a crtd response split across multiple
read operations and treat it as a complete response, causing various
certificate-related errors.

This patch:
 - allow the use of other than the '\n' character as the end of message mark
   for helper responses.
 - Use the '\1' char as end-of-message char for crtd helper. This char looks
   safe because the crtd messages are clear text only messages.

diff --git a/src/helper.cc b/src/helper.cc
index c7f62b2879f4d38667af57f951dd4522a423e3cc..7f4b976be20e8bd8763f7fb18da3a179c9200d63 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -881,30 +881,25 @@ helperHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xerrno, voi
         srv->rbuf[0] = '\0';
     }
 
-    if (hlp->return_full_reply) {
-        debugs(84, 3, HERE << "Return entire buffer");
-        helperReturnBuffer(0, srv, hlp, srv->rbuf, srv->rbuf + srv->roffset);
-    } else {
-        while ((t = strchr(srv->rbuf, '\n'))) {
-            /* end of reply found */
-            char *msg = srv->rbuf;
-            int i = 0;
-            debugs(84, 3, "helperHandleRead: end of reply found");
-
-            if (t > srv->rbuf && t[-1] == '\r')
-                t[-1] = '\0';
-
-            *t++ = '\0';
-
-            if (hlp->childs.concurrency) {
-                i = strtol(msg, &msg, 10);
-
-                while (*msg && xisspace(*msg))
-                    msg++;
-            }
-
-            helperReturnBuffer(i, srv, hlp, msg, t);
+    while ((t = strchr(srv->rbuf, hlp->eom))) {
+        /* end of reply found */
+        char *msg = srv->rbuf;
+        int i = 0;
+        debugs(84, 3, "helperHandleRead: end of reply found");
+        
+        if (t > srv->rbuf && t[-1] == '\r' && hlp->eom == '\n')
+            t[-1] = '\0';
+        
+        *t++ = '\0';
+        
+        if (hlp->childs.concurrency) {
+            i = strtol(msg, &msg, 10);
+            
+            while (*msg && xisspace(*msg))
+                msg++;
         }
+        
+        helperReturnBuffer(i, srv, hlp, msg, t);
     }
 
     if (srv->rfd != -1)
@@ -950,12 +945,12 @@ helperStatefulHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xer
         srv->roffset = 0;
     }
 
-    if ((t = strchr(srv->rbuf, '\n'))) {
+    if ((t = strchr(srv->rbuf, hlp->eom))) {
         /* end of reply found */
         int called = 1;
         debugs(84, 3, "helperStatefulHandleRead: end of reply found");
 
-        if (t > srv->rbuf && t[-1] == '\r')
+        if (t > srv->rbuf && t[-1] == '\r' && hlp->eom == '\n')
             t[-1] = '\0';
 
         *t = '\0';

diff --git a/src/helper.h b/src/helper.h
index b07d38d32fa2912e7480222d8cd4dd8a3f0ff19a..a5dac223256e33e2203dc4c5d259e5eb29bbe4d3 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -49,7 +49,7 @@ typedef void HLPSCB(void *, void *lastserver, char *buf);
 class helper
 {
 public:
-    inline helper(const char *name) : cmdline(NULL), id_name(name) {};
+    inline helper(const char *name) : cmdline(NULL), id_name(name), eom('\n') {}
     ~helper();
 
 public:
@@ -62,6 +62,7 @@ public:
     Ip::Address addr;
     time_t last_queue_warn;
     time_t last_restart;
+    char eom;   ///< The char which marks the end of (response) message, normally '\n'
 
     struct _stats {
         int requests;
@@ -69,8 +70,6 @@ public:
         int queue_size;
         int avg_svc_time;
     } stats;
-    /// True if callback expects the whole helper output, as a c-string.
-    bool return_full_reply;
 
 private:
     CBDATA_CLASS2(helper);

diff --git a/src/ssl/crtd_message.cc b/src/ssl/crtd_message.cc
index 21b85e2b3d946d0b8aac7ff31153a3731a8bb0b1..1ade0a7ceafa96d6bef3cf1410cb14ffc50a55f5 100644 (file)
--- a/src/ssl/crtd_message.cc
+++ b/src/ssl/crtd_message.cc
@@ -126,7 +126,7 @@ std::string Ssl::CrtdMessage::compose() const
     if (code.empty()) return std::string();
     char buffer[10];
     snprintf(buffer, sizeof(buffer), "%zd", body.length());
-    return code + ' ' + buffer + ' ' + body + '\n';
+    return code + ' ' + buffer + ' ' + body;
 }
 
 void Ssl::CrtdMessage::clear()

diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc
index e6d414037ba7cfcd6d4b5fa62b182651e52e64a3..546ed3d32943bdfb4051503b551114b64b442e1d 100644 (file)
--- a/src/ssl/helper.cc
+++ b/src/ssl/helper.cc
@@ -30,6 +30,9 @@ void Ssl::Helper::Init()
         ssl_crtd = new helper("ssl_crtd");
     ssl_crtd->childs = Ssl::TheConfig.ssl_crtdChildren;
     ssl_crtd->ipc_type = IPC_STREAM;
+    // The crtd messages may contain the eol ('\n') character. We are
+    // going to use the '\1' char as the end-of-message mark.
+    ssl_crtd->eom = '\1';
     assert(ssl_crtd->cmdline == NULL);
     {
         char *tmp = xstrdup(Ssl::TheConfig.ssl_crtd);
@@ -57,7 +60,6 @@ void Ssl::Helper::Init()
         }
         safe_free(tmp_begin);
     }
-    ssl_crtd->return_full_reply = true;
     helperOpenServers(ssl_crtd);
 }
 
@@ -88,5 +90,7 @@ void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void
     }
 
     first_warn = 0;
-    helperSubmit(ssl_crtd, message.compose().c_str(), callback, data);
+    std::string msg = message.compose();
+    msg += '\n';
+    helperSubmit(ssl_crtd, msg.c_str(), callback, data);
 }

diff --git a/src/ssl/ssl_crtd.cc b/src/ssl/ssl_crtd.cc
index ca43cfbbf48db06df8ff9bb1f7db48e026301205..2f5b61bb3ecbcd3d212a92265e28c409d2936b06 100644 (file)
--- a/src/ssl/ssl_crtd.cc
+++ b/src/ssl/ssl_crtd.cc
@@ -245,7 +245,8 @@ static bool proccessNewRequest(Ssl::CrtdMessage const & request_message, std::st
     response_message.setCode("ok");
     response_message.setBody(bufferToWrite);
 
-    std::cout << response_message.compose();
+    // Use the '\1' char as end-of-message character
+    std::cout << response_message.compose() << '\1' << std::flush;
 
     return true;
 }