]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Reorganized FTP response storage/wrapping to fix multi-line response gatewaying
authorAlex Rousskov <rousskov@measurement-factory.com>
Thu, 29 Aug 2013 00:15:55 +0000 (18:15 -0600)
committerAlex Rousskov <rousskov@measurement-factory.com>
Thu, 29 Aug 2013 00:15:55 +0000 (18:15 -0600)
Multi-line FTP control responses use various line prefixes to tell the client
that the response continues to the next line. Some multi-line responses use a
"CODE-" line prefix. Some, like FEAT, must use a single space as a line prefix
(except for the first line that uses "CODE-"). Squid was removing the virgin
prefix and then using "CODE-" prefix for all lines, breaking FEAT and probably
other responses.

While modifying original multi-line prefix was a bad idea, leaving FTP
multi-lines "as is" does not work either because HTTP wrapping removes leading
spaces which are significant in FEAT and other FTP responses.

Squid now preserves leading multi-lines by wrapping them using quoted strings.
Adaptation services wishing to interpret multi-lines must unquote any quoted
FTP-* header field values before adaptation and return quoted values back (if
needed).  Which FTP-* header values are quoted and which are not may be
value-dependent and may change. Quoting and unquoting requires handling of
HTTP \-CHAR escape sequences.

The last FTP response line has to be treated specially because it has [more]
strict syntax, has to be interpreted by Squid, subjected to squid.conf ACLs,
and is more likely to be adapted. Squid used to wrap all multi-lines into
multiple fields of an FTP-Reason header while only storing the "reason" from
the last multi-line there. That was messy, and became prohibitively so when
multi-line quoting of multi-lines was introduced.

Now Squid wraps all multi-lines except the last one using FTP-Pre header.  All
FTP-Pre lines may be wrapped as quoted strings. FTP-Status and FTP-Reason
headers are used for the FTP code and reason phrase from the last line:

  FTP-Pre: "123-first line"
  FTP-Pre: " second line"
  FTP-Status: 123
  FTP-Reason: from the third line

Needs more work if there are adaptation services that merge multiple FTP-Pre
header values together.

1  2 
src/FtpGatewayServer.cc
src/FtpServer.cc
src/FtpServer.h
src/HttpHeader.cc
src/HttpHeader.h
src/client_side.cc

index 5df26b25566edf32a59726b2a04e107221827409,5df26b25566edf32a59726b2a04e107221827409..1a055bc7af063dc9e096cb7774052cf975178b65
@@@ -314,13 -314,13 +314,15 @@@ ServerStateData::createHttpReply(const 
      }
      if (clen >= 0)
          header.putInt64(HDR_CONTENT_LENGTH, clen);
++
++    if (ctrl.message) {
++        for (wordlist *W = ctrl.message; W && W->next; W = W->next)
++            header.putStr(HDR_FTP_PRE, httpHeaderQuoteString(W->key).termedBuf());
++    }
      if (ctrl.replycode > 0)
          header.putInt(HDR_FTP_STATUS, ctrl.replycode);
--    if (ctrl.message) {
--        for (wordlist *W = ctrl.message; W; W = W->next)
--            header.putStr(HDR_FTP_REASON, W->key);
--    } else if (ctrl.last_command)
--        header.putStr(HDR_FTP_REASON, ctrl.last_command);
++    if (ctrl.last_reply)
++        header.putStr(HDR_FTP_REASON, ctrl.last_reply);
  
      reply->hdrCacheInit();
  
