From 4c400d5d18ce473204931b7c1d7f85c9fb55ce4a Mon Sep 17 00:00:00 2001 From: soroosh-sdi Date: Fri, 24 Sep 2021 13:33:10 +0330 Subject: [PATCH] Remove whitelisting remapping targets Signed-off-by: soroosh-sdi --- Changelog.md | 1 + docs/path-resolution.rst | 5 -- docs/using-the-compiler.rst | 3 +- solc/CommandLineParser.cpp | 12 ---- test/solc/CommandLineInterface.cpp | 2 - test/solc/CommandLineInterfaceAllowPaths.cpp | 68 ++++++++++---------- test/solc/CommandLineParser.cpp | 4 +- 7 files changed, 38 insertions(+), 57 deletions(-) diff --git a/Changelog.md b/Changelog.md index c77facc1f..fb3d8a4ee 100644 --- a/Changelog.md +++ b/Changelog.md @@ -4,6 +4,7 @@ Breaking changes: * Disallow ``.pop()`` on arrays containing nested mappings. * Disallow ``delete`` on types that contain nested mappings. * Inline Assembly: Consider functions, function parameters and return variables for shadowing checks. + * Commandline Interface: Remapping targets are not automatically added to allowed paths. ### 0.8.10 (unreleased) diff --git a/docs/path-resolution.rst b/docs/path-resolution.rst index 40a727068..5297a887d 100644 --- a/docs/path-resolution.rst +++ b/docs/path-resolution.rst @@ -430,9 +430,6 @@ locations that are considered safe by default: - Outside of Standard JSON mode: - The directories containing input files listed on the command line. - - The directories used as :ref:`remapping ` targets. - If the target is not a directory (i.e does not end with ``/``, ``/.`` or ``/..``) the directory - containing the target is used instead. - Base path and include paths. - In Standard JSON mode: @@ -446,7 +443,6 @@ The option accepts a comma-separated list of paths: cd /home/user/project/ solc token/contract.sol \ - lib/util.sol=libs/util.sol \ --base-path=token/ \ --include-path=/lib/ \ --allow-paths=../utils/,/tmp/libraries @@ -457,7 +453,6 @@ importing files from the following directories: - ``/home/user/project/token/`` (because ``token/`` contains the input file and also because it is the base path), - ``/lib/`` (because ``/lib/`` is one of the include paths), -- ``/home/user/project/libs/`` (because ``libs/`` is a directory containing a remapping target), - ``/home/user/utils/`` (because of ``../utils/`` passed to ``--allow-paths``), - ``/tmp/libraries/`` (because of ``/tmp/libraries`` passed to ``--allow-paths``), diff --git a/docs/using-the-compiler.rst b/docs/using-the-compiler.rst index 88da5d5e6..eb79d0f28 100644 --- a/docs/using-the-compiler.rst +++ b/docs/using-the-compiler.rst @@ -54,8 +54,7 @@ or ../ ` are treated as relative to the directories specified us Furthermore, the part of the path added via these options will not appear in the contract metadata. For security reasons the compiler has :ref:`restrictions on what directories it can access `. -Directories of source files specified on the command line and target paths of -remappings are automatically allowed to be accessed by the file reader, but everything +Directories of source files specified on the command line are automatically allowed to be accessed by the file reader, but everything else is rejected by default. Additional paths (and their subdirectories) can be allowed via the ``--allow-paths /sample/path,/another/sample/path`` switch. diff --git a/solc/CommandLineParser.cpp b/solc/CommandLineParser.cpp index 89304ef3c..dddf87226 100644 --- a/solc/CommandLineParser.cpp +++ b/solc/CommandLineParser.cpp @@ -329,18 +329,6 @@ bool CommandLineParser::parseInputPathsAndRemappings() return false; } - if (!remapping->target.empty()) - { - // If the target is a directory, whitelist it. Otherwise whitelist containing dir. - // NOTE: /a/b/c/ is a directory while /a/b/c is not. - boost::filesystem::path remappingDir = remapping->target; - if (remappingDir.filename() != "..") - // As an exception we'll treat /a/b/c/.. as a directory too. It would be - // unintuitive to whitelist /a/b/c when the target is equivalent to /a/b/. - remappingDir.remove_filename(); - m_options.input.allowedDirectories.insert(remappingDir.empty() ? "." : remappingDir); - } - m_options.input.remappings.emplace_back(move(remapping.value())); } else if (positionalArg == "-") diff --git a/test/solc/CommandLineInterface.cpp b/test/solc/CommandLineInterface.cpp index 3d1fce7d2..969a3b56d 100644 --- a/test/solc/CommandLineInterface.cpp +++ b/test/solc/CommandLineInterface.cpp @@ -160,8 +160,6 @@ BOOST_AUTO_TEST_CASE(cli_input) PathSet expectedAllowedPaths = { boost::filesystem::canonical(tempDir1), boost::filesystem::canonical(tempDir2), - "b/c", - "c/d/e", }; OptionsReaderAndMessages result = parseCommandLineAndReadInputFiles({ diff --git a/test/solc/CommandLineInterfaceAllowPaths.cpp b/test/solc/CommandLineInterfaceAllowPaths.cpp index a15199618..576ab5c5e 100644 --- a/test/solc/CommandLineInterfaceAllowPaths.cpp +++ b/test/solc/CommandLineInterfaceAllowPaths.cpp @@ -375,48 +375,48 @@ BOOST_FIXTURE_TEST_CASE(allow_path_automatic_whitelisting_input_files, AllowPath BOOST_TEST(checkImport("import 'contract.sol'", {"contract.sol"})); } -BOOST_FIXTURE_TEST_CASE(allow_path_automatic_whitelisting_remappings, AllowPathsFixture) +BOOST_FIXTURE_TEST_CASE(allow_path_remappings_do_not_whitelist, AllowPathsFixture) { - // Adding a remapping whitelists target's parent directory and subdirectories - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/c.sol"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c/d.sol'", {"x=../code/a/b/c.sol"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/X.sol'", {"x=../code/a/b/c.sol"})); + // Adding a remapping does not whitelist target's parent directory and subdirectories + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/c.sol"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c/d.sol'", {"x=../code/a/b/c.sol"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/X.sol'", {"x=../code/a/b/c.sol"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b/c.sol"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=../code/a/b/c.sol"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/bc/d.sol'", {"x=../code/a/b/c.sol"}) == ImportCheck::PathDisallowed()); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=/contract.sol"})); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=/contract.sol"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=/contract.sol/"}) == ImportCheck::PathDisallowed()); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {m_portablePrefix + "/a/b=../code/X/b"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {m_portablePrefix + "/a/b/=../code/X/b/"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/bc/d.sol'", {m_portablePrefix + "/a/b=../code/X/b"})); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {m_portablePrefix + "/a/b=../code/X/b"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {m_portablePrefix + "/a/b/=../code/X/b/"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/bc/d.sol'", {m_portablePrefix + "/a/b=../code/X/b"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/bc/d.sol'", {m_portablePrefix + "/a/b/=../code/X/b/"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {m_portablePrefix + "/a/b:y/z=x/w"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {m_portablePrefix + "/a/b:y/z=x/w"}) == ImportCheck::PathDisallowed()); - // Adding a remapping whitelists the target and subdirectories when the target is a directory - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c/d.sol'", {"x=../code/a/b/"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/X.sol'", {"x=../code/a/b/"})); + // Adding a remapping does not whitelist the target and subdirectories when the target is a directory + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c/d.sol'", {"x=../code/a/b/"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/X.sol'", {"x=../code/a/b/"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b/"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=../code/a/b/"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/bc/d.sol'", {"x=../code/a/b/"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/bc/d.sol'", {"x=../code/a/c/"}) == ImportCheck::PathDisallowed()); - // Adding a remapping whitelists target's parent directory and subdirectories when the target + // Adding a remapping does not whitelist target's parent directory and subdirectories when the target // is a directory but does not have a trailing slash - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c/d.sol'", {"x=../code/a/b"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/X.sol'", {"x=../code/a/b"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b"})); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c/d.sol'", {"x=../code/a/b"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/X.sol'", {"x=../code/a/b"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=../code/a/b"}) == ImportCheck::PathDisallowed()); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/bc/d.sol'", {"x=../code/a/b"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/bc/d.sol'", {"x=../code/a/c"})); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/bc/d.sol'", {"x=../code/a/b"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/bc/d.sol'", {"x=../code/a/c"}) == ImportCheck::PathDisallowed()); - // Adding a remapping to a relative target at VFS root whitelists the work dir + // Adding a remapping to a relative target at VFS root does not whitelist the work dir BOOST_TEST(checkImport("import '/../../x/y/z.sol'", {"x=contract.sol", "--base-path=../code/a/b/"}) == ImportCheck::PathDisallowed()); - BOOST_TEST(checkImport("import '/../../../work/a/b/c.sol'", {"x=contract.sol", "--base-path=../code/a/b/"})); + BOOST_TEST(checkImport("import '/../../../work/a/b/c.sol'", {"x=contract.sol", "--base-path=../code/a/b/"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '/../../x/y/z.sol'", {"x=contract.sol/", "--base-path=../code/a/b/"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '/../../../work/a/b/c.sol'", {"x=contract.sol/", "--base-path=../code/a/b/"}) == ImportCheck::PathDisallowed()); @@ -426,7 +426,7 @@ BOOST_FIXTURE_TEST_CASE(allow_path_automatic_whitelisting_remappings, AllowPaths BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"../code/="}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '/../work/a/b/c.sol'", {"../code/=", "--base-path", m_portablePrefix}) == ImportCheck::PathDisallowed()); - // Adding a remapping that includes .. or . segments whitelists the parent dir and subdirectories + // Adding a remapping that includes .. or . segments does not whitelist the parent dir and subdirectories // of the resolved target BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=."}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=."}) == ImportCheck::PathDisallowed()); @@ -434,25 +434,25 @@ BOOST_FIXTURE_TEST_CASE(allow_path_automatic_whitelisting_remappings, AllowPaths BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=./"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=./"}) == ImportCheck::PathDisallowed()); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=.."})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=.."})); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=.."}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=.."}) == ImportCheck::PathDisallowed()); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=../"})); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=../"}) == ImportCheck::PathDisallowed()); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/./.."})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b/./.."})); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/./.."}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b/./.."}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=../code/a/b/./.."}) == ImportCheck::PathDisallowed()); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/./../"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b/./../"})); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/./../"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b/./../"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=../code/a/b/./../"}) == ImportCheck::PathDisallowed()); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/./../b"})); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b/./../b"})); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/./../b"}) == ImportCheck::PathDisallowed()); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b/./../b"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=../code/a/b/./../b"}) == ImportCheck::PathDisallowed()); - BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/./../b/"})); + BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/b/c.sol'", {"x=../code/a/b/./../b/"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/a/X/c.sol'", {"x=../code/a/b/./../b/"}) == ImportCheck::PathDisallowed()); BOOST_TEST(checkImport("import '" + m_portablePrefix + "/X/b/c.sol'", {"x=../code/a/b/./../b/"}) == ImportCheck::PathDisallowed()); diff --git a/test/solc/CommandLineParser.cpp b/test/solc/CommandLineParser.cpp index f2e4ea387..296f43f87 100644 --- a/test/solc/CommandLineParser.cpp +++ b/test/solc/CommandLineParser.cpp @@ -173,7 +173,7 @@ BOOST_AUTO_TEST_CASE(cli_mode_options) expectedOptions.input.basePath = "/home/user/"; expectedOptions.input.includePaths = {"/usr/lib/include/", "/home/user/include"}; - expectedOptions.input.allowedDirectories = {"/tmp", "/home", "project", "../contracts", "c", "/usr/lib"}; + expectedOptions.input.allowedDirectories = {"/tmp", "/home", "project", "../contracts"}; expectedOptions.input.ignoreMissingFiles = true; expectedOptions.input.errorRecovery = (inputMode == InputMode::Compiler); expectedOptions.output.dir = "/tmp/out"; @@ -313,7 +313,7 @@ BOOST_AUTO_TEST_CASE(assembly_mode_options) expectedOptions.input.addStdin = true; expectedOptions.input.basePath = "/home/user/"; expectedOptions.input.includePaths = {"/usr/lib/include/", "/home/user/include"}; - expectedOptions.input.allowedDirectories = {"/tmp", "/home", "project", "../contracts", "c", "/usr/lib"}; + expectedOptions.input.allowedDirectories = {"/tmp", "/home", "project", "../contracts"}; expectedOptions.input.ignoreMissingFiles = true; expectedOptions.output.overwriteFiles = true; expectedOptions.output.evmVersion = EVMVersion::spuriousDragon();