From d4dc3ad51fa64fd167b19c9b89d19a2dea148149 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 12 May 2026 11:55:35 +0200 Subject: [PATCH 01/45] add check for FTP command format --- src/clients/FtpClient.cc | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index c9db1ea7e61..9e41bedbdd5 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -822,10 +822,20 @@ Ftp::Client::dataClosed(const CommCloseCbParams &) void Ftp::Client::writeCommand(const char *buf) -{ +{ + // Check that the command has the following format: \r\n. + // Reject any CR or LF characters in the command. + const auto len = strlen(buf); + Assure(len >= 2); + + if(buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len -2){ + debugs(9, 2, "refuse to write malformed FTP commands"); + failed(ERR_FTP_FAILURE, 0); + return; + } + char *ebuf; /* trace FTP protocol communications at level 2 */ - debugs(9, 2, "ftp<< " << buf); if (Config.Ftp.telnet) ebuf = escapeIAC(buf); From 5028f33ac857e13e0eb9a72250f8c5a41988fe9c Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 12 May 2026 11:56:47 +0200 Subject: [PATCH 02/45] adjust command comment --- src/clients/FtpClient.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 9e41bedbdd5..fc2af151922 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -823,8 +823,7 @@ Ftp::Client::dataClosed(const CommCloseCbParams &) void Ftp::Client::writeCommand(const char *buf) { - // Check that the command has the following format: \r\n. - // Reject any CR or LF characters in the command. + // Check that the command ends with "\r\n" and reject any CR or LF characters in the command. const auto len = strlen(buf); Assure(len >= 2); From 03d61d73081385500b85fa9d65021cb23051ce93 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 12 May 2026 11:58:47 +0200 Subject: [PATCH 03/45] fix typo --- src/clients/FtpClient.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index fc2af151922..72d4b17e426 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -827,7 +827,7 @@ Ftp::Client::writeCommand(const char *buf) const auto len = strlen(buf); Assure(len >= 2); - if(buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len -2){ + if(buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len-2){ debugs(9, 2, "refuse to write malformed FTP commands"); failed(ERR_FTP_FAILURE, 0); return; From 5afbe4b140ac348fa5d66b26226aa06bb8534a7d Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 12 May 2026 13:37:50 +0200 Subject: [PATCH 04/45] remove assure and merge in condition --- src/clients/FtpClient.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 72d4b17e426..119b3313aab 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -825,10 +825,8 @@ Ftp::Client::writeCommand(const char *buf) { // Check that the command ends with "\r\n" and reject any CR or LF characters in the command. const auto len = strlen(buf); - Assure(len >= 2); - - if(buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len-2){ - debugs(9, 2, "refuse to write malformed FTP commands"); + if(len < 2 || buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len-2){ + debugs(9, 2, "refuse to write malformed FTP command"); failed(ERR_FTP_FAILURE, 0); return; } From 0f614e2b35bdf95e473e799b5bac813ce4e0b714 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 12 May 2026 13:41:20 +0200 Subject: [PATCH 05/45] add back debug --- src/clients/FtpClient.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 119b3313aab..7f7ed959d99 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -833,6 +833,7 @@ Ftp::Client::writeCommand(const char *buf) char *ebuf; /* trace FTP protocol communications at level 2 */ + debugs(9, 2, "ftp<< " << buf); if (Config.Ftp.telnet) ebuf = escapeIAC(buf); From b0eef7d189fe5402afa581e7a83766d8fe447531 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 12 May 2026 13:45:59 +0200 Subject: [PATCH 06/45] remove weird char --- src/clients/FtpClient.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 7f7ed959d99..9a7c4042170 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -825,7 +825,7 @@ Ftp::Client::writeCommand(const char *buf) { // Check that the command ends with "\r\n" and reject any CR or LF characters in the command. const auto len = strlen(buf); - if(len < 2 || buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len-2){ + if(len < 2 || buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len-2){ debugs(9, 2, "refuse to write malformed FTP command"); failed(ERR_FTP_FAILURE, 0); return; From e5b8579abfe7e3574e5df85c44756c2cd0ef85e8 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 12 May 2026 13:53:23 +0200 Subject: [PATCH 07/45] remove trailing whitespace --- src/clients/FtpClient.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 9a7c4042170..3576db7a0f3 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -822,7 +822,7 @@ Ftp::Client::dataClosed(const CommCloseCbParams &) void Ftp::Client::writeCommand(const char *buf) -{ +{ // Check that the command ends with "\r\n" and reject any CR or LF characters in the command. const auto len = strlen(buf); if(len < 2 || buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len-2){ From 1af928d89b82d64c2a6aee94da3c8e0d514dbbc5 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 12 May 2026 14:07:10 +0200 Subject: [PATCH 08/45] fix if format --- src/clients/FtpClient.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 3576db7a0f3..94a6e943f70 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -825,7 +825,7 @@ Ftp::Client::writeCommand(const char *buf) { // Check that the command ends with "\r\n" and reject any CR or LF characters in the command. const auto len = strlen(buf); - if(len < 2 || buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len-2){ + if (len < 2 || buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len-2) { debugs(9, 2, "refuse to write malformed FTP command"); failed(ERR_FTP_FAILURE, 0); return; From 2c46042d0b1db851a9df2dbf9fe3bb9f55dd8f83 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 12 May 2026 17:46:47 +0200 Subject: [PATCH 09/45] fix PR review --- src/clients/FtpClient.cc | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 94a6e943f70..cfd653442fc 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -823,10 +823,16 @@ Ftp::Client::dataClosed(const CommCloseCbParams &) void Ftp::Client::writeCommand(const char *buf) { - // Check that the command ends with "\r\n" and reject any CR or LF characters in the command. - const auto len = strlen(buf); - if (len < 2 || buf[len-2] != '\r' || buf[len-1] != '\n' || strcspn(buf, crlf) != len-2) { - debugs(9, 2, "refuse to write malformed FTP command"); + const auto bufLen = strlen(buf); + + // The caller must supply a non-empty command followed by CRLF. + // TODO: Move CRLF appending code from callers to here. + Assure(bufLen > 2); + Assure(buf[bufLen-2] == '\r'); + Assure(buf[bufLen-1] != '\n'); + const auto crlfIndex = strcspn(buf, crlf); + if (crlfIndex != bufLen-2) { + debugs(9, 2, "cannot write malformed FTP command: "< Date: Tue, 12 May 2026 18:00:14 +0200 Subject: [PATCH 10/45] fix typo --- src/clients/FtpClient.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index cfd653442fc..79d754fecd7 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -829,7 +829,7 @@ Ftp::Client::writeCommand(const char *buf) // TODO: Move CRLF appending code from callers to here. Assure(bufLen > 2); Assure(buf[bufLen-2] == '\r'); - Assure(buf[bufLen-1] != '\n'); + Assure(buf[bufLen-1] == '\n'); const auto crlfIndex = strcspn(buf, crlf); if (crlfIndex != bufLen-2) { debugs(9, 2, "cannot write malformed FTP command: "< Date: Tue, 12 May 2026 18:13:46 +0200 Subject: [PATCH 11/45] log bad char and it's position --- src/clients/FtpClient.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 79d754fecd7..4cef18552da 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -832,7 +832,8 @@ Ftp::Client::writeCommand(const char *buf) Assure(buf[bufLen-1] == '\n'); const auto crlfIndex = strcspn(buf, crlf); if (crlfIndex != bufLen-2) { - debugs(9, 2, "cannot write malformed FTP command: "< Date: Mon, 18 May 2026 11:44:46 +0200 Subject: [PATCH 12/45] move logic to Uri.cc --- src/anyp/Uri.cc | 30 ++++++++++++++++++++++++++++++ src/clients/FtpClient.cc | 8 +------- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 8eae3429a63..aac20af9dfd 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -69,6 +69,19 @@ PathChars() return pathValid; } +/** + * containsEscapedFtpCommandDelimiter is used to check for unescaped FTP command delimiters in the different components of a URI. + * The function unescapes the input string and checks for the presence of carriage return (\r) or line feed (\n) characters. + * \param s the string to check for unescaped FTP command delimiters + * \return true if the string contains unescaped FTP command delimiters, false otherwise + */ +static bool containsEscapedFtpCommandDelimiter(const char *s){ + LOCAL_ARRAY(char, unescaped, MAX_URL); + xstrncpy(unescaped, s, MAX_URL); + rfc1738_unescape(unescaped); + return strpbrk(unescaped, "\r\n") != nullptr; +} + /** * Governed by RFC 3986 section 2.1 */ @@ -570,6 +583,23 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) } } + if(scheme == AnyP::PROTO_FTP){ + if(strpbrk(login, "\r\n")){ + debugs(23, 2, "error: FTP login contains unescaped FTP command delimiters"); + return false; + } + + if(containsEscapedFtpCommandDelimiter(urlpath)){ + debugs(23, 2, "error: FTP path contains unescaped FTP command delimiters"); + return false; + } + + if(containsEscapedFtpCommandDelimiter(foundHost)){ + debugs(23, 2, "error: FTP host contains unescaped FTP command delimiters"); + return false; + } + } + setScheme(scheme); path(urlpath); host(foundHost); diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 4cef18552da..966ece19b1a 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -830,13 +830,7 @@ Ftp::Client::writeCommand(const char *buf) Assure(bufLen > 2); Assure(buf[bufLen-2] == '\r'); Assure(buf[bufLen-1] == '\n'); - const auto crlfIndex = strcspn(buf, crlf); - if (crlfIndex != bufLen-2) { - const auto invalidChar = buf[crlfIndex] == '\r' ? "\\r" : "\\n"; - debugs(9, 2, "cannot write malformed FTP command: "< Date: Mon, 18 May 2026 12:09:57 +0200 Subject: [PATCH 13/45] refactor function --- src/anyp/Uri.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index aac20af9dfd..c93742648d5 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -76,10 +76,16 @@ PathChars() * \return true if the string contains unescaped FTP command delimiters, false otherwise */ static bool containsEscapedFtpCommandDelimiter(const char *s){ + const char *ftpDelimiter = "\r\n"; + + // s might already be unescped + if(strpbrk(s, ftpDelimiter)) + return true; + LOCAL_ARRAY(char, unescaped, MAX_URL); xstrncpy(unescaped, s, MAX_URL); rfc1738_unescape(unescaped); - return strpbrk(unescaped, "\r\n") != nullptr; + return strpbrk(unescaped, ftpDelimiter) != nullptr; } /** @@ -584,7 +590,7 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) } if(scheme == AnyP::PROTO_FTP){ - if(strpbrk(login, "\r\n")){ + if(containsEscapedFtpCommandDelimiter(login)){ debugs(23, 2, "error: FTP login contains unescaped FTP command delimiters"); return false; } From 645e8fc37feb0a0dbd18b74b652ad92b379fa469 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Mon, 18 May 2026 13:09:06 +0200 Subject: [PATCH 14/45] remove whitesapces --- src/anyp/Uri.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index c93742648d5..df3786a54f9 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -77,7 +77,7 @@ PathChars() */ static bool containsEscapedFtpCommandDelimiter(const char *s){ const char *ftpDelimiter = "\r\n"; - + // s might already be unescped if(strpbrk(s, ftpDelimiter)) return true; From 699b944b9ea831fdfddc359e95d1b916afc1762c Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Mon, 18 May 2026 11:22:38 +0000 Subject: [PATCH 15/45] run scripts/source-maintenance.sh --- src/anyp/Uri.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index df3786a54f9..7173473c1a4 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -75,7 +75,7 @@ PathChars() * \param s the string to check for unescaped FTP command delimiters * \return true if the string contains unescaped FTP command delimiters, false otherwise */ -static bool containsEscapedFtpCommandDelimiter(const char *s){ +static bool containsEscapedFtpCommandDelimiter(const char *s) { const char *ftpDelimiter = "\r\n"; // s might already be unescped @@ -589,18 +589,18 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) } } - if(scheme == AnyP::PROTO_FTP){ - if(containsEscapedFtpCommandDelimiter(login)){ + if(scheme == AnyP::PROTO_FTP) { + if(containsEscapedFtpCommandDelimiter(login)) { debugs(23, 2, "error: FTP login contains unescaped FTP command delimiters"); return false; } - if(containsEscapedFtpCommandDelimiter(urlpath)){ + if(containsEscapedFtpCommandDelimiter(urlpath)) { debugs(23, 2, "error: FTP path contains unescaped FTP command delimiters"); return false; } - if(containsEscapedFtpCommandDelimiter(foundHost)){ + if(containsEscapedFtpCommandDelimiter(foundHost)) { debugs(23, 2, "error: FTP host contains unescaped FTP command delimiters"); return false; } From 717252677a8a44386c94be5b532e622d4f4f25fe Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Mon, 18 May 2026 16:12:34 +0200 Subject: [PATCH 16/45] replace c-string with SBuf --- src/anyp/Uri.cc | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 7173473c1a4..19aea97d486 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -70,22 +70,19 @@ PathChars() } /** - * containsEscapedFtpCommandDelimiter is used to check for unescaped FTP command delimiters in the different components of a URI. - * The function unescapes the input string and checks for the presence of carriage return (\r) or line feed (\n) characters. + * containsFtpCommandDelimiter checks for unescaped FTP command delimiters in a string. * \param s the string to check for unescaped FTP command delimiters - * \return true if the string contains unescaped FTP command delimiters, false otherwise + * \return true if s contains unescaped FTP command delimiters, false otherwise */ -static bool containsEscapedFtpCommandDelimiter(const char *s) { - const char *ftpDelimiter = "\r\n"; +static bool containsFtpCommandDelimiter(const SBuf &s){ + const auto crlf = CharacterSet("crlf", "\r\n"); - // s might already be unescped - if(strpbrk(s, ftpDelimiter)) + // s might already be unescaped + if(s.findFirstOf(crlf) != SBuf::npos) return true; - LOCAL_ARRAY(char, unescaped, MAX_URL); - xstrncpy(unescaped, s, MAX_URL); - rfc1738_unescape(unescaped); - return strpbrk(unescaped, ftpDelimiter) != nullptr; + const auto decoded = AnyP::Uri::Decode(s); + return decoded && decoded->findFirstOf(crlf) != SBuf::npos; } /** @@ -589,18 +586,19 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) } } + const auto loginInfo = SBuf(login); if(scheme == AnyP::PROTO_FTP) { - if(containsEscapedFtpCommandDelimiter(login)) { + if(containsFtpCommandDelimiter(loginInfo)) { debugs(23, 2, "error: FTP login contains unescaped FTP command delimiters"); return false; } - if(containsEscapedFtpCommandDelimiter(urlpath)) { + if(containsFtpCommandDelimiter(SBuf(urlpath))) { debugs(23, 2, "error: FTP path contains unescaped FTP command delimiters"); return false; } - if(containsEscapedFtpCommandDelimiter(foundHost)) { + if(containsFtpCommandDelimiter(SBuf(foundHost))) { debugs(23, 2, "error: FTP host contains unescaped FTP command delimiters"); return false; } @@ -609,7 +607,7 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) setScheme(scheme); path(urlpath); host(foundHost); - userInfo(SBuf(login)); + userInfo(loginInfo); port(foundPort); return true; From 773b02ce8bdf6bbdeb45d67da70534d48bebbad1 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 19 May 2026 07:31:32 +0200 Subject: [PATCH 17/45] Fix linter --- src/anyp/Uri.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 19aea97d486..225d6825e4b 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -74,7 +74,8 @@ PathChars() * \param s the string to check for unescaped FTP command delimiters * \return true if s contains unescaped FTP command delimiters, false otherwise */ -static bool containsFtpCommandDelimiter(const SBuf &s){ +static bool containsFtpCommandDelimiter(const SBuf &s) +{ const auto crlf = CharacterSet("crlf", "\r\n"); // s might already be unescaped From 9759307b4321c990779f1cd3298eb3dc3e460b5a Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 26 May 2026 09:46:56 +0200 Subject: [PATCH 18/45] update comment of containsFtpCommandDelimiter --- src/anyp/Uri.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 225d6825e4b..51bec772c74 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -70,15 +70,15 @@ PathChars() } /** - * containsFtpCommandDelimiter checks for unescaped FTP command delimiters in a string. - * \param s the string to check for unescaped FTP command delimiters - * \return true if s contains unescaped FTP command delimiters, false otherwise + * containsFtpCommandDelimiter checks for FTP command delimiters in a string. + * \param s the string to check for FTP command delimiters. s might already be decoded. + * \return true if s contains FTP command delimiters, false otherwise. */ static bool containsFtpCommandDelimiter(const SBuf &s) { const auto crlf = CharacterSet("crlf", "\r\n"); - // s might already be unescaped + // s might already be decoded if(s.findFirstOf(crlf) != SBuf::npos) return true; From bf23c6de6d957f3efc0ea71a76ca31e672c5fab9 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Tue, 26 May 2026 09:48:16 +0200 Subject: [PATCH 19/45] update debug messages --- src/anyp/Uri.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 51bec772c74..a2b0f4a0b31 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -590,17 +590,17 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) const auto loginInfo = SBuf(login); if(scheme == AnyP::PROTO_FTP) { if(containsFtpCommandDelimiter(loginInfo)) { - debugs(23, 2, "error: FTP login contains unescaped FTP command delimiters"); + debugs(23, 2, "error: FTP login contains FTP command delimiters"); return false; } if(containsFtpCommandDelimiter(SBuf(urlpath))) { - debugs(23, 2, "error: FTP path contains unescaped FTP command delimiters"); + debugs(23, 2, "error: FTP path contains FTP command delimiters"); return false; } if(containsFtpCommandDelimiter(SBuf(foundHost))) { - debugs(23, 2, "error: FTP host contains unescaped FTP command delimiters"); + debugs(23, 2, "error: FTP host contains FTP command delimiters"); return false; } } From 0335fd3a85a4d36b1e3caed6721e87d7549a20d2 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Wed, 27 May 2026 15:44:30 +0200 Subject: [PATCH 20/45] avoid decoding when checking for CRLF --- src/anyp/Uri.cc | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index a2b0f4a0b31..dfab0516f32 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -71,19 +71,26 @@ PathChars() /** * containsFtpCommandDelimiter checks for FTP command delimiters in a string. - * \param s the string to check for FTP command delimiters. s might already be decoded. + * \param s the string to check for FTP command delimiters. s shouldn't not be decoded. * \return true if s contains FTP command delimiters, false otherwise. */ static bool containsFtpCommandDelimiter(const SBuf &s) { - const auto crlf = CharacterSet("crlf", "\r\n"); - - // s might already be decoded - if(s.findFirstOf(crlf) != SBuf::npos) - return true; + Parser::Tokenizer tk(s); + const auto nonPercent = CharacterSet("percent", "%").complement("non-percent"); + while (!tk.atEnd()) { + tk.skipAll(nonPercent); - const auto decoded = AnyP::Uri::Decode(s); - return decoded && decoded->findFirstOf(crlf) != SBuf::npos; + if (tk.skip('%')) { + int64_t hex1 = 0, hex2 = 0; + if (tk.int64(hex1, 16, false, 1) && tk.int64(hex2, 16, false, 1)){ + const auto decoded = static_cast((hex1 << 4) | hex2); + if (decoded == '\r' || decoded == '\n') + return true; + } + } + } + return false; } /** @@ -461,8 +468,6 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) *t = 0; strncpy((char *) foundHost, t + 1, sizeof(foundHost)-1); foundHost[sizeof(foundHost)-1] = '\0'; - // Bug 4498: URL-unescape the login info after extraction - rfc1738_unescape(login); } /* Is there any host information? (we should eventually parse it above) */ @@ -587,9 +592,8 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) } } - const auto loginInfo = SBuf(login); if(scheme == AnyP::PROTO_FTP) { - if(containsFtpCommandDelimiter(loginInfo)) { + if(containsFtpCommandDelimiter(SBuf(login))) { debugs(23, 2, "error: FTP login contains FTP command delimiters"); return false; } @@ -605,10 +609,14 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) } } + // Bug 4498: URL-unescape the login info after extraction + if (*login) + rfc1738_unescape(login); + setScheme(scheme); path(urlpath); host(foundHost); - userInfo(loginInfo); + userInfo(SBuf(login)); port(foundPort); return true; From 75000be43caf74c21c985f613761ca3635947afd Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Wed, 27 May 2026 15:48:54 +0200 Subject: [PATCH 21/45] fix linter --- src/anyp/Uri.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index dfab0516f32..c61a590015b 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -83,7 +83,7 @@ static bool containsFtpCommandDelimiter(const SBuf &s) if (tk.skip('%')) { int64_t hex1 = 0, hex2 = 0; - if (tk.int64(hex1, 16, false, 1) && tk.int64(hex2, 16, false, 1)){ + if (tk.int64(hex1, 16, false, 1) && tk.int64(hex2, 16, false, 1)) { const auto decoded = static_cast((hex1 << 4) | hex2); if (decoded == '\r' || decoded == '\n') return true; From 97711b57176dbe1fba1230a32c0bcaa9af8ba7f9 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Thu, 28 May 2026 14:59:27 +0200 Subject: [PATCH 22/45] Fix typo Co-authored-by: Ricardo Ferreira Ribeiro --- src/anyp/Uri.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index c61a590015b..b22eae9b82c 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -71,7 +71,7 @@ PathChars() /** * containsFtpCommandDelimiter checks for FTP command delimiters in a string. - * \param s the string to check for FTP command delimiters. s shouldn't not be decoded. + * \param s the string to check for FTP command delimiters. s should not be decoded. * \return true if s contains FTP command delimiters, false otherwise. */ static bool containsFtpCommandDelimiter(const SBuf &s) From 53afbfc051f5451a75c4773c1b736e304e49a1ce Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Thu, 4 Jun 2026 14:16:17 +0200 Subject: [PATCH 23/45] use decode in check and remove unused proxy_host --- src/anyp/Uri.cc | 39 +++++++++++---------------------------- src/clients/FtpGateway.cc | 7 +------ 2 files changed, 12 insertions(+), 34 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index b22eae9b82c..eef3b2a4f3a 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -70,26 +70,20 @@ PathChars() } /** - * containsFtpCommandDelimiter checks for FTP command delimiters in a string. - * \param s the string to check for FTP command delimiters. s should not be decoded. - * \return true if s contains FTP command delimiters, false otherwise. + * validateFtpUrlComponent validates that the given URL component does not contain delimiters in a FTP request. + * \param s the URL component to validate. s should not have been previously decoded. + * \return true if the URL component is invalid, i.e., contains delimiters, or cannot be decoded, false otherwise. */ -static bool containsFtpCommandDelimiter(const SBuf &s) +static bool validateFtpUrlComponent(const SBuf &s) { - Parser::Tokenizer tk(s); - const auto nonPercent = CharacterSet("percent", "%").complement("non-percent"); - while (!tk.atEnd()) { - tk.skipAll(nonPercent); + const auto crlf = CharacterSet("crlf", "\r\n"); + const auto decoded = AnyP::Uri::Decode(s); - if (tk.skip('%')) { - int64_t hex1 = 0, hex2 = 0; - if (tk.int64(hex1, 16, false, 1) && tk.int64(hex2, 16, false, 1)) { - const auto decoded = static_cast((hex1 << 4) | hex2); - if (decoded == '\r' || decoded == '\n') - return true; - } - } + if(!decoded || decoded->findFirstOf(crlf) != SBuf::npos) { + debugs(23, 2, "ERROR: FTP URL is invalid; invalid component="<haveControlChannel("ftpSendUser")) return; - if (ftpState->proxy_host != nullptr) - snprintf(cbuf, CTRL_BUFLEN, "USER %s@%s\r\n", ftpState->user, ftpState->request->url.host()); - else - snprintf(cbuf, CTRL_BUFLEN, "USER %s\r\n", ftpState->user); + snprintf(cbuf, CTRL_BUFLEN, "USER %s\r\n", ftpState->user); ftpState->writeCommand(cbuf); From 5af247af3ba315845c20759084eb70171d441b20 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Fri, 5 Jun 2026 09:32:21 +0200 Subject: [PATCH 24/45] small fixes --- src/anyp/Uri.cc | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index eef3b2a4f3a..f4fe651ad97 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -70,16 +70,15 @@ PathChars() } /** - * validateFtpUrlComponent validates that the given URL component does not contain delimiters in a FTP request. - * \param s the URL component to validate. s should not have been previously decoded. - * \return true if the URL component is invalid, i.e., contains delimiters, or cannot be decoded, false otherwise. + * containsFtpCommandDelimiter validates that the given URL component does not contain delimiters in a FTP request. + * \param s the URL component to validate. s should have been previously decoded. + * \return true if the URL component contains FTP delimiters, false otherwise. */ -static bool validateFtpUrlComponent(const SBuf &s) +static bool containsFtpCommandDelimiter(const SBuf &s) { - const auto crlf = CharacterSet("crlf", "\r\n"); - const auto decoded = AnyP::Uri::Decode(s); + static const auto crlf = CharacterSet::CR + CharacterSet::LF; - if(!decoded || decoded->findFirstOf(crlf) != SBuf::npos) { + if(!s || s.findFirstOf(crlf) != SBuf::npos) { debugs(23, 2, "ERROR: FTP URL is invalid; invalid component="< Date: Fri, 5 Jun 2026 10:46:02 +0200 Subject: [PATCH 25/45] fix compiler and comment --- src/anyp/Uri.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index f4fe651ad97..4c2ee415e3d 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -78,7 +78,7 @@ static bool containsFtpCommandDelimiter(const SBuf &s) { static const auto crlf = CharacterSet::CR + CharacterSet::LF; - if(!s || s.findFirstOf(crlf) != SBuf::npos) { + if (s.findFirstOf(crlf) != SBuf::npos) { debugs(23, 2, "ERROR: FTP URL is invalid; invalid component="< Date: Fri, 5 Jun 2026 10:51:41 +0200 Subject: [PATCH 26/45] revert removal of proxy_host --- src/clients/FtpGateway.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc index e9031ce41a2..71c3e8d1f67 100644 --- a/src/clients/FtpGateway.cc +++ b/src/clients/FtpGateway.cc @@ -115,6 +115,7 @@ class Gateway : public Ftp::Client char *filepath; char *dirpath; int64_t restart_offset; + char *proxy_host; size_t list_width; String cwd_message; char *old_filepath; @@ -340,6 +341,7 @@ Ftp::Gateway::Gateway(FwdState *fwdState): filepath(nullptr), dirpath(nullptr), restart_offset(0), + proxy_host(nullptr), list_width(0), old_filepath(nullptr), typecode('\0') @@ -1289,7 +1291,10 @@ ftpSendUser(Ftp::Gateway * ftpState) if (!ftpState || !ftpState->haveControlChannel("ftpSendUser")) return; - snprintf(cbuf, CTRL_BUFLEN, "USER %s\r\n", ftpState->user); + if (ftpState->proxy_host != nullptr) + snprintf(cbuf, CTRL_BUFLEN, "USER %s@%s\r\n", ftpState->user, ftpState->request->url.host()); + else + snprintf(cbuf, CTRL_BUFLEN, "USER %s\r\n", ftpState->user); ftpState->writeCommand(cbuf); From 0881b5dc7392425876c66b77a1375dc1eb7cda20 Mon Sep 17 00:00:00 2001 From: Ricardo Ferreira Ribeiro Date: Fri, 5 Jun 2026 12:38:42 +0200 Subject: [PATCH 27/45] remove invalid string from logs --- src/anyp/Uri.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 4c2ee415e3d..635654cdb64 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -79,7 +79,7 @@ static bool containsFtpCommandDelimiter(const SBuf &s) static const auto crlf = CharacterSet::CR + CharacterSet::LF; if (s.findFirstOf(crlf) != SBuf::npos) { - debugs(23, 2, "ERROR: FTP URL is invalid; invalid component="< Date: Mon, 15 Jun 2026 09:34:49 -0400 Subject: [PATCH 28/45] fixup: Clarified intended/validated bufLen scope --- src/clients/FtpClient.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 966ece19b1a..8b536f3d194 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -823,10 +823,9 @@ Ftp::Client::dataClosed(const CommCloseCbParams &) void Ftp::Client::writeCommand(const char *buf) { - const auto bufLen = strlen(buf); - // The caller must supply a non-empty command followed by CRLF. // TODO: Move CRLF appending code from callers to here. + const auto bufLen = strlen(buf); Assure(bufLen > 2); Assure(buf[bufLen-2] == '\r'); Assure(buf[bufLen-1] == '\n'); From 02a34530a02273e5b7007e6392475212c129da02 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 09:36:57 -0400 Subject: [PATCH 29/45] fixup: Annotated complex assertion condition, dependencies --- src/clients/FtpClient.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 8b536f3d194..14c8f23c577 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -829,6 +829,9 @@ Ftp::Client::writeCommand(const char *buf) Assure(bufLen > 2); Assure(buf[bufLen-2] == '\r'); Assure(buf[bufLen-1] == '\n'); + // If this assertion fails, then the caller has assembled an FTP command + // using input unchecked for embedded CRLF. For an example of such checks, + // see containsFtpCommandDelimiter() calls in AnyP::Uri::parse(). Assure(strcspn(buf, crlf) == bufLen-2); char *ebuf; From 9474c89121326f0662db42e45c2ce6fbdde23d30 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 09:57:33 -0400 Subject: [PATCH 30/45] fixup: Polished foundHost validation explanation The branch-added "XXX" comment is not really an XXX: We are not doing anything wrong here. We are avoiding an arguably expensive assertion. Also be more specific regarding why foundHost cannot have CRs or LFs. --- src/anyp/Uri.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 635654cdb64..0776ecb71e3 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -589,9 +589,13 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) const auto loginInfo = SBuf(login); if(scheme == AnyP::PROTO_FTP) { - // XXX: We do not need to validate the host because it cannot contain a raw CR or LF due to the way it is initialized. - // Moreover, the host is not used in a FTP command argument. - // Therefore, there is no risk of FTP command injection via the host component of a FTP URL. + // For CONNECTS, a parseHost() call above ensures that foundHost has + // no FTP command delimiters. For other methods, "whitespace is also + // a hostname delimiter" loop above ensures that. Optimization: Do + // not create an SBuf just to assert that invariant here because the + // host component is not used in FTP command arguments. + // Assure(!containsFtpCommandDelimiter(SBuf(foundHost))); + const auto urlpathDecoded = AnyP::Uri::Decode(SBuf(urlpath)); if(!urlpathDecoded || containsFtpCommandDelimiter(urlpathDecoded.value()) || containsFtpCommandDelimiter(loginInfo)) return false; From 29d0f8c990b2066308692503899cf02335d111af Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 10:04:53 -0400 Subject: [PATCH 31/45] fixup: Clarified loginInfo scope The new variable is used beyond the `if` statement below its definition. --- src/anyp/Uri.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 0776ecb71e3..65e8bed4d9d 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -588,6 +588,7 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) } const auto loginInfo = SBuf(login); + if(scheme == AnyP::PROTO_FTP) { // For CONNECTS, a parseHost() call above ensures that foundHost has // no FTP command delimiters. For other methods, "whitespace is also From 5b290a8d08bbe781815089765405e87a58900c04 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 10:08:17 -0400 Subject: [PATCH 32/45] fixup: Minor optimization: Perform the faster check first The `if` statement order does not matter match, but this order is faster if both components are problematic and this order helps clarify intended the urlpathDecoded usage scope. --- src/anyp/Uri.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 65e8bed4d9d..612f270e21a 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -597,8 +597,11 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) // host component is not used in FTP command arguments. // Assure(!containsFtpCommandDelimiter(SBuf(foundHost))); + if (containsFtpCommandDelimiter(loginInfo)) + return false; + const auto urlpathDecoded = AnyP::Uri::Decode(SBuf(urlpath)); - if(!urlpathDecoded || containsFtpCommandDelimiter(urlpathDecoded.value()) || containsFtpCommandDelimiter(loginInfo)) + if(!urlpathDecoded || containsFtpCommandDelimiter(urlpathDecoded.value())) return false; } From d7598569bc0246eadc495587e03d48122a48e767 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 10:12:29 -0400 Subject: [PATCH 33/45] fixup: Do not convert urlpath to SBuf twice. --- src/anyp/Uri.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 612f270e21a..89084a635d3 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -587,7 +587,9 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) } } + // Optimization: Convert legacy c-strings to SBuf once. const auto loginInfo = SBuf(login); + const auto urlpathInfo = SBuf(urlpath); if(scheme == AnyP::PROTO_FTP) { // For CONNECTS, a parseHost() call above ensures that foundHost has @@ -600,13 +602,13 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) if (containsFtpCommandDelimiter(loginInfo)) return false; - const auto urlpathDecoded = AnyP::Uri::Decode(SBuf(urlpath)); + const auto urlpathDecoded = AnyP::Uri::Decode(urlpathInfo); if(!urlpathDecoded || containsFtpCommandDelimiter(urlpathDecoded.value())) return false; } setScheme(scheme); - path(urlpath); + path(urlpathInfo); host(foundHost); userInfo(loginInfo); port(foundPort); From 9f48a4f114e9c3133ec977640338cd5ca4335672 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 10:20:20 -0400 Subject: [PATCH 34/45] fixup: Avoid "empty" words in C++ names Words like "info", "data", and "state" are popular in legacy Squid code, but they usually do not add anything to the meaning of the named entity and, hence, should be avoided. Highlighting that we are copying something is actually useful. --- src/anyp/Uri.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 89084a635d3..d061f7cb7cc 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -588,8 +588,8 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) } // Optimization: Convert legacy c-strings to SBuf once. - const auto loginInfo = SBuf(login); - const auto urlpathInfo = SBuf(urlpath); + const auto loginCopy = SBuf(login); + const auto urlpathCopy = SBuf(urlpath); if(scheme == AnyP::PROTO_FTP) { // For CONNECTS, a parseHost() call above ensures that foundHost has @@ -597,20 +597,20 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) // a hostname delimiter" loop above ensures that. Optimization: Do // not create an SBuf just to assert that invariant here because the // host component is not used in FTP command arguments. - // Assure(!containsFtpCommandDelimiter(SBuf(foundHost))); + // Assure(!containsFtpCommandDelimiter(foundHostCopy)); - if (containsFtpCommandDelimiter(loginInfo)) + if (containsFtpCommandDelimiter(loginCopy)) return false; - const auto urlpathDecoded = AnyP::Uri::Decode(urlpathInfo); + const auto urlpathDecoded = AnyP::Uri::Decode(urlpathCopy); if(!urlpathDecoded || containsFtpCommandDelimiter(urlpathDecoded.value())) return false; } setScheme(scheme); - path(urlpathInfo); + path(urlpathCopy); host(foundHost); - userInfo(loginInfo); + userInfo(loginCopy); port(foundPort); return true; From f0561099bcc37b64ccb5dc1ad25a41aa738d88b9 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 11:32:25 -0400 Subject: [PATCH 35/45] fixup: AnyP::Uri::parse() helpers should throw on errors AnyP::Uri::parse() has a try-catch wrapper around its code, making it feasible to throw on parsing errors inside [the affected part of] that legacy code. In general, throwing on parsing errors simplifies and makes safer parsing code, so we usually want to throw in that context rather than rely on boolean return values. Also use "///" for Doxygen comments because they often consume fewer lines and simplify consistent formatting. --- src/anyp/Uri.cc | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index d061f7cb7cc..be44a13a02a 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -69,21 +69,16 @@ PathChars() return pathValid; } -/** - * containsFtpCommandDelimiter validates that the given URL component does not contain delimiters in a FTP request. - * \param s the URL component to validate. s should have been previously decoded. - * \return true if the URL component contains FTP delimiters, false otherwise. - */ -static bool containsFtpCommandDelimiter(const SBuf &s) +/// Throws when the given URL component contains a CR or LF character. +/// \param component is a URL part that needs to be checked +/// \prec component is not percent-encoded (e.g., it was decoded after extracting it from an HTTP request target) +static void +rejectFtpCommandDelimiters(const SBuf &component) { - static const auto crlf = CharacterSet::CR + CharacterSet::LF; - - if (s.findFirstOf(crlf) != SBuf::npos) { - debugs(23, 2, "ERROR: FTP URL contains command delimiter characters"); - return true; - } + static const auto delimiterCharacters = CharacterSet::CR + CharacterSet::LF; - return false; + if (component.findFirstOf(delimiterCharacters) != SBuf::npos) + throw TextException("URL contains an FTP command delimiter character", Here()); } /** @@ -594,17 +589,16 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) if(scheme == AnyP::PROTO_FTP) { // For CONNECTS, a parseHost() call above ensures that foundHost has // no FTP command delimiters. For other methods, "whitespace is also - // a hostname delimiter" loop above ensures that. Optimization: Do - // not create an SBuf just to assert that invariant here because the - // host component is not used in FTP command arguments. - // Assure(!containsFtpCommandDelimiter(foundHostCopy)); + // a hostname delimiter" loop above ensures that invariant as well. + // Optimization: Do not create an SBuf just to check that invariant + // because the host component is not used in FTP command arguments. - if (containsFtpCommandDelimiter(loginCopy)) - return false; + rejectFtpCommandDelimiters(loginCopy); - const auto urlpathDecoded = AnyP::Uri::Decode(urlpathCopy); - if(!urlpathDecoded || containsFtpCommandDelimiter(urlpathDecoded.value())) - return false; + if (const auto urlpathDecoded = AnyP::Uri::Decode(urlpathCopy)) + rejectFtpCommandDelimiters(*urlpathDecoded); + else + throw TextException("invalid percent-encoding in an FTP URL path", Here()); } setScheme(scheme); From 652e948839e7647334fa9a6c9ee666432be182ef Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 17:25:19 -0400 Subject: [PATCH 36/45] fixup: Name the set of FTP delimiter characters This might be useful in triaging lower-level code and for detecting CharactersSet reuse/duplication. --- src/anyp/Uri.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index be44a13a02a..3c5f6c1a229 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -75,9 +75,9 @@ PathChars() static void rejectFtpCommandDelimiters(const SBuf &component) { - static const auto delimiterCharacters = CharacterSet::CR + CharacterSet::LF; + static const auto delimiterChars = (CharacterSet::CR + CharacterSet::LF).rename("CRLF"); - if (component.findFirstOf(delimiterCharacters) != SBuf::npos) + if (component.findFirstOf(delimiterChars) != SBuf::npos) throw TextException("URL contains an FTP command delimiter character", Here()); } From 01608f5464027fa00ddad87291acffbde1738bd5 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 18:16:18 -0400 Subject: [PATCH 37/45] fixup: Documented why FTP is treated specially Also disclosed that we are going "too far" with these checks. --- src/anyp/Uri.cc | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 3c5f6c1a229..5a283c93261 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -587,11 +587,17 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) const auto urlpathCopy = SBuf(urlpath); if(scheme == AnyP::PROTO_FTP) { - // For CONNECTS, a parseHost() call above ensures that foundHost has - // no FTP command delimiters. For other methods, "whitespace is also - // a hostname delimiter" loop above ensures that invariant as well. - // Optimization: Do not create an SBuf just to check that invariant - // because the host component is not used in FTP command arguments. + // Some FTP URL components may be used to form FTP commands. FTP + // does not have a standard, consistently supported way of encoding + // CR and LF delimiters in command parameters, so we reject URIs + // containing those delimiters in those components. + + // These checks also apply to use cases where this Squid instance + // does not form an FTP command from the URL (e.g., we may forward + // the URL to another HTTP proxy instead). Even though doing so may + // break some "works without Squid" transactions, we cover those + // additional use cases to protect broken recipients because these + // delimiters affect command framing. rejectFtpCommandDelimiters(loginCopy); @@ -599,6 +605,12 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) rejectFtpCommandDelimiters(*urlpathDecoded); else throw TextException("invalid percent-encoding in an FTP URL path", Here()); + + // For CONNECTS, a parseHost() call above ensures that foundHost has + // no FTP command delimiters. For other methods, "whitespace is also + // a hostname delimiter" loop above ensures that invariant as well. + // Optimization: Do not create an SBuf just to check that invariant + // because the host component is not used in FTP command arguments. } setScheme(scheme); From 60f573d4833717da3c0bccaf3bc45f1f1f3e07d7 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 18:17:34 -0400 Subject: [PATCH 38/45] fixup: More common/readable `if` statement formatting % git grep 'if[(]' '*.cc' | wc -l 21 % git grep 'if [(]' '*.cc' | wc -l 14016 --- src/anyp/Uri.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 5a283c93261..1814ee7c6e4 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -586,7 +586,7 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) const auto loginCopy = SBuf(login); const auto urlpathCopy = SBuf(urlpath); - if(scheme == AnyP::PROTO_FTP) { + if (scheme == AnyP::PROTO_FTP) { // Some FTP URL components may be used to form FTP commands. FTP // does not have a standard, consistently supported way of encoding // CR and LF delimiters in command parameters, so we reject URIs From 4e9a4b7cf0dd457b0650dccc52fd02583480ac65 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 18:20:53 -0400 Subject: [PATCH 39/45] fixup: Updated a stale branch-added comment --- src/clients/FtpClient.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 14c8f23c577..0452336982d 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -831,7 +831,7 @@ Ftp::Client::writeCommand(const char *buf) Assure(buf[bufLen-1] == '\n'); // If this assertion fails, then the caller has assembled an FTP command // using input unchecked for embedded CRLF. For an example of such checks, - // see containsFtpCommandDelimiter() calls in AnyP::Uri::parse(). + // see rejectFtpCommandDelimiters() calls in AnyP::Uri::parse(). Assure(strcspn(buf, crlf) == bufLen-2); char *ebuf; From 516d1f0139c275018718227bb84d4cd7cb682cbb Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 18:25:37 -0400 Subject: [PATCH 40/45] fixup: Explicitly name code that forms FTP commands --- src/anyp/Uri.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 1814ee7c6e4..ca99feb806b 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -587,17 +587,17 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) const auto urlpathCopy = SBuf(urlpath); if (scheme == AnyP::PROTO_FTP) { - // Some FTP URL components may be used to form FTP commands. FTP - // does not have a standard, consistently supported way of encoding - // CR and LF delimiters in command parameters, so we reject URIs - // containing those delimiters in those components. + // Ftp::Gateway uses some FTP URL components to form FTP commands. + // FTP does not have a standard, consistently supported way of + // encoding CR and LF delimiters in command parameters, so we reject + // URIs containing those delimiters in those components. // These checks also apply to use cases where this Squid instance // does not form an FTP command from the URL (e.g., we may forward // the URL to another HTTP proxy instead). Even though doing so may // break some "works without Squid" transactions, we cover those // additional use cases to protect broken recipients because these - // delimiters affect command framing. + // delimiters affect FTP command framing. rejectFtpCommandDelimiters(loginCopy); From dde3c5349d957d1c41e3ee2d475d3fa7756c4491 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 15 Jun 2026 22:25:09 -0400 Subject: [PATCH 41/45] fixup: No Foo:: inside Foo --- src/anyp/Uri.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index ca99feb806b..a2b811099fe 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -601,7 +601,7 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) rejectFtpCommandDelimiters(loginCopy); - if (const auto urlpathDecoded = AnyP::Uri::Decode(urlpathCopy)) + if (const auto urlpathDecoded = Decode(urlpathCopy)) rejectFtpCommandDelimiters(*urlpathDecoded); else throw TextException("invalid percent-encoding in an FTP URL path", Here()); From becd5b4a86c5541a911907738b7ab3740b534b83 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 16 Jun 2026 17:03:25 -0400 Subject: [PATCH 42/45] OT: Do not attempt to reforward pinned requests ... to better tolerate Ftp::Relay::sendCommand() and similar exceptions. ERROR: Squid BUG: assurance failed: ... exception location: FtpClient.cc(835) writeCommand AsyncJob.cc(145) callException: assurance failed ... AsyncJob.cc(100) mustStop: Ftp::Relay will stop, reason: exception ... FATAL: check failed: !request->pinnedConnection() exception location: FwdState.cc(1119) connectStart When branch-added Ftp::Client::writeCommand() assertions throw, the worker dies instead of just killing the affected ftp_port transaction. `JobDialer::dial()` catches the BUG exception, as expected, but the worker still dies because `Client::swanSong()` cleanup code calls `FwdState::handleUnregisteredServerEnd()` which attempts to retry the failed FTP transaction despite true `request->pinnedConnection()`, which triggers another, this time uncaught, exception in `connectStart()`. Also documented suspicions that `FwdState::pinnedCanRetry()` should _always_ return false and, hence, should be removed (instead of adding more and more code/conditions when that method returns false). ---- I hope this out-of-scope change can be reverted (and merged separately) after we add CR protections to command parser and/or replace writeCommand() assertions with (moved) embedded new line handling code. --- src/FwdState.cc | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/FwdState.cc b/src/FwdState.cc index 62c33a67f84..b2b3cfa9148 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -1435,6 +1435,31 @@ FwdState::pinnedCanRetry() const if (request->flags.sslBumped) return false; + // Currently, all retries call useDestinations() which never calls + // usePinned() and usually calls connectStart(). The latter requires nil + // request->pinnedConnection(). + if (request->pinnedConnection()) + return false; + + // XXX: The only case where the condition above is false despite true + // `request->flags.pinned` is when HttpRequest::clientConnectionManager job + // disappears. However, in that case, we should probably return `false` + // anyway to avoid retries with a gone client! + + // XXX: FwdState never re-pins a connection. In other words, if FwdState + // fails while using a pinned connection, we never pin a new connection to + // the same HttpRequest::clientConnectionManager job. 2013 commit 693cb033 + // removed most re-pinning code. 2019 commit 3dde9e52 probably assumed that + // "FTP proxying and connection-based HTTP authentication" cases mentioned + // below might re-pin, so it added this method to avoid breaking those + // alleged cases, but that assumption was probably wrong then and/or is + // wrong now. + + // TODO: Confirm that "connection-based HTTP authentication" does not need + // some other fix and remove this method. Also adjust peer selection code to + // avoid computing/returning regular destinations (that cannot be used) + // after selecting a PINNED connection. + // The other pinned cases are FTP proxying and connection-based HTTP // authentication. TODO: Do these cases have restrictions? return true; From 21581bdc49106f797333128f3e6c67b808d0b92b Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 17 Jun 2026 11:41:21 -0400 Subject: [PATCH 43/45] Reject FTP commands with parameters containing CR characters FTP command syntax treats CR and LF characters as command terminators. All RFC 959 sightings of CR and LF either refer to the CRLF terminator or treat both characters the same way. For example, RFC 959 defines an FTP command parameter as a sequence of chars, where: ::= any of the 128 ASCII characters except and Squid should (and now does) restrict CR use to the command terminator sequence, especially since some FTP servers treat CRs as command delimiters -- if we continue to allow embedded CRs in FTP command parameters than our FtpClient::writeCommand() will assert when trying to forward those commands to the FTP server. Moreover, we already use that CR treatment when _parsing_ FTP responses (see Ftp::Client::parseControlReply()). When it comes to command termination, CRs are still optional. A new ban on CRs in FTP command parameter values means that Squid starts treating some FTP commands as syntactically invalid, using EarlyErrorKind::MalformedCommand for the first time since its inception in 2014 commit eacfca83. For example, `PWD\rQUIT` input is now invalid. N.B. This change removes "RFC 959 Section 3.1.1.5.2" reference. That reference was wrong: RFC Section 3.1 is about "data representations" used for file transfers rather than for command syntax. I probably missed this problem when the addition of that reference was requested at http://www.squid-cache.org/mail-archive/squid-dev/201408/0023.html --- src/servers/FtpServer.cc | 45 +++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index bb236a3bc6e..1a851ad73cb 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -641,15 +641,31 @@ Ftp::Server::parseOneRequest() { flags.readMore = false; // common for all but one case below - // OWS [ RWS ] OWS LF - - // InlineSpaceChars are isspace(3) or RFC 959 Section 3.1.1.5.2, except - // for the LF character that we must exclude here (but see FullWhiteSpace). - static const char * const InlineSpaceChars = " \f\r\t\v"; + // FTP command syntax is specified in RFC 959 Section 5.3. We generalize + // that grammar to parse all commands using the same code. We also relax the + // rules a little in hope to accommodate more real world use cases: + // * We allow ASCII HT, VT, and NP characters in various delimiters (in addition to SP). + // * We allow zero or more CR characters in command terminator (instead of exactly one). + // * We allow space characters before any command. + // + // command = BWS code [ RWS parameter ] OWS *CR LF + // code = 1*code_char + // parameter = 1*parameter_char ; without leading and trailing inner_space_chars + // code_char = ; any ASCII character other than space_char + // parameter_char = ; any ASCII character other than CR or LF + // BWS = 0*space_char ; optional "bad" space before the command code + // RWS = 1*inner_space_char ; required space before the command parameter + // OWS = 0*inner_space_char ; optional space after the command parameter + // inner_space_char = SP / HT / VT / NP ; space_char without CR and LF + // space_char = SP / HT / VT / NP / CR / LF; any isspace(3) character in "C" locale + + static const auto InlineSpaceChars = " \f\t\v"; static const CharacterSet InlineSpace = CharacterSet("Ftp::Inline", InlineSpaceChars); - static const CharacterSet FullWhiteSpace = (InlineSpace + CharacterSet::LF).rename("Ftp::FWS"); + static const CharacterSet CrLfChars = (CharacterSet::CR + CharacterSet::LF).rename("CRLF"); + static const CharacterSet FullWhiteSpace = (InlineSpace + CrLfChars).rename("Ftp::FWS"); static const CharacterSet CommandChars = FullWhiteSpace.complement("Ftp::Command"); - static const CharacterSet TailChars = CharacterSet::LF.complement("Ftp::Tail"); + // RFC 959 Section 5.3.2 excludes both CR and LF from and definitions + static const CharacterSet TailChars = CrLfChars.complement("Ftp::Tail"); // This set is used to ignore empty commands without allowing an attacker // to keep us endlessly busy by feeding us whitespace or empty commands. @@ -663,7 +679,7 @@ Ftp::Server::parseOneRequest() (void)tok.skipAll(LeadingSpace); // leading OWS and empty commands const bool parsed = tok.prefix(cmd, CommandChars); // required command - // note that the condition below will eat either RWS or trailing OWS + // note that the condition below eats leading RWS and trailing OWS, if any if (parsed && tok.skipAll(InlineSpace) && tok.prefix(params, TailChars)) { // now params may include trailing OWS // TODO: Support right-trimming using CharacterSet in Tokenizer instead @@ -682,8 +698,21 @@ Ftp::Server::parseOneRequest() return earlyError(EarlyErrorKind::HugeRequest); } + if (parsed) + (void)tok.skipAll(CharacterSet::CR); + // technically, we may skip multiple NLs below, but that is OK if (!parsed || !tok.skipAll(CharacterSet::LF)) { // did not find terminating LF yet + + if (!tok.remaining().isEmpty()) { + // We always consume all valid input, so any leftovers imply that we + // found something that we cannot parse now and will never parse if + // more input becomes available later (e.g., `PWD\rQUIT\n`). + changeState(fssError, "bad FTP command syntax"); + quitAfterError(nullptr); + return earlyError(EarlyErrorKind::MalformedCommand); + } + // we need more data, but can we buffer more? if (inBuf.length() >= Config.maxRequestHeaderSize) { changeState(fssError, "huge req"); From 56fdddd65da352ea772ca8f1ae0a1c19255b633e Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 17 Jun 2026 15:17:28 -0400 Subject: [PATCH 44/45] Centralize and "guarantee" FTP command checks This commit effectively reverts branch commit 5b6e34d1 changes. In that commit, we chose to reject problematic `ftp://` URLs in Uri::parse() rather than wait until they trigger a creation of a problematic FTP command in Ftp::Gateway because detecting (and rejecting) a problematic request ASAP is often the safest option. For example, an allowed request could cause problems in adaptation services or other parts of Squid ecosystem that carelessly converts URLs to FTP commands. However, the above choice comes with significant drawbacks: * It broke previously working cases where problematic `ftp://` URLs did not reach Ftp::Gateway (e.g., where they were rewritten by a REQMOD service or sent to a cache_peer). * Squid started rejecting `ftp://` URL paths with broken percent-encoding. For example, an HTTP GET request for `ftp://example.com/double%%` resulted in an HTTP 400 (Bad Request) ERR_INVALID_URL response instead of leading to Squid sending FTP commands with a different filename (i.e. `RETR double%`). That is a significant change that may break some "working" use cases as well. * Finally, URI-based controls missed another CRLF injection point (now addressed in the previous branch commit 21581bdc), proving that they are not sufficient. Squid did not handle the resulting exceptions well enough, necessitating more out of scope changes (see branch commit becd5b4a that this change also reverts). Some of these problems were known in advance, and we hesitated moving the checks into `Uri.cc`. Now, given the number, the weight, and the scope of these drawbacks, we believe that we made the wrong decision. --- src/FwdState.cc | 25 --------------------- src/anyp/Uri.cc | 47 ++-------------------------------------- src/clients/FtpClient.cc | 12 ++++++---- 3 files changed, 10 insertions(+), 74 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index b2b3cfa9148..62c33a67f84 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -1435,31 +1435,6 @@ FwdState::pinnedCanRetry() const if (request->flags.sslBumped) return false; - // Currently, all retries call useDestinations() which never calls - // usePinned() and usually calls connectStart(). The latter requires nil - // request->pinnedConnection(). - if (request->pinnedConnection()) - return false; - - // XXX: The only case where the condition above is false despite true - // `request->flags.pinned` is when HttpRequest::clientConnectionManager job - // disappears. However, in that case, we should probably return `false` - // anyway to avoid retries with a gone client! - - // XXX: FwdState never re-pins a connection. In other words, if FwdState - // fails while using a pinned connection, we never pin a new connection to - // the same HttpRequest::clientConnectionManager job. 2013 commit 693cb033 - // removed most re-pinning code. 2019 commit 3dde9e52 probably assumed that - // "FTP proxying and connection-based HTTP authentication" cases mentioned - // below might re-pin, so it added this method to avoid breaking those - // alleged cases, but that assumption was probably wrong then and/or is - // wrong now. - - // TODO: Confirm that "connection-based HTTP authentication" does not need - // some other fix and remove this method. Also adjust peer selection code to - // avoid computing/returning regular destinations (that cannot be used) - // after selecting a PINNED connection. - // The other pinned cases are FTP proxying and connection-based HTTP // authentication. TODO: Do these cases have restrictions? return true; diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index a2b811099fe..8eae3429a63 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -69,18 +69,6 @@ PathChars() return pathValid; } -/// Throws when the given URL component contains a CR or LF character. -/// \param component is a URL part that needs to be checked -/// \prec component is not percent-encoded (e.g., it was decoded after extracting it from an HTTP request target) -static void -rejectFtpCommandDelimiters(const SBuf &component) -{ - static const auto delimiterChars = (CharacterSet::CR + CharacterSet::LF).rename("CRLF"); - - if (component.findFirstOf(delimiterChars) != SBuf::npos) - throw TextException("URL contains an FTP command delimiter character", Here()); -} - /** * Governed by RFC 3986 section 2.1 */ @@ -582,41 +570,10 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) } } - // Optimization: Convert legacy c-strings to SBuf once. - const auto loginCopy = SBuf(login); - const auto urlpathCopy = SBuf(urlpath); - - if (scheme == AnyP::PROTO_FTP) { - // Ftp::Gateway uses some FTP URL components to form FTP commands. - // FTP does not have a standard, consistently supported way of - // encoding CR and LF delimiters in command parameters, so we reject - // URIs containing those delimiters in those components. - - // These checks also apply to use cases where this Squid instance - // does not form an FTP command from the URL (e.g., we may forward - // the URL to another HTTP proxy instead). Even though doing so may - // break some "works without Squid" transactions, we cover those - // additional use cases to protect broken recipients because these - // delimiters affect FTP command framing. - - rejectFtpCommandDelimiters(loginCopy); - - if (const auto urlpathDecoded = Decode(urlpathCopy)) - rejectFtpCommandDelimiters(*urlpathDecoded); - else - throw TextException("invalid percent-encoding in an FTP URL path", Here()); - - // For CONNECTS, a parseHost() call above ensures that foundHost has - // no FTP command delimiters. For other methods, "whitespace is also - // a hostname delimiter" loop above ensures that invariant as well. - // Optimization: Do not create an SBuf just to check that invariant - // because the host component is not used in FTP command arguments. - } - setScheme(scheme); - path(urlpathCopy); + path(urlpath); host(foundHost); - userInfo(loginCopy); + userInfo(SBuf(login)); port(foundPort); return true; diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 0452336982d..9a0d57992de 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -829,10 +829,14 @@ Ftp::Client::writeCommand(const char *buf) Assure(bufLen > 2); Assure(buf[bufLen-2] == '\r'); Assure(buf[bufLen-1] == '\n'); - // If this assertion fails, then the caller has assembled an FTP command - // using input unchecked for embedded CRLF. For an example of such checks, - // see rejectFtpCommandDelimiters() calls in AnyP::Uri::parse(). - Assure(strcspn(buf, crlf) == bufLen-2); + + const auto crlfCharPosition = strcspn(buf, crlf); + if (crlfCharPosition != bufLen-2) { + const auto invalidCharName = buf[crlfCharPosition] == '\r' ? "CR" : "LF"; + debugs(9, 2, "ERROR: Caller assembled a malformed FTP command. Found " << invalidCharName << " at position " << crlfCharPosition); + failed(ERR_FTP_FAILURE, 0); + return; + } char *ebuf; /* trace FTP protocol communications at level 2 */ From 5c0a27f29402e840eb9b674d56c331c167dd06fb Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 17 Jun 2026 17:37:05 -0400 Subject: [PATCH 45/45] Revert "Reject FTP commands with parameters containing CR characters" This reverts branch commit 21581bdc49106f797333128f3e6c67b808d0b92b because official code can now survive the corresponding CR rejection in Ftp::Client::writeCommand() because that rejection code is no longer throwing. The reverted changes are valuable and should be merged separately. --- src/servers/FtpServer.cc | 45 +++++++--------------------------------- 1 file changed, 8 insertions(+), 37 deletions(-) diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index 1a851ad73cb..bb236a3bc6e 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -641,31 +641,15 @@ Ftp::Server::parseOneRequest() { flags.readMore = false; // common for all but one case below - // FTP command syntax is specified in RFC 959 Section 5.3. We generalize - // that grammar to parse all commands using the same code. We also relax the - // rules a little in hope to accommodate more real world use cases: - // * We allow ASCII HT, VT, and NP characters in various delimiters (in addition to SP). - // * We allow zero or more CR characters in command terminator (instead of exactly one). - // * We allow space characters before any command. - // - // command = BWS code [ RWS parameter ] OWS *CR LF - // code = 1*code_char - // parameter = 1*parameter_char ; without leading and trailing inner_space_chars - // code_char = ; any ASCII character other than space_char - // parameter_char = ; any ASCII character other than CR or LF - // BWS = 0*space_char ; optional "bad" space before the command code - // RWS = 1*inner_space_char ; required space before the command parameter - // OWS = 0*inner_space_char ; optional space after the command parameter - // inner_space_char = SP / HT / VT / NP ; space_char without CR and LF - // space_char = SP / HT / VT / NP / CR / LF; any isspace(3) character in "C" locale - - static const auto InlineSpaceChars = " \f\t\v"; + // OWS [ RWS ] OWS LF + + // InlineSpaceChars are isspace(3) or RFC 959 Section 3.1.1.5.2, except + // for the LF character that we must exclude here (but see FullWhiteSpace). + static const char * const InlineSpaceChars = " \f\r\t\v"; static const CharacterSet InlineSpace = CharacterSet("Ftp::Inline", InlineSpaceChars); - static const CharacterSet CrLfChars = (CharacterSet::CR + CharacterSet::LF).rename("CRLF"); - static const CharacterSet FullWhiteSpace = (InlineSpace + CrLfChars).rename("Ftp::FWS"); + static const CharacterSet FullWhiteSpace = (InlineSpace + CharacterSet::LF).rename("Ftp::FWS"); static const CharacterSet CommandChars = FullWhiteSpace.complement("Ftp::Command"); - // RFC 959 Section 5.3.2 excludes both CR and LF from and definitions - static const CharacterSet TailChars = CrLfChars.complement("Ftp::Tail"); + static const CharacterSet TailChars = CharacterSet::LF.complement("Ftp::Tail"); // This set is used to ignore empty commands without allowing an attacker // to keep us endlessly busy by feeding us whitespace or empty commands. @@ -679,7 +663,7 @@ Ftp::Server::parseOneRequest() (void)tok.skipAll(LeadingSpace); // leading OWS and empty commands const bool parsed = tok.prefix(cmd, CommandChars); // required command - // note that the condition below eats leading RWS and trailing OWS, if any + // note that the condition below will eat either RWS or trailing OWS if (parsed && tok.skipAll(InlineSpace) && tok.prefix(params, TailChars)) { // now params may include trailing OWS // TODO: Support right-trimming using CharacterSet in Tokenizer instead @@ -698,21 +682,8 @@ Ftp::Server::parseOneRequest() return earlyError(EarlyErrorKind::HugeRequest); } - if (parsed) - (void)tok.skipAll(CharacterSet::CR); - // technically, we may skip multiple NLs below, but that is OK if (!parsed || !tok.skipAll(CharacterSet::LF)) { // did not find terminating LF yet - - if (!tok.remaining().isEmpty()) { - // We always consume all valid input, so any leftovers imply that we - // found something that we cannot parse now and will never parse if - // more input becomes available later (e.g., `PWD\rQUIT\n`). - changeState(fssError, "bad FTP command syntax"); - quitAfterError(nullptr); - return earlyError(EarlyErrorKind::MalformedCommand); - } - // we need more data, but can we buffer more? if (inBuf.length() >= Config.maxRequestHeaderSize) { changeState(fssError, "huge req");