diff --git a/snapcraft/formatting_utils.py b/snapcraft/formatting_utils.py index a0966cd379..cf33bfb436 100644 --- a/snapcraft/formatting_utils.py +++ b/snapcraft/formatting_utils.py @@ -39,15 +39,17 @@ def format_path_variable( :param str prepend: String to prepend to each path in the definition. :param str separator: String to place between each path in the definition. """ - if not paths: raise ValueError("Failed to format '${}': no paths supplied".format(envvar)) - return '{envvar}="${envvar}{separator}{paths}"'.format( - envvar=envvar, - separator=separator, - paths=combine_paths(paths, prepend, separator), - ) + combined_paths = combine_paths(paths, prepend, separator) + + if separator.isspace(): + formatted = f'{envvar}="${envvar}{separator}{combined_paths}"' + else: + formatted = f'{envvar}="${{{envvar}:+${envvar}{separator}}}{combined_paths}"' + + return formatted def humanize_list( diff --git a/snapcraft/internal/meta/_snap_packaging.py b/snapcraft/internal/meta/_snap_packaging.py index ed9e1de46d..5708712de2 100644 --- a/snapcraft/internal/meta/_snap_packaging.py +++ b/snapcraft/internal/meta/_snap_packaging.py @@ -92,6 +92,7 @@ def create_snap_packaging(project_config: _config.Config) -> str: packaging.setup_assets() packaging.generate_hook_wrappers() packaging.write_snap_directory() + packaging.warn_ld_library_paths() return packaging.meta_dir @@ -453,6 +454,53 @@ def write_snap_yaml(self) -> None: package_snap_path = os.path.join(self.meta_dir, "snap.yaml") self._snap_meta.write_snap_yaml(path=package_snap_path) + def warn_ld_library_paths(self) -> None: + root_ld_library_path = self._snap_meta.environment.get("LD_LIBRARY_PATH") + # Dictionary of app names with LD_LIBRARY_PATH in their environment. + app_environment: Dict[str, str] = dict() + + for app_name, app_props in self._config_data.get("apps", dict()).items(): + with contextlib.suppress(KeyError): + app_environment[app_name] = app_props["environment"]["LD_LIBRARY_PATH"] + + if root_ld_library_path is None and not app_environment: + return + + ld_library_path_empty: Set[str] = set() + if root_ld_library_path is None and app_environment: + ld_library_path_empty = { + name + for name, ld_env in app_environment.items() + if "$LD_LIBRARY_PATH" in ld_env or "${LD_LIBRARY_PATH}" in ld_env + } + elif ( + root_ld_library_path is not None + and "LD_LIBRARY_PATH" in root_ld_library_path + ): + ld_library_path_empty = {"."} + + _EMPTY_LD_LIBRARY_PATH_ITEM_PATTERN = re.compile("^:|::|:$") + + for name, ld_env in app_environment.items(): + if _EMPTY_LD_LIBRARY_PATH_ITEM_PATTERN.findall(ld_env): + ld_library_path_empty.add(name) + + if ( + root_ld_library_path is not None + and _EMPTY_LD_LIBRARY_PATH_ITEM_PATTERN.findall(root_ld_library_path) + ): + ld_library_path_empty.add(".") + + if ld_library_path_empty: + logger.warning( + "CVE-2020-27348: A potentially empty LD_LIBRARY_PATH has been set for environment " + "in {}. " + "The current working directory will be added to the library path if empty. " + "This can cause unexpected libraries to be loaded.".format( + formatting_utils.humanize_list(sorted(ld_library_path_empty), "and") + ) + ) + def setup_assets(self) -> None: # We do _setup_from_setup first since it is legacy and let the # declarative items take over. @@ -519,7 +567,9 @@ def _assemble_runtime_environment(self) -> str: # All ELF files have had rpath and interpreter patched. Strip all LD_LIBRARY_PATH variables env = [e for e in env if not e.startswith("export LD_LIBRARY_PATH=")] else: - env.append('export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH:$LD_LIBRARY_PATH"') + env.append( + 'export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}"' + ) return "\n".join(env) diff --git a/snapcraft/internal/project_loader/_config.py b/snapcraft/internal/project_loader/_config.py index 20dc002074..7f8c6f532d 100644 --- a/snapcraft/internal/project_loader/_config.py +++ b/snapcraft/internal/project_loader/_config.py @@ -351,7 +351,9 @@ def snap_env(self): if dependency_paths: # Add more specific LD_LIBRARY_PATH from the dependencies. env.append( - 'LD_LIBRARY_PATH="' + ":".join(dependency_paths) + ':$LD_LIBRARY_PATH"' + 'LD_LIBRARY_PATH="' + + ":".join(dependency_paths) + + '${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}"' ) return env diff --git a/snapcraft/internal/project_loader/_env.py b/snapcraft/internal/project_loader/_env.py index bae9f4e561..dd75b616f2 100644 --- a/snapcraft/internal/project_loader/_env.py +++ b/snapcraft/internal/project_loader/_env.py @@ -26,10 +26,8 @@ def runtime_env(root: str, arch_triplet: str) -> List[str]: env.append( 'PATH="' - + ":".join( - ["{0}/usr/sbin", "{0}/usr/bin", "{0}/sbin", "{0}/bin", "$PATH"] - ).format(root) - + '"' + + ":".join(["{0}/usr/sbin", "{0}/usr/bin", "{0}/sbin", "{0}/bin"]).format(root) + + '${PATH:+:$PATH}"' ) # Add the default LD_LIBRARY_PATH diff --git a/tests/unit/meta/test_meta.py b/tests/unit/meta/test_meta.py index b8a8e1acfb..0d2703ffef 100644 --- a/tests/unit/meta/test_meta.py +++ b/tests/unit/meta/test_meta.py @@ -1143,8 +1143,8 @@ def test_snap_hooks_stubs_not_created_stubs_from_command_chain(self): textwrap.dedent( """\ #!/bin/sh - export PATH="$SNAP/usr/sbin:$SNAP/usr/bin:$SNAP/sbin:$SNAP/bin:$PATH" - export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH:$LD_LIBRARY_PATH" + export PATH="$SNAP/usr/sbin:$SNAP/usr/bin:$SNAP/sbin:$SNAP/bin${PATH:+:$PATH}" + export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" exec "$SNAP/snap/hooks/install" "$@" """ ) @@ -1261,7 +1261,7 @@ def test_generated_hook_wrappers_include_environment(self, mock_snap_env): """\ #!/bin/sh export PATH=$SNAP/foo - export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH:$LD_LIBRARY_PATH" + export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" exec "$SNAP/snap/hooks/snap-hook" "$@" """ ) @@ -1310,6 +1310,176 @@ def test_no_confinement(self): ) +class TestRootEnvironmentLibraryPathWarnings(CreateBaseTestCase): + def setUp(self): + super().setUp() + + self.config_data["grade"] = "stable" + + self.fake_logger = fixtures.FakeLogger(level=logging.WARNING) + self.useFixture(self.fake_logger) + + def assert_warnings(self): + self.generate_meta_yaml() + + self.assertThat( + self.fake_logger.output, + Contains( + "CVE-2020-27348: A potentially empty LD_LIBRARY_PATH has been set for environment " + "in '.'. The current working directory will be added to the library path if empty. " + "This can cause unexpected libraries to be loaded." + ), + ) + + def assert_no_warnings(self): + self.generate_meta_yaml() + + self.assertThat( + self.fake_logger.output, + Not( + Contains( + "CVE-2020-27348: A potentially empty LD_LIBRARY_PATH has been set for environment " + "in '.'. The current working directory will be added to the library path if empty. " + "This can cause unexpected libraries to be loaded." + ) + ), + ) + + def test_root_ld_library_path(self): + self.config_data["environment"] = {"LD_LIBRARY_PATH": "/foo:$LD_LIBRARY_PATH"} + + self.assert_warnings() + + def test_root_ld_library_path_braces(self): + self.config_data["environment"] = {"LD_LIBRARY_PATH": "/foo:${LD_LIBRARY_PATH}"} + + self.assert_warnings() + + def test_root_ld_library_path_with_colon_at_start(self): + self.config_data["environment"] = {"LD_LIBRARY_PATH": ":/foo:/bar"} + + self.assert_warnings() + + def test_root_ld_library_path_with_colon_at_end(self): + self.config_data["environment"] = {"LD_LIBRARY_PATH": "/foo:/bar:"} + + self.assert_warnings() + + def test_root_ld_library_path_with_colon_at_middle(self): + self.config_data["environment"] = {"LD_LIBRARY_PATH": "/foo::/bar"} + + self.assert_warnings() + + def test_root_ld_library_path_good(self): + self.config_data["environment"] = {"LD_LIBRARY_PATH": "/foo:/bar"} + + self.assert_no_warnings() + + +class TestAppsEnvironmentLibraryPathWarnings(CreateBaseTestCase): + def setUp(self): + super().setUp() + + self.config_data["grade"] = "stable" + self.config_data["apps"] = { + "app1": {"command": "foo"}, + "app2": {"command": "bar"}, + } + + _create_file(os.path.join(self.prime_dir, "foo"), executable=True) + _create_file(os.path.join(self.prime_dir, "bar"), executable=True) + + self.fake_logger = fixtures.FakeLogger(level=logging.WARNING) + self.useFixture(self.fake_logger) + + def assert_warnings_both(self): + self.generate_meta_yaml() + + self.assertThat( + self.fake_logger.output, + Contains( + "CVE-2020-27348: A potentially empty LD_LIBRARY_PATH has been set for environment " + "in 'app1' and 'app2'. The current working directory will be added to the library " + "path if empty. " + "This can cause unexpected libraries to be loaded." + ), + ) + + def assert_warnings_app1(self): + self.generate_meta_yaml() + + self.assertThat( + self.fake_logger.output, + Contains( + "CVE-2020-27348: A potentially empty LD_LIBRARY_PATH has been set for environment " + "in 'app1'. The current working directory will be added to the library " + "path if empty. " + "This can cause unexpected libraries to be loaded." + ), + ) + + def assert_no_warnings(self): + self.generate_meta_yaml() + + self.assertThat( + self.fake_logger.output, + Not( + Contains( + "CVE-2020-27348: A potentially empty LD_LIBRARY_PATH has been set for environment " + "in 'app1' and 'app2'. The current working directory will be added to the library " + "path if empty. " + "This can cause unexpected libraries to be loaded." + ) + ), + ) + + def test_app_ld_library_path_app1(self): + self.config_data["apps"]["app1"]["environment"] = { + "LD_LIBRARY_PATH": "/foo:$LD_LIBRARY_PATH" + } + + self.assert_warnings_app1() + + def test_app_ld_library_path_app1_braces(self): + self.config_data["apps"]["app1"]["environment"] = { + "LD_LIBRARY_PATH": "/foo:${LD_LIBRARY_PATH}" + } + + self.assert_warnings_app1() + + def test_app_ld_library_path_app1_app2(self): + self.config_data["apps"]["app1"]["environment"] = { + "LD_LIBRARY_PATH": "/foo:$LD_LIBRARY_PATH" + } + self.config_data["apps"]["app2"]["environment"] = { + "LD_LIBRARY_PATH": "/foo:$LD_LIBRARY_PATH" + } + + self.assert_warnings_both() + + def test_root_ld_library_path_set(self): + self.config_data["environment"] = {"LD_LIBRARY_PATH": "/foo:/bar"} + + self.config_data["apps"]["app1"]["environment"] = { + "LD_LIBRARY_PATH": "/foo:$LD_LIBRARY_PATH" + } + self.config_data["apps"]["app2"]["environment"] = { + "LD_LIBRARY_PATH": "/foo:$LD_LIBRARY_PATH" + } + + self.assert_no_warnings() + + def test_app_ld_library_path_colon_middle_app1_app2(self): + self.config_data["apps"]["app1"]["environment"] = { + "LD_LIBRARY_PATH": "/foo::/bar" + } + self.config_data["apps"]["app2"]["environment"] = { + "LD_LIBRARY_PATH": "/foo::/bar" + } + + self.assert_warnings_both() + + class TestGrade(CreateBaseTestCase): def test_stable(self): self.config_data["grade"] = "stable" diff --git a/tests/unit/meta/test_snap_packaging.py b/tests/unit/meta/test_snap_packaging.py index e21f0b5270..82a8c2bbc7 100644 --- a/tests/unit/meta/test_snap_packaging.py +++ b/tests/unit/meta/test_snap_packaging.py @@ -69,8 +69,8 @@ def test_strict_app(self): expected_runner = textwrap.dedent( """ #!/bin/sh - export PATH="$SNAP/usr/sbin:$SNAP/usr/bin:$SNAP/sbin:$SNAP/bin:$PATH" - export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH:$LD_LIBRARY_PATH" + export PATH="$SNAP/usr/sbin:$SNAP/usr/bin:$SNAP/sbin:$SNAP/bin${PATH:+:$PATH}" + export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" exec "$@" """ ).lstrip() @@ -145,8 +145,8 @@ def test_assembled_runtime_environment_strict(self): expected_env = textwrap.dedent( """ - export PATH="$SNAP/usr/sbin:$SNAP/usr/bin:$SNAP/sbin:$SNAP/bin:$PATH" - export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH:$LD_LIBRARY_PATH" + export PATH="$SNAP/usr/sbin:$SNAP/usr/bin:$SNAP/sbin:$SNAP/bin${PATH:+:$PATH}" + export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" """ ).strip() @@ -165,7 +165,7 @@ def test_assembled_runtime_environment_strict_patched(self): # Verify that, since all parts are using patchelf, no LD_LIBRARY_PATH is set expected_env = textwrap.dedent( """ - export PATH="$SNAP/usr/sbin:$SNAP/usr/bin:$SNAP/sbin:$SNAP/bin:$PATH" + export PATH="$SNAP/usr/sbin:$SNAP/usr/bin:$SNAP/sbin:$SNAP/bin${PATH:+:$PATH}" """ ).strip() diff --git a/tests/unit/project_loader/test_environment.py b/tests/unit/project_loader/test_environment.py index 7f4b80ad03..b2d1bc2cfd 100644 --- a/tests/unit/project_loader/test_environment.py +++ b/tests/unit/project_loader/test_environment.py @@ -57,10 +57,6 @@ def test_config_snap_environment(self): lib_paths = [ os.path.join(self.prime_dir, "lib"), os.path.join(self.prime_dir, "usr", "lib"), - os.path.join(self.prime_dir, "lib", project_config.project.arch_triplet), - os.path.join( - self.prime_dir, "usr", "lib", project_config.project.arch_triplet - ), ] for lib_path in lib_paths: os.makedirs(lib_path) @@ -69,43 +65,25 @@ def test_config_snap_environment(self): self.assertThat( environment, Contains( - 'PATH="{0}/usr/sbin:{0}/usr/bin:{0}/sbin:{0}/bin:$PATH"'.format( + 'PATH="{0}/usr/sbin:{0}/usr/bin:{0}/sbin:{0}/bin${{PATH:+:$PATH}}"'.format( self.prime_dir ) ), ) - - # Ensure that LD_LIBRARY_PATH is present and it contains only the - # basics. - paths = [] - for variable in environment: - if "LD_LIBRARY_PATH" in variable: - these_paths = variable.split("=")[1].strip() - paths.extend(these_paths.replace('"', "").split(":")) - - self.assertTrue(len(paths) > 0, "Expected LD_LIBRARY_PATH to be in environment") - - expected = ( - os.path.join(self.prime_dir, i) - for i in [ - "lib", - os.path.join("usr", "lib"), - os.path.join("lib", project_config.project.arch_triplet), - os.path.join("usr", "lib", project_config.project.arch_triplet), - ] + self.assertThat( + environment, + Contains( + 'LD_LIBRARY_PATH="${{LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}}' + '{0}/lib:{0}/usr/lib"'.format(self.prime_dir) + ), ) - for item in expected: - self.assertTrue( - item in paths, - "Expected LD_LIBRARY_PATH in {!r} to include {!r}".format(paths, item), - ) def test_config_snap_environment_with_no_library_paths(self): project_config = self.make_snapcraft_project(self.snapcraft_yaml) environment = project_config.snap_env() self.assertTrue( - 'PATH="{0}/usr/sbin:{0}/usr/bin:{0}/sbin:{0}/bin:$PATH"'.format( + 'PATH="{0}/usr/sbin:{0}/usr/bin:{0}/sbin:{0}/bin${{PATH:+:$PATH}}"'.format( self.prime_dir ) in environment, @@ -132,20 +110,14 @@ def test_config_snap_environment_with_dependencies(self, mock_get_dependencies): # Ensure that LD_LIBRARY_PATH is present and it contains the # extra dependency paths. - paths = [] - for variable in project_config.snap_env(): - if "LD_LIBRARY_PATH" in variable: - these_paths = variable.split("=")[1].strip() - paths.extend(these_paths.replace('"', "").split(":")) - - self.assertTrue(len(paths) > 0, "Expected LD_LIBRARY_PATH to be in environment") - - expected = (os.path.join(self.prime_dir, i) for i in ["lib1", "lib2"]) - for item in expected: - self.assertTrue( - item in paths, - "Expected LD_LIBRARY_PATH ({!r}) to include {!r}".format(paths, item), - ) + self.assertThat( + project_config.snap_env(), + Contains( + 'LD_LIBRARY_PATH="{0}/lib1:{0}/lib2${{LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}}"'.format( + self.prime_dir + ) + ), + ) @mock.patch.object( snapcraft.internal.pluginhandler.PluginHandler, "get_primed_dependency_paths" @@ -241,13 +213,13 @@ def test_config_env_dedup(self): 'PATH="{0}/parts/main/install/usr/sbin:' "{0}/parts/main/install/usr/bin:" "{0}/parts/main/install/sbin:" - '{0}/parts/main/install/bin:$PATH"' + '{0}/parts/main/install/bin${{PATH:+:$PATH}}"' ).format(self.path), ( 'PATH="{0}/stage/usr/sbin:' "{0}/stage/usr/bin:" "{0}/stage/sbin:" - '{0}/stage/bin:$PATH"' + '{0}/stage/bin${{PATH:+:$PATH}}"' ).format(self.path), 'PERL5LIB="{0}/stage/usr/share/perl5/"'.format(self.path), 'SNAPCRAFT_ARCH_TRIPLET="{}"'.format( @@ -326,42 +298,39 @@ def test_config_stage_environment(self): project_config = self.make_snapcraft_project(self.snapcraft_yaml) environment = project_config.stage_env() - self.assertTrue( - 'PATH="{0}/usr/sbin:{0}/usr/bin:{0}/sbin:{0}/bin:$PATH"'.format( + self.assertIn( + 'PATH="{0}/usr/sbin:{0}/usr/bin:{0}/sbin:{0}/bin${{PATH:+:$PATH}}"'.format( self.stage_dir - ) - in environment + ), + environment, ) - self.assertTrue( - 'LD_LIBRARY_PATH="$LD_LIBRARY_PATH:{stage_dir}/lib:' + self.assertIn( + 'LD_LIBRARY_PATH="${{LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}}{stage_dir}/lib:' "{stage_dir}/usr/lib:{stage_dir}/lib/{arch_triplet}:" '{stage_dir}/usr/lib/{arch_triplet}"'.format( stage_dir=self.stage_dir, arch_triplet=project_config.project.arch_triplet, - ) - in environment, - "Current environment is {!r}".format(environment), + ), + environment, ) - self.assertTrue( + self.assertIn( 'CFLAGS="$CFLAGS -isystem{stage_dir}/include -isystem{stage_dir}/usr/include ' "-isystem{stage_dir}/include/{arch_triplet} " '-isystem{stage_dir}/usr/include/{arch_triplet}"'.format( stage_dir=self.stage_dir, arch_triplet=project_config.project.arch_triplet, - ) - in environment, - "Current environment is {!r}".format(environment), + ), + environment, ) - self.assertTrue( + self.assertIn( 'CPPFLAGS="$CPPFLAGS -isystem{stage_dir}/include ' "-isystem{stage_dir}/usr/include " "-isystem{stage_dir}/include/{arch_triplet} " '-isystem{stage_dir}/usr/include/{arch_triplet}"'.format( stage_dir=self.stage_dir, arch_triplet=project_config.project.arch_triplet, - ) - in environment, - "Current environment is {!r}".format(environment), + ), + environment, ) self.assertTrue( 'CXXFLAGS="$CXXFLAGS -isystem{stage_dir}/include ' diff --git a/tests/unit/test_formatting_utils.py b/tests/unit/test_formatting_utils.py index 87e9b7223e..7269eeda4e 100644 --- a/tests/unit/test_formatting_utils.py +++ b/tests/unit/test_formatting_utils.py @@ -14,6 +14,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . +import pytest from testtools.matchers import Equals from snapcraft import formatting_utils @@ -56,23 +57,27 @@ def test_another_conjunction(self): self.assertThat(output, Equals("'bar', 'baz', 'foo', or 'qux'")) -class FormatPathVariableTestCases(unit.TestCase): - def test_no_paths(self): - self.assertRaises( - ValueError, formatting_utils.format_path_variable, "PATH", None, "/usr", ":" - ) +def test_no_paths(): + with pytest.raises(ValueError): + formatting_utils.format_path_variable("PATH", list(), "/usr", ":") + + +def test_one_path(): + paths = ["/bin"] + output = formatting_utils.format_path_variable("PATH", paths, "/usr", ":") + + assert output == 'PATH="${PATH:+$PATH:}/usr/bin"' + + +def test_two_paths(): + paths = ["/bin", "/sbin"] + output = formatting_utils.format_path_variable("PATH", paths, "/usr", ":") + + assert output == 'PATH="${PATH:+$PATH:}/usr/bin:/usr/sbin"' - def test_one_path(self): - paths = ["/bin"] - output = formatting_utils.format_path_variable("PATH", paths, "/usr", ":") - self.assertThat(output, Equals('PATH="$PATH:/usr/bin"')) - def test_two_paths(self): - paths = ["/bin", "/sbin"] - output = formatting_utils.format_path_variable("PATH", paths, "/usr", ":") - self.assertThat(output, Equals('PATH="$PATH:/usr/bin:/usr/sbin"')) +def test_two_paths_other_paremeters(): + paths = ["/usr/bin", "/usr/sbin"] + output = formatting_utils.format_path_variable("PATH", paths, "", ",") - def test_two_paths_other_paremeters(self): - paths = ["/usr/bin", "/usr/sbin"] - output = formatting_utils.format_path_variable("PATH", paths, "", ",") - self.assertThat(output, Equals('PATH="$PATH,/usr/bin,/usr/sbin"')) + assert output == 'PATH="${PATH:+$PATH,}/usr/bin,/usr/sbin"'