Reject excessively large FTP control replies (#2434)

When parsing FTP control replies, `Ftp::Client::parseControlReply()`
stores individual lines in the `ctrl.message` wordlist. The stored
values are later combined, appended, encoded, and/or converted to String
objects, exposing the results to `String::SizeMax_` limitations. Recent
commit 46f3f80 already ensures `reply_header_max_size` limits for
control replies. This change adds checks for cases where
`reply_header_max_size` configuration exceeds the recommended maximum
value. It also protects any sensitive worldlist-manipulating code that
might become reachable before `reply_header_max_size` limit is checked.

Excessively large FTP control replies now lead to ERR_FTP_FAILURE.

This is a Measurement Factory project.

diff --git a/src/SquidString.h b/src/SquidString.h
index b1094815d4e5c04f57e5cabe07ea0991b47a8edf..daa74a111ee908c5ca1974f49682618f75927dfe 100644 (file)
--- a/src/SquidString.h
+++ b/src/SquidString.h
@@ -71,6 +71,10 @@ public:
     /// the useful content length is strictly less than this limit.
     static size_type SizeMaxXXX() { return SizeMax_; }
 
+    /// The size limit for input that is later fed to legacy processing/encoding
+    /// algorithms that grow the String without checking SizeMaxXXX().
+    static size_type RawSizeMaxXXX() { return (SizeMaxXXX()+1)/3; }
+
     size_type size() const { return len_; }
 
     /// variant of size() suited to be used for printf-alikes.

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index 4942f479bd28c6ad382752302962bfb0eeb44549..b2e463798d8381e11b2f6c6d697aac857b595766 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -998,7 +998,7 @@ configDoConfigure(void)
     // Warn about the dangers of exceeding String limits when manipulating HTTP
     // headers. Technically, we do not concatenate _requests_, so we could relax
     // their check, but we keep the two checks the same for simplicity sake.
-    const auto safeRawHeaderValueSizeMax = (String::SizeMaxXXX()+1)/3;
+    const auto safeRawHeaderValueSizeMax = String::RawSizeMaxXXX();
     // TODO: static_assert(safeRawHeaderValueSizeMax >= 64*1024); // no WARNINGs for default settings
     if (Config.maxRequestHeaderSize > safeRawHeaderValueSizeMax)
         debugs(3, DBG_CRITICAL, "WARNING: Increasing request_header_max_size beyond " << safeRawHeaderValueSizeMax <<

diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc
index 1290c5fc4f19fc110a913e2a87a0b96b98dbe357..eb635853445c33ba6b5a4bbc83932b236396ecfb 100644 (file)
--- a/src/clients/FtpClient.cc
+++ b/src/clients/FtpClient.cc
@@ -444,10 +444,15 @@ Ftp::Client::handleControlReply()
 
     size_t bytes_used = 0;
     wordlistDestroy(&ctrl.message);
-
-    if (!parseControlReply(bytes_used)) {
-        /* didn't get complete reply yet */
-        scheduleReadControlReply(0);
+    try {
+        if (!parseControlReply(bytes_used)) {
+            /* didn't get complete reply yet */
+            scheduleReadControlReply(0);
+            return;
+        }
+    } catch (...) {
+        debugs(9, 2, "ERROR: Cannot parse control reply: " << CurrentException);
+        failed(ERR_FTP_FAILURE, 0);
         return;
     }
 
@@ -1122,11 +1127,12 @@ bool
 Ftp::Client::parseControlReply(size_t &bytesUsed)
 {
     char *s;
-    char *sbuf;
     char *end;
     int usable;
     int complete = 0;
     wordlist *head = nullptr;
+    auto headDeleter = [](wordlist *h) { wordlistDestroy(&h); };
+    auto headGuard = std::unique_ptr<wordlist, decltype(headDeleter)>(head, headDeleter);
     wordlist *list;
     wordlist **tail = &head;
     size_t linelen;
@@ -1135,7 +1141,8 @@ Ftp::Client::parseControlReply(size_t &bytesUsed)
      * We need a NULL-terminated buffer for scanning, ick
      */
     const size_t len = ctrl.offset;
-    sbuf = (char *)xmalloc(len + 1);
+    const auto sbufOwner = std::unique_ptr<void, decltype(&xfree)>(xmalloc(len + 1), xfree);
+    const auto sbuf = static_cast<char*>(sbufOwner.get());
     xstrncpy(sbuf, ctrl.buf, len + 1);
     end = sbuf + len - 1;
 
@@ -1148,7 +1155,6 @@ Ftp::Client::parseControlReply(size_t &bytesUsed)
 
     if (usable == 0) {
         debugs(9, 3, "didn't find end of line");
-        safe_free(sbuf);
         return false;
     }
 
@@ -1157,6 +1163,9 @@ Ftp::Client::parseControlReply(size_t &bytesUsed)
     s = sbuf;
     s += strspn(s, crlf);
 
+    // cumulative length of parsed control reply lines added to the list
+    size_t replyLength = 0;
+
     for (; s < end; s += strcspn(s, crlf), s += strspn(s, crlf)) {
         if (complete)
             break;
@@ -1164,14 +1173,20 @@ Ftp::Client::parseControlReply(size_t &bytesUsed)
         debugs(9, 5, "s = {" << s << "}");
 
         linelen = strcspn(s, crlf) + 1;
+        replyLength += linelen;
 
         if (linelen < 2)
             break;
 
+        if (replyLength > String::RawSizeMaxXXX())
+            throw TextException(ToSBuf("control reply too long: ", replyLength, " exceeds safe limit of ", String::RawSizeMaxXXX(), " bytes"), Here());
+
         if (linelen > 3)
             complete = (*s >= '0' && *s <= '9' && *(s + 3) == ' ');
 
         list = new wordlist();
+        if (!headGuard)
+            headGuard.reset(list);
 
         list->key = (char *)xmalloc(linelen);
 
@@ -1193,14 +1208,12 @@ Ftp::Client::parseControlReply(size_t &bytesUsed)
     }
 
     bytesUsed = static_cast<size_t>(s - sbuf);
-    safe_free(sbuf);
 
     if (!complete) {
-        wordlistDestroy(&head);
         return false;
     }
 
-    ctrl.message = head;
+    ctrl.message = headGuard.release();
     assert(ctrl.replycode >= 0);
     assert(ctrl.last_reply);
     assert(ctrl.message);