Skip to content

Reject FTP commands with parameters containing CR characters #2453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
somecookie wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Reject FTP commands with parameters containing CR characters #2453

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 37 additions & 8 deletions src/servers/FtpServer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,15 +641,31 @@ Ftp::Server::parseOneRequest()
{
flags.readMore = false; // common for all but one case below

// OWS <command> [ RWS <parameter> ] 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
Comment thread
rousskov marked this conversation as resolved.
// 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";

@somecookie somecookie Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we align the names with the definition to make the code easier to read, e.g.,

Current Grammar token Suggested name
InlineSpace inner_space_char InnerSpaceChars
FullWhiteSpace space_char SpaceChars
CommandChars code_char CodeChars
TailChars parameter_char ParameterChars
LeadingSpace BWS BadWhiteSpace

@somecookie somecookie Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided not to go for this to keep the diff minimal which makes it clearer what is changed in the parsing logic

@rousskov rousskov Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed the mismatch. It should be fixed. The only question is when: In this PR or in a dedicated polishing followup.

When sketching this code, I preserved old names and old naming patterns because I did not want to change otherwise unchanged official code lines. Emphasizing what we are changing felt more important to me. It still does. However, if others explicitly request that we polish names in this PR (at the expense of diff clarity), I will not object.

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 <char> and <pr-char> 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.
Expand All @@ -663,7 +679,7 @@ Ftp::Server::parseOneRequest()
(void)tok.skipAll(LeadingSpace); // leading OWS and empty commands

@somecookie somecookie Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(void)tok.skipAll(LeadingSpace); // leading OWS and empty commands
(void)tok.skipAll(LeadingSpace); // leading BWS

@somecookie somecookie Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for variable names above

@rousskov rousskov Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep this otherwise unchanged line intact because the old comment is not wrong -- old "OWS and empty commands" description matches LeadingSpace definition in PR code. If others want to polish this comment, here is a better variation (BWS includes "empty commands"):

Suggested change
(void)tok.skipAll(LeadingSpace); // leading OWS and empty commands
(void)tok.skipAll(LeadingSpace); // leading BWS

or, if you prefer, just

Suggested change
(void)tok.skipAll(LeadingSpace); // leading OWS and empty commands
(void)tok.skipAll(LeadingSpace); // BWS

Again, I would not commit this comment change unless explicitly requested by others.

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
Expand All @@ -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");
Expand Down
Loading