index 5b03dc0517b43536365342b25beb4327707b3997,5b03dc0517b43536365342b25beb4327707b3997..a4a0ab033bbfd0642a2db2cb5673700551f23411
@@@ -339,10 -339,10 +339,8 @@@ ServerStateData::handleControlReply(
  
      size_t bytes_used = 0;
      wordlistDestroy(&ctrl.message);
--    ctrl.message = parseControlReply(ctrl.buf, ctrl.offset, &ctrl.replycode,
--                                     &bytes_used);
  
--    if (ctrl.message == NULL) {
++    if (!parseControlReply(bytes_used)) {
          /* didn't get complete reply yet */
  
          if (ctrl.offset == ctrl.size) {
  
          scheduleReadControlReply(0);
          return;
--    } else if (ctrl.offset == bytes_used) {
++    } 
++
++    assert(ctrl.message); // the entire FTP server response, line by line
++    assert(ctrl.replycode >= 0); // FTP status code (from the last line)
++    assert(ctrl.last_reply); // FTP reason (from the last line)
++
++    if (ctrl.offset == bytes_used) {
          /* used it all up */
          ctrl.offset = 0;
      } else {
          memmove(ctrl.buf, ctrl.buf + bytes_used, ctrl.offset);
      }
  
--    /* Move the last line of the reply message to ctrl.last_reply */
--    const wordlist *W;
--    for (W = ctrl.message; W && W->next; W = W->next);
--    if (W) {
--        safe_free(ctrl.last_reply);
--        ctrl.last_reply = xstrdup(W->key);
--    }
--
      debugs(9, 3, HERE << "state=" << state << ", code=" << ctrl.replycode);
  }
  
@@@ -733,8 -733,8 +729,10 @@@ ServerStateData::doneSendingRequestBody
       */
  }
  
--wordlist *
--ServerStateData::parseControlReply(char *buf, size_t len, int *codep, size_t *used)
++/// Parses FTP server control response into ctrl structure fields,
++/// setting bytesUsed and returning true on success.
++bool
++ServerStateData::parseControlReply(size_t &bytesUsed)
  {
      char *s;
      char *sbuf;
      wordlist *head = NULL;
      wordlist *list;
      wordlist **tail = &head;
--    size_t offset;
      size_t linelen;
--    int code = -1;
      debugs(9, 3, HERE);
      /*
       * We need a NULL-terminated buffer for scanning, ick
       */
++    const size_t len = ctrl.offset;
      sbuf = (char *)xmalloc(len + 1);
--    xstrncpy(sbuf, buf, len + 1);
++    xstrncpy(sbuf, ctrl.buf, len + 1);
      end = sbuf + len - 1;
  
      while (*end != '\r' && *end != '\n' && end > sbuf)
      if (usable == 0) {
          debugs(9, 3, HERE << "didn't find end of line");
          safe_free(sbuf);
--        return NULL;
++        return false;
      }
  
      debugs(9, 3, HERE << len << " bytes to play with");
          if (linelen > 3)
              complete = (*s >= '0' && *s <= '9' && *(s + 3) == ' ');
  
--        if (complete)
--            code = atoi(s);
--
--        offset = 0;
--
--        if (linelen > 3)
--            if (*s >= '0' && *s <= '9' && (*(s + 3) == '-' || *(s + 3) == ' '))
--                offset = 4;
--
          list = new wordlist();
  
--        list->key = (char *)xmalloc(linelen - offset);
++        list->key = (char *)xmalloc(linelen);
  
--        xstrncpy(list->key, s + offset, linelen - offset);
++        xstrncpy(list->key, s, linelen);
  
          /* trace the FTP communication chat at level 2 */
--        debugs(9, 2, "ftp>> " << code << " " << list->key);
++        debugs(9, 2, "ftp>> " << list->key);
++
++        if (complete) {
++            // use list->key for last_reply because s contains the new line
++            ctrl.last_reply = xstrdup(list->key + 4);
++            ctrl.replycode = atoi(list->key);
++        }
  
          *tail = list;
  
          tail = &list->next;
      }
  
--    *used = (size_t) (s - sbuf);
++    bytesUsed = static_cast<size_t>(s - sbuf);
      safe_free(sbuf);
  
--    if (!complete)
++    if (!complete) {
          wordlistDestroy(&head);
++        return false;
++    }
  
--    if (codep)
--        *codep = code;
--
--    return head;
++    ctrl.message = head;
++    assert(ctrl.replycode >= 0);
++    assert(ctrl.last_reply);
++    assert(ctrl.message);
++    return true;
  }
  
  }; // namespace Ftp
diff --cc src/FtpServer.h
index 7dfc7854eb9c8ab3f648969355fbeb7b34e79e0c,7dfc7854eb9c8ab3f648969355fbeb7b34e79e0c..705d04f6e3a8ec5a8cb438027a99ab3320b3ab10
@@@ -113,7 -113,7 +113,7 @@@ protected
      virtual void doneSendingRequestBody();
  
  private:
--    static wordlist *parseControlReply(char *buf, size_t len, int *codep, size_t *used);
++    bool parseControlReply(size_t &bytesUsed);
  
      CBDATA_CLASS2(ServerStateData);
  };
