bpo-30119: Fix ftplib to handle the ftp user's commands.

[corona10]

bpo-30119: Handle ftp user command.

See https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2017-3533
and http://blog.blindspotsecurity.com/2017/02/advisory-javapython-ftp-injections.html

https://bugs.python.org/issue30119

[mention-bot]

@corona10, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tim-one, @freddrake and @giampaolo to be potential reviewers.

[corona10]

@giampaolo If you have times, Can you take a look?

[giampaolo]

I'm not sure I fully understand the nature of this security issue but at a first look it seems it's the server which is more subject to risks, not the client (but I may be wrong). Also, http://blog.blindspotsecurity.com/2017/02/advisory-javapython-ftp-injections.html suggests this is more about urllib/2 rather than ftplib.

In your patch you are disallowing any command with "\r" or "\n" in it, such as sendcmd("mkd foo\nbar"). To my knowledge RFC-959 does not describe how to escape CR/LF characters but I don't recall them being explicitly disallowed.

Also and I don't see why urllib.parse.unquote() should be used, since a FTP client is supposed to deal with paths, not URLs.

[corona10]

@giampaolo
First of all, Thank you for your response.
I agree most of you comments.
Especially, but at a first look it seems it's the server which is more subject to risks.
However, One of our roles is to prevent to use Python for malicious attacks.
By rfc959, There is no reason to command includes CRLF
because it denotes for the end of command.
I think that it worth to prevent it from inserting CRLF in the command.

The ftp client will automatically append CRLF to the end of the command. One option is to allow CRLF at the end of the command.

            USER <SP> <username> <CRLF>
            PASS <SP> <password> <CRLF>
            ACCT <SP> <account-information> <CRLF>
            CWD  <SP> <pathname> <CRLF>
            CDUP <CRLF>
            SMNT <SP> <pathname> <CRLF>
            QUIT <CRLF>
            REIN <CRLF>
            PORT <SP> <host-port> <CRLF>
            PASV <CRLF>
            TYPE <SP> <type-code> <CRLF>
            STRU <SP> <structure-code> <CRLF>
            MODE <SP> <mode-code> <CRLF>
            RETR <SP> <pathname> <CRLF>
            STOR <SP> <pathname> <CRLF>
            STOU <CRLF>
            APPE <SP> <pathname> <CRLF>
            ALLO <SP> <decimal-integer>
                [<SP> R <SP> <decimal-integer>] <CRLF>
            REST <SP> <marker> <CRLF>
            RNFR <SP> <pathname> <CRLF>
            RNTO <SP> <pathname> <CRLF>
            ABOR <CRLF>
            DELE <SP> <pathname> <CRLF>
            RMD  <SP> <pathname> <CRLF>
            MKD  <SP> <pathname> <CRLF>
            PWD  <CRLF>
            LIST [<SP> <pathname>] <CRLF>
            NLST [<SP> <pathname>] <CRLF>
            SITE <SP> <string> <CRLF>
            SYST <CRLF>
            STAT [<SP> <pathname>] <CRLF>
            HELP [<SP> <string>] <CRLF>
            NOOP <CRLF

[giampaolo]

Yes, I agree it's probably a good idea to disallow a command containing CRLF. I would not introduce a new exception for that though (not worth it). You can add that check in putline method and raise ValueError.

[corona10]

@giampaolo
Thank you for your quick response.
For summary,

• The FTP client will only allow commands which contain CRLF only at the end of the command.
• When user input commands with CRLF than raise ValueError.

[giampaolo]

Right. Also, I'm not sure this change solves the vulnerability issue about the CRLF injection. I expect urllib/2 needs to be fixed for that (a PR is welcome :P). As such Misc/NEWS should be changed in accordance.

[corona10]

@giampaolo Yes, I submitted the patch for that issue also.
See #1216

[corona10]

@giampaolo
FYI, OpenJDK handling this issue in this way.
WDYT?

[corona10]

@giampaolo

      5.3.2.  FTP COMMAND ARGUMENTS

         The syntax of the above argument fields (using BNF notation
         where applicable) is:

            <username> ::= <string>
            <password> ::= <string>
            <account-information> ::= <string>
            <string> ::= <char> | <char><string>
            <char> ::= any of the 128 ASCII characters except <CR> and
            <LF>
            <marker> ::= <pr-string>
            <pr-string> ::= <pr-char> | <pr-char><pr-string>
            <pr-char> ::= printable characters, any
                          ASCII code 33 through 126
            <byte-size> ::= <number>
            <host-port> ::= <host-number>,<port-number>
            <host-number> ::= <number>,<number>,<number>,<number>
            <port-number> ::= <number>,<number>
            <number> ::= any decimal integer 1 through 255
            <form-code> ::= N | T | C
            <type-code> ::= A [<sp> <form-code>]
                          | E [<sp> <form-code>]
                          | I
                          | L <sp> <byte-size>
            <structure-code> ::= F | R | P
            <mode-code> ::= S | B | C
            <pathname> ::= <string>
            <decimal-integer> ::= any decimal integer

By

<string> ::= <char> | <char><string>           
<char> ::= any of the 128 ASCII characters except <CR> and <LF>
<pathname> ::= <string>

I think that we can deny all of the commands which include CR or LF.

[corona10]

@giampaolo Can I proceed this PR?

[giampaolo]

Whereas I agree doing this in urllib is a good idea as I see you've done in #1216 I'm not so sure about ftplib. From a security perspective I don't I understand which are the risks of allowing "\r" and "\n".
The "magic marker" is "\r\n", not those, and any decent FTP server parser should not fall for "\r" and "\n" being present in the line. What I'm missing is a use-case scenario about how "\r" and "\n" could be exploited in order to create a security concern.

[corona10]

@giampaolo I wrote about more discuss on bpo-30119

[vadmium]

IMO an FTP server (or firewall parsing the FTP connection) is likely to recognize a lone LF (\n) as a command-line delimiter. The first FTP server I tried does so:

>>> s = create_connection(("ftp.debian.org", "ftp"))
>>> s.recv(100)
b'220 ftp.debian.org FTP server\r\n'
>>> s.sendall(b"USER %b\r\n" % b"anonymous\nQUIT")  # Injection via LF only
>>> s.recv(100)  # Responses to both commands
b'331 Please specify the password.\r\n221 Goodbye.\r\n'

Assuming CR is supposed to be illegal in an FTP command line, the client shouldn’t be sending CR either. This may be less important because servers are less likely to interpret it as the end of a command, but it still seems plausible. Python’s own universal newline decoders accept CR as a newline.

[ecbftw]

Hi Everyone,

I'm the guy that worked out how to exploit these bugs with firewalls. I just figured I would chime in and provide some opinions.

For one, any URL parsing code that is not FTP-specific should not have input validation checks added. After all, other protocols might be able to handle CR/LF/NUL just fine through an appropriate encoding mechanism. I don't think you guys were pushing for URL library management of this problem, but I just want to emphasize that.

I think any FTP command should be checked for both CR and LF. If either exist, an exception should be thrown, as you appear to be doing. This check should be performed just before sending it on the TCP connection. It is much safer to do this rather than trying to do something hacky like replace them with a space character. (Cleaning invalid input and continuing to use it is often a recipe for disaster in security.) I don't see a good reason to allow CR/LF at the end of the command... I mean, why? Just make the caller strip any string. If FTP happened to have some way to properly encode delimiter characters like this, then that would be the right way to fix it, but there isn't a standard way, so throwing exceptions is your best bet.

NUL (\0) bytes should also be viewed with skepticism. For instance, if the FTP command is a CWD, then a NUL should be rejected since those bytes aren't allowed in any filesystem and they have implications for string manipulation in C. (As an example, it used to be very easy to trick PHP and ASP applications into truncating filenames by inserting a '\0', which had significant security impacts.) If the character has no legitimate use, then reject it.

If the FTP implementation is designed as a low-level library for use by python programmers without support for URL fetching, should you bother with this stuff? Yes, I think so. The risk might currently be very low, but later someone could wrap up your library with something that accepts URLs and runs some commands. Their library may not be able to deal with these special characters, since it may be too abstract at that layer. So if a command isn't syntactically legal, then blow up at the low level.

HTH,
tim

[giampaolo]

Again, this was created as a security concern but I fail to see an exhaustive description of how exactly this should have security implications or some sort of CVE about it. I re-read your blog post once again but I fail to understand the implications. The only thing I grasped is that some FTP servers erroneously treat "\n" in the middle of the string as "\r\n" so 2 commands can be sent in one packet, then... what? How can this be exploited? It's the server which should be fixed, not the client.

Security concerns aside, there is the argument of rejecting <CR> / <LF> because they're just wrong. RFC-959 disallows them, but RFC-959 is 30 years old and mandates ASCII only, which is clearly no longer the case nowadays:

            <username> ::= <string>
            <password> ::= <string>
            <account-information> ::= <string>
            <string> ::= <char> | <char><string>
            <char> ::= any of the 128 ASCII characters except <CR> and
            <LF>
            <marker>

RFC-2640 describes how to escape them though:

   When a <CR> character is encountered as part of a pathname it MUST be
   padded with a <NUL> character prior to sending the command. On
   receipt of a pathname containing a <CR><NUL> sequence the <NUL>
   character MUST be stripped away.

Now, ftplib does not do this (escaping), meaning it's not fully RFC-2640 compliant although it does encode strings. The thing I'm worried about is breaking apps of those users who for some reason use <CR> / <LF> in their paths or user/password pairs, erroneously or not, because I expect few FTP servers do the null byte replacement and right now ftplib is able to interact with those uncompliant servers.

@ecbftw you state you don't want to disclose the exploit script so maybe you can send it to me privately?

[ecbftw]

Thank you @giampaolo. And thank you @corona10 for the initial patch.

[giampaolo]

This needs backports for Python 2.7, 3.3, 3.4, 3.5 and 3.6. @corona10 would you be up to it?
https://docs.python.org/devguide/committing.html?highlight=backport#backporting-changes-to-an-older-version

[bedevere-bot]

GH-2886 is a backport of this pull request to the 3.6 branch.

[bedevere-bot]

GH-2887 is a backport of this pull request to the 3.5 branch.

[bedevere-bot]

GH-2894 is a backport of this pull request to the 2.7 branch.