Skip to content

Commit

Permalink
Switch FTP code to using UnescapeBinaryURLComponent().
Browse files Browse the repository at this point in the history 
It was using UnescapeURLComponent() to unescape some characters, while
leaving others escaped, which doesn't really make sense in an FTP
context. Instead, we now unescape all characters, and fail requests in the
case of unsafe characters, instead of leaving just those characters
escaped. Also, we no longer apply unescaping logic on the paths
returned to us from the server in response to the PWD command, as that
seemed unexpected.

This CL is part of a project to cleaning up uses of
UnescapeURLComponent(), as it's been used for wildly disparate
purposes, and much of its logic is only suitable for cases where
we want to format a URL for safe display to the user. We want
to make it leave more weird characters escaped, but need to
eliminate usage of it in other contexts before we can do that.

UnescapeBinaryURLComponent() unconditionally unescapes all escaped
characters, and should be used instead in most cases, particulars where
URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS is in use. The primary cases
where it shouldn't be used (without additional checks), are cases where
unescaping path separators or funky characters (like %01) is a problem.
In those places, we should generally fail, instead of partially
unescaping a URL.

Bug: 959280
Change-Id: Ief2dccd3f0a7e3cfb753586c51837f3596857fa9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1594150
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#656597}
  • Loading branch information
Matt Menke authored and Commit Bot committed May 3, 2019
1 parent 9731d02 commit c7984c0
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 89 deletions.
42 changes: 26 additions & 16 deletions net/ftp/ftp_network_transaction.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,31 @@ int FtpNetworkTransaction::Start(

DetectTypecode();

if (request_->url.has_path()) {
std::string gurl_path(request_->url.path());

// Get rid of the typecode, see RFC 1738 section 3.2.2. FTP url-path.
std::string::size_type pos = gurl_path.rfind(';');
if (pos != std::string::npos)
gurl_path.resize(pos);

// If the path contains characters not considered safe to unescape, fail the
// request.
std::set<unsigned char> illegal_encoded_bytes{'/', '\\'};
// Null shouldn't be needed, since GURLs with nulls in the path aren't
// considered valid, but can't hurt. Note that this range includes CRs and
// LFs.
for (char c = '\x00'; c < '\x20'; ++c) {
illegal_encoded_bytes.insert(c);
}
if (ContainsEncodedBytes(gurl_path, illegal_encoded_bytes))
return ERR_INVALID_URL;

// This may unescape to non-ASCII characters, but we allow that. See the
// comment for IsValidFTPCommandSubstring.
unescaped_path_ = UnescapeBinaryURLComponent(gurl_path);
}

next_state_ = STATE_CTRL_RESOLVE_HOST;
int rv = DoLoop(OK);
if (rv == ERR_IO_PENDING)
Expand Down Expand Up @@ -498,27 +523,12 @@ int FtpNetworkTransaction::SendFtpCommand(const std::string& command,

std::string FtpNetworkTransaction::GetRequestPathForFtpCommand(
bool is_directory) const {
std::string path(current_remote_directory_);
if (request_->url.has_path()) {
std::string gurl_path(request_->url.path());

// Get rid of the typecode, see RFC 1738 section 3.2.2. FTP url-path.
std::string::size_type pos = gurl_path.rfind(';');
if (pos != std::string::npos)
gurl_path.resize(pos);
std::string path(current_remote_directory_ + unescaped_path_);

path.append(gurl_path);
}
// Make sure that if the path is expected to be a file, it won't end
// with a trailing slash.
if (!is_directory && path.length() > 1 && path.back() == '/')
path.erase(path.length() - 1);
UnescapeRule::Type unescape_rules =
UnescapeRule::SPACES |
UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS;
// This may unescape to non-ASCII characters, but we allow that. See the
// comment for IsValidFTPCommandSubstring.
path = UnescapeURLComponent(path, unescape_rules);

if (system_type_ == SYSTEM_TYPE_VMS) {
if (is_directory)
Expand Down
2 changes: 2 additions & 0 deletions net/ftp/ftp_network_transaction.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,8 @@ class NET_EXPORT_PRIVATE FtpNetworkTransaction : public FtpTransaction {

ClientSocketFactory* const socket_factory_;

std::string unescaped_path_;

std::unique_ptr<StreamSocket> ctrl_socket_;
std::unique_ptr<StreamSocket> data_socket_;

Expand Down
129 changes: 56 additions & 73 deletions net/ftp/ftp_network_transaction_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
#include "net/base/io_buffer.h"
#include "net/base/ip_endpoint.h"
Expand Down Expand Up @@ -399,49 +400,26 @@ class FtpSocketDataProviderFileDownload : public FtpSocketDataProvider {
return MockWriteResult(ASYNC, data.length());
switch (state()) {
case PRE_SIZE:
return Verify("SIZE /file\r\n", data, PRE_CWD, "213 18\r\n");
return Verify(base::StringPrintf("SIZE %s\r\n", file_path_.c_str()),
data, PRE_CWD, "213 18\r\n");
case PRE_CWD:
return Verify("CWD /file\r\n", data,
use_epsv() ? PRE_RETR_EPSV : PRE_RETR_PASV,
return Verify(base::StringPrintf("CWD %s\r\n", file_path_.c_str()),
data, use_epsv() ? PRE_RETR_EPSV : PRE_RETR_PASV,
"550 Not a directory\r\n");
case PRE_RETR:
return Verify("RETR /file\r\n", data, PRE_QUIT, "200 OK\r\n");
return Verify(base::StringPrintf("RETR %s\r\n", file_path_.c_str()),
data, PRE_QUIT, "200 OK\r\n");
default:
return FtpSocketDataProvider::OnWrite(data);
}
}

private:
DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderFileDownload);
};

class FtpSocketDataProviderPathSeparatorsNotUnescaped
: public FtpSocketDataProvider {
public:
FtpSocketDataProviderPathSeparatorsNotUnescaped() = default;
~FtpSocketDataProviderPathSeparatorsNotUnescaped() override = default;

MockWriteResult OnWrite(const std::string& data) override {
if (InjectFault())
return MockWriteResult(ASYNC, data.length());
switch (state()) {
case PRE_SIZE:
return Verify("SIZE /foo%2f..%2fbar%5c\r\n", data, PRE_CWD,
"213 18\r\n");
case PRE_CWD:
return Verify("CWD /foo%2f..%2fbar%5c\r\n", data,
use_epsv() ? PRE_RETR_EPSV : PRE_RETR_PASV,
"550 Not a directory\r\n");
case PRE_RETR:
return Verify("RETR /foo%2f..%2fbar%5c\r\n", data, PRE_QUIT,
"200 OK\r\n");
default:
return FtpSocketDataProvider::OnWrite(data);
}
}
void set_file_path(const std::string& file_path) { file_path_ = file_path; }

private:
DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderPathSeparatorsNotUnescaped);
std::string file_path_ = "/file";

DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderFileDownload);
};

class FtpSocketDataProviderFileNotFound : public FtpSocketDataProvider {
Expand Down Expand Up @@ -582,34 +560,6 @@ class FtpSocketDataProviderVMSFileDownload : public FtpSocketDataProvider {
DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderVMSFileDownload);
};

class FtpSocketDataProviderEscaping : public FtpSocketDataProviderFileDownload {
public:
FtpSocketDataProviderEscaping() = default;
~FtpSocketDataProviderEscaping() override = default;

MockWriteResult OnWrite(const std::string& data) override {
if (InjectFault())
return MockWriteResult(ASYNC, data.length());
switch (state()) {
case PRE_SIZE:
return Verify("SIZE / !\"#$%y\200\201\r\n", data, PRE_CWD,
"213 18\r\n");
case PRE_CWD:
return Verify("CWD / !\"#$%y\200\201\r\n", data,
use_epsv() ? PRE_RETR_EPSV : PRE_RETR_PASV,
"550 Not a directory\r\n");
case PRE_RETR:
return Verify("RETR / !\"#$%y\200\201\r\n", data, PRE_QUIT,
"200 OK\r\n");
default:
return FtpSocketDataProviderFileDownload::OnWrite(data);
}
}

private:
DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderEscaping);
};

class FtpSocketDataProviderFileDownloadInvalidResponse
: public FtpSocketDataProviderFileDownload {
public:
Expand Down Expand Up @@ -1322,16 +1272,31 @@ TEST_P(FtpNetworkTransactionTest, DownloadTransactionSpaceInPassword) {
ExecuteTransaction(&ctrl_socket, "ftp://test:hello%20world@host/file", OK);
}

// Make sure FtpNetworkTransaction doesn't request paths like
// "/foo/../bar". Doing so wouldn't be a security issue, client side, but just
// doesn't seem like a good idea.
TEST_P(FtpNetworkTransactionTest,
DownloadTransactionPathSeparatorsNotUnescaped) {
FtpSocketDataProviderPathSeparatorsNotUnescaped ctrl_socket;
ExecuteTransaction(&ctrl_socket, "ftp://host/foo%2f..%2fbar%5c", OK);
TEST_P(FtpNetworkTransactionTest, FailOnInvalidUrls) {
const char* const kBadUrls[]{
// Make sure FtpNetworkTransaction doesn't request paths like
// "/foo/../bar". Doing so wouldn't be a security issue, client side, but
// just doesn't seem like a good idea.
"ftp://host/foo%2f..%2fbar%5c",

// We pass an artificial value of 18 as a response to the SIZE command.
EXPECT_EQ(18, transaction_->GetResponseInfo()->expected_content_size);
// LF
"ftp://host/foo%10.txt",
// CR
"ftp://host/foo%13.txt",

"ftp://host/foo%00.txt",
};

for (const char* bad_url : kBadUrls) {
SCOPED_TRACE(bad_url);

SetUpTransaction();
FtpRequestInfo request_info = GetRequestInfo(bad_url);
ASSERT_EQ(
ERR_INVALID_URL,
transaction_->Start(&request_info, callback_.callback(),
NetLogWithSource(), TRAFFIC_ANNOTATION_FOR_TESTS));
}
}

TEST_P(FtpNetworkTransactionTest, EvilRestartUser) {
Expand Down Expand Up @@ -1403,9 +1368,27 @@ TEST_P(FtpNetworkTransactionTest, EvilRestartPassword) {
}

TEST_P(FtpNetworkTransactionTest, Escaping) {
FtpSocketDataProviderEscaping ctrl_socket;
ExecuteTransaction(&ctrl_socket, "ftp://host/%20%21%22%23%24%25%79%80%81",
OK);
const struct TestCase {
const char* url;
const char* expected_path;
} kTestCases[] = {
{"ftp://host/%20%21%22%23%24%25%79%80%81", "/ !\"#$%y\200\201"},
// This is no allowed to be unescaped by UnescapeURLComponent, since it's
// a lock icon. But it has no special meaning or security concern in the
// context of making FTP requests.
{"ftp://host/%F0%9F%94%92", "/\xF0\x9F\x94\x92"},
// Invalid UTF-8 character, which again has no special meaning over FTP.
{"ftp://host/%81", "/\x81"},
};

for (const auto& test_case : kTestCases) {
SCOPED_TRACE(test_case.url);

SetUpTransaction();
FtpSocketDataProviderFileDownload ctrl_socket;
ctrl_socket.set_file_path(test_case.expected_path);
ExecuteTransaction(&ctrl_socket, test_case.url, OK);
}
}

// Test for http://crbug.com/23794.
Expand Down

0 comments on commit c7984c0

Please sign in to comment.