index c80f8e61e7e1a6dae71ce021e50481a897523aa0,c80f8e61e7e1a6dae71ce021e50481a897523aa0..4726add3750e78ec303b45b2fac5a2456fba0e41
@@@ -163,6 -163,6 +163,7 @@@ static const HttpHeaderFieldAttrs Heade
      {"Front-End-Https", HDR_FRONT_END_HTTPS, ftStr},
      {"FTP-Command", HDR_FTP_COMMAND, ftStr},
      {"FTP-Arguments", HDR_FTP_ARGUMENTS, ftStr},
++    {"FTP-Pre", HDR_FTP_PRE, ftStr},
      {"FTP-Status", HDR_FTP_STATUS, ftInt},
      {"FTP-Reason", HDR_FTP_REASON, ftStr},
      {"Other:", HDR_OTHER, ftStr}      /* ':' will not allow matches */
index bbe97064148b2293efb207f6fc52fa3da7b7da08,bbe97064148b2293efb207f6fc52fa3da7b7da08..6930bd51116aa7d9c48b7a52563c5db51840da40
@@@ -148,6 -148,6 +148,7 @@@ typedef enum 
      HDR_FRONT_END_HTTPS,                /**< MS Exchange custom header we may have to add */
      HDR_FTP_COMMAND,                    /**< Internal header for FTP command */
      HDR_FTP_ARGUMENTS,                  /**< Internal header for FTP command arguments */
++    HDR_FTP_PRE,                        /**< Custom: Contains leading FTP control response lines */
      HDR_FTP_STATUS,                     /**< Internal header for FTP reply status */
      HDR_FTP_REASON,                     /**< Internal header for FTP reply reason */
      HDR_OTHER,                          /**< internal tag value for "unknown" headers */
@@@ -300,6 -300,6 +301,10 @@@ private
  };
  
  int httpHeaderParseQuotedString(const char *start, const int len, String *val);
++
++/// quotes string using RFC 2616 quoted-string rules
++String httpHeaderQuoteString(const char *raw);
++
  int httpHeaderHasByNameListMember(const HttpHeader * hdr, const char *name, const char *member, const char separator);
  void httpHeaderUpdate(HttpHeader * old, const HttpHeader * fresh, const HttpHeaderMask * denied_mask);
  void httpHeaderCalcMask(HttpHeaderMask * mask, http_hdr_type http_hdr_type_enums[], size_t count);
index 1c3c47c6ea40030a917a27546734f262738c7f3c,1c3c47c6ea40030a917a27546734f262738c7f3c..d49dccfe166ab737549bd0ae06d81bd07035171d
@@@ -5437,23 -5437,23 +5437,19 @@@ FtpPrintReply(MemBuf &mb, const HttpRep
  {
      const HttpHeader &header = reply->header;
  
--    char status[4];
--    if (header.has(HDR_FTP_STATUS))
--        snprintf(status, sizeof(status), "%i", header.getInt(HDR_FTP_STATUS));
--    else
--        status[0] = '\0';
--
      HttpHeaderPos pos = HttpHeaderInitPos;
--    const HttpHeaderEntry *e = header.getEntry(&pos);
--    while (e) {
--        const HttpHeaderEntry *const next = header.getEntry(&pos);
--        if (e->id == HDR_FTP_REASON) {
--            const bool isLastLine = next == NULL || next->id != HDR_FTP_REASON;
--            const int separator = status[0] == '\0' || isLastLine ? ' ' : '-';
--            mb.Printf("%s%s%c%s\r\n", prefix, status, separator,
--                      e->value.termedBuf());
++    while (const HttpHeaderEntry *e = header.getEntry(&pos)) {
++        if (e->id == HDR_FTP_PRE) {
++            String raw;
++            if (httpHeaderParseQuotedString(e->value.rawBuf(), e->value.size(), &raw))
++                mb.Printf("%s\r\n", raw.termedBuf());
          }
--        e = next;
++    }
++
++    if (header.has(HDR_FTP_STATUS)) {
++        const char *reason = header.getStr(HDR_FTP_REASON);
++        mb.Printf("%i %s\r\n", header.getInt(HDR_FTP_STATUS),
++                  (reason ? reason : 0));
      }
  }