cerc-io/solidity
10
0
Fork 0
mirror of https://github.com/ethereum/solidity synced 2023-10-03 13:03:40 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge pull request #12021 from soroosh-sdi/remapping_not_whitlist

Browse Source 
Do not whitelist remapping targets
This commit is contained in:
Kamil Śliwak 2021-10-20 18:02:03 +02:00 committed by GitHub
commit e6f6ca40bb
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 38 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
Changelog.md
View File

@ -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)

5
docs/path-resolution.rst
View File

@ -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 <import-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``),

3
docs/using-the-compiler.rst
View File

@ -54,8 +54,7 @@ or ../ <direct-imports>` 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 <allowed-paths>`.
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.

12
solc/CommandLineParser.cpp
View File

@ -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 == "-")

2
test/solc/CommandLineInterface.cpp
View File

@ -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({

68
test/solc/CommandLineInterfaceAllowPaths.cpp
View File

@ -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());

4
test/solc/CommandLineParser.cpp
View File

@ -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();