project_loader, formatting_utils: take empty env values into account (#…

…3345)

Prevent library injection vulnerability on strict mode snaps built
with Snapcraft via misconfigured LD_LIBRARY_PATH:

- project_loader: do not export empty environment
- meta: do not export empty environment. Warn on empty environment.

CVE-2020-27348
LP: #1901572

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

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(

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)
 

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

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

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"

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