-
Notifications
You must be signed in to change notification settings - Fork 640
Do not send FTP commands with embedded CRs or LFs #2444
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.
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
Changes from all commits
d4dc3ad
5028f33
03d61d7
5afbe4b
0f614e2
b0eef7d
e5b8579
1af928d
2c46042
61e5948
7ee396e
5b6e34d
cc8aae5
645e8fc
699b944
7172526
773b02c
9759307
bf23c6d
0335fd3
75000be
97711b5
53afbfc
5af247a
11ab7c6
af94fd2
0881b5d
89cbe35
02a3453
9474c89
29d0f8c
5b290a8
d759856
9f48a4f
f056109
652e948
01608f5
60f573d
4e9a4b7
516d1f0
dde3c53
becd5b4
21581bd
56fdddd
5c0a27f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -823,6 +823,21 @@ Ftp::Client::dataClosed(const CommCloseCbParams &) | |
| void | ||
| Ftp::Client::writeCommand(const char *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'); | ||
|
|
||
| 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, we should throw here. We tried, but other/out-of-scope problems in current code forced us to table that idea (for now). FWIW, I am uncomfortable with the proposed |
||
| return; | ||
| } | ||
|
|
||
| char *ebuf; | ||
| /* trace FTP protocol communications at level 2 */ | ||
| debugs(9, 2, "ftp<< " << buf); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the check being added is not validating "non-empty", it is validating length. The string may still be whitespace characters, which make non-zero length empty.
(I am not certain this is more than a theoretical case, thus the non-blocker review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This source code comment uses the words "non-empty" to mean "has at least one character". IMO, that is a reasonable usage. "Has at least one non-space character" could also be a reasonable definition in this context, but that is not the definition being used here. The actual assertion code (quoted above) clearly distinguishes the two cases, so I do not think there is an ambiguity problem here.
A command consisting entirely of space characters would be invalid and should not be generated, but our new assertions do not validate that invariant (because it is unrelated to this PR scope and correctly detecting excessive spaces in the assembled command may not be trivial and could be controversial). I do not recommend adding such an assertion in this PR.
Removing "non-empty" from this C++ comment would make this code worse because many readers would suspect an off-by one error without the comment -- the assertions further below only require a
bufLen >= 2condition. Using that weaker condition instead is not an improvement either -- Squid should not generate a\r\ncommand, of course.If you can suggest a better wording for this comment, please do so.