From 7a863dad79d0ab589eb2c81cac88bfb2aeee01cb Mon Sep 17 00:00:00 2001 From: mmenke Date: Fri, 4 Mar 2016 07:53:20 -0800 Subject: Remove use of UnescapeRule::URL_SPECIAL_CHARS from extensions/file_util. We're removing this, in favor of PATH_SEPARATORS and URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS, to avoid any security issues with this, going forward. Both of the uses of URL_SPECIAL_CHARS in file_util.cc look problematic, so modifying them to no longer escape paragraph separators, and adding a pair of tests. BUG=589257 Review URL: https://codereview.chromium.org/1760073003 Cr-Commit-Position: refs/heads/master@{#379291} --- extensions/common/file_util.cc | 12 ++++-- extensions/common/file_util_unittest.cc | 70 +++++++++++++++------------------ 2 files changed, 39 insertions(+), 43 deletions(-) (limited to 'extensions/common') diff --git a/extensions/common/file_util.cc b/extensions/common/file_util.cc index ae12f2c..830cc48 100644 --- a/extensions/common/file_util.cc +++ b/extensions/common/file_util.cc @@ -441,8 +441,10 @@ base::FilePath ExtensionURLToRelativeFilePath(const GURL& url) { return base::FilePath(); // Drop the leading slashes and convert %-encoded UTF8 to regular UTF8. - std::string file_path = net::UnescapeURLComponent(url_path, - net::UnescapeRule::SPACES | net::UnescapeRule::URL_SPECIAL_CHARS); + std::string file_path = net::UnescapeURLComponent( + url_path, + net::UnescapeRule::SPACES | + net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS); size_t skip = file_path.find_first_not_of("/\\"); if (skip != file_path.npos) file_path = file_path.substr(skip); @@ -460,8 +462,10 @@ base::FilePath ExtensionURLToRelativeFilePath(const GURL& url) { base::FilePath ExtensionResourceURLToFilePath(const GURL& url, const base::FilePath& root) { - std::string host = net::UnescapeURLComponent(url.host(), - net::UnescapeRule::SPACES | net::UnescapeRule::URL_SPECIAL_CHARS); + std::string host = net::UnescapeURLComponent( + url.host(), + net::UnescapeRule::SPACES | + net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS); if (host.empty()) return base::FilePath(); diff --git a/extensions/common/file_util_unittest.cc b/extensions/common/file_util_unittest.cc index c058400..e63856c 100644 --- a/extensions/common/file_util_unittest.cc +++ b/extensions/common/file_util_unittest.cc @@ -454,26 +454,20 @@ TEST_F(FileUtilTest, ExtensionURLToRelativeFilePath) { const char* url; const char* expected_relative_path; } test_cases[] = { - { URL_PREFIX "simple.html", - "simple.html" }, - { URL_PREFIX "directory/to/file.html", - "directory/to/file.html" }, - { URL_PREFIX "escape%20spaces.html", - "escape spaces.html" }, - { URL_PREFIX "%C3%9Cber.html", - "\xC3\x9C" "ber.html" }, + {URL_PREFIX "simple.html", "simple.html"}, + {URL_PREFIX "directory/to/file.html", "directory/to/file.html"}, + {URL_PREFIX "escape%20spaces.html", "escape spaces.html"}, + {URL_PREFIX "%C3%9Cber.html", + "\xC3\x9C" + "ber.html"}, #if defined(OS_WIN) - { URL_PREFIX "C%3A/simple.html", - "" }, + {URL_PREFIX "C%3A/simple.html", ""}, #endif - { URL_PREFIX "////simple.html", - "simple.html" }, - { URL_PREFIX "/simple.html", - "simple.html" }, - { URL_PREFIX "\\simple.html", - "simple.html" }, - { URL_PREFIX "\\\\foo\\simple.html", - "foo/simple.html" }, + {URL_PREFIX "////simple.html", "simple.html"}, + {URL_PREFIX "/simple.html", "simple.html"}, + {URL_PREFIX "\\simple.html", "simple.html"}, + {URL_PREFIX "\\\\foo\\simple.html", "foo/simple.html"}, + {URL_PREFIX "..%2f..%2fsimple.html", "..%2f..%2fsimple.html"}, }; #undef URL_PREFIX @@ -506,6 +500,10 @@ TEST_F(FileUtilTest, ExtensionResourceURLToFilePath) { ASSERT_TRUE(base::WriteFile(resource_path, data, sizeof(data))); resource_path = api_path.Append(FILE_PATH_LITERAL("escape spaces.js")); ASSERT_TRUE(base::WriteFile(resource_path, data, sizeof(data))); + resource_path = api_path.Append(FILE_PATH_LITERAL("escape spaces.js")); + ASSERT_TRUE(base::WriteFile(resource_path, data, sizeof(data))); + resource_path = api_path.Append(FILE_PATH_LITERAL("..%2f..%2fsimple.html")); + ASSERT_TRUE(base::WriteFile(resource_path, data, sizeof(data))); #ifdef FILE_PATH_USES_WIN_SEPARATORS #define SEP "\\" @@ -517,27 +515,21 @@ TEST_F(FileUtilTest, ExtensionResourceURLToFilePath) { const char* url; const base::FilePath::CharType* expected_path; } test_cases[] = { - { URL_PREFIX "apiname/test.js", - FILE_PATH_LITERAL("test.js") }, - { URL_PREFIX "/apiname/test.js", - FILE_PATH_LITERAL("test.js") }, - // Test % escape - { URL_PREFIX "apiname/%74%65st.js", - FILE_PATH_LITERAL("test.js") }, - { URL_PREFIX "apiname/escape%20spaces.js", - FILE_PATH_LITERAL("escape spaces.js") }, - // Test file does not exist. - { URL_PREFIX "apiname/directory/to/file.js", - NULL }, - // Test apiname/../../test.js - { URL_PREFIX "apiname/../../test.js", - FILE_PATH_LITERAL("test.js") }, - { URL_PREFIX "apiname/..%2F../test.js", - NULL }, - { URL_PREFIX "apiname/f/../../../test.js", - FILE_PATH_LITERAL("test.js") }, - { URL_PREFIX "apiname/f%2F..%2F..%2F../test.js", - NULL }, + {URL_PREFIX "apiname/test.js", FILE_PATH_LITERAL("test.js")}, + {URL_PREFIX "/apiname/test.js", FILE_PATH_LITERAL("test.js")}, + // Test % escape + {URL_PREFIX "apiname/%74%65st.js", FILE_PATH_LITERAL("test.js")}, + {URL_PREFIX "apiname/escape%20spaces.js", + FILE_PATH_LITERAL("escape spaces.js")}, + // Test file does not exist. + {URL_PREFIX "apiname/directory/to/file.js", NULL}, + // Test apiname/../../test.js + {URL_PREFIX "apiname/../../test.js", FILE_PATH_LITERAL("test.js")}, + {URL_PREFIX "apiname/..%2F../test.js", NULL}, + {URL_PREFIX "apiname/f/../../../test.js", FILE_PATH_LITERAL("test.js")}, + {URL_PREFIX "apiname/f%2F..%2F..%2F../test.js", NULL}, + {URL_PREFIX "apiname/..%2f..%2fsimple.html", + FILE_PATH_LITERAL("..%2f..%2fsimple.html")}, }; #undef SEP #undef URL_PREFIX -- cgit v1.1