Change file permissions on instances

include/multipass/platform.h

@@ -58,7 +58,11 @@ class Platform : public Singleton<Platform>
     virtual bool is_remote_supported(const std::string& remote) const;
     virtual bool is_backend_supported(const QString& backend) const; // temporary (?)
     virtual int chown(const char* path, unsigned int uid, unsigned int gid) const;
-    virtual bool set_permissions(const std::filesystem::path& path, std::filesystem::perms permissions) const;
+    virtual bool set_permissions(const std::filesystem::path& path,
+                                 std::filesystem::perms permissions,
+                                 bool try_inherit = false) const;
+    virtual bool take_ownership(const std::filesystem::path& path) const;
+    virtual void setup_permission_inheritance(bool restricted = true) const;
     virtual bool link(const char* target, const char* link) const;
     virtual bool symlink(const char* target, const char* link, bool is_dir) const;
     virtual int utime(const char* path, int atime, int mtime) const;

include/multipass/utils/permission_utils.h

@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) Canonical, Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef MULTIPASS_PERMISSION_UTILS_H
+#define MULTIPASS_PERMISSION_UTILS_H
+
+#include <multipass/singleton.h>
+
+#include <QFileDevice>
+#include <filesystem>
+
+#define MP_PERMISSIONS multipass::PermissionUtils::instance()
+
+namespace multipass
+{
+namespace fs = std::filesystem;
+
+class PermissionUtils : public Singleton<PermissionUtils>
+{
+public:
+    PermissionUtils(const PrivatePass&) noexcept;
+
+    virtual void set_permissions(const fs::path& path, const fs::perms& permissions) const;
+    virtual void take_ownership(const fs::path& path) const;
+
+    // sets owner to root and sets permissions such that only owner has access.
+    virtual void restrict_permissions(const fs::path& path) const;
+};
+} // namespace multipass
+
+#endif // MULTIPASS_PERMISSION_UTILS_H

src/cert/client_cert_store.cpp

@@ -76,8 +76,6 @@ void mp::ClientCertStore::add_cert(const std::string& pem_cert)
     if (!MP_FILEOPS.open(file, QIODevice::WriteOnly))
         throw std::runtime_error("failed to create file to store certificate");
 
-    file.setPermissions(QFile::ReadOwner | QFile::WriteOwner);
-
     // QIODevice::Append is not supported in QSaveFile, so must write out all of the
     // existing clients certs each time.
     for (const auto& saved_cert : authenticated_client_certs)

src/daemon/daemon_config.cpp

@@ -31,6 +31,7 @@
 #include <multipass/ssl_cert_provider.h>
 #include <multipass/standard_paths.h>
 #include <multipass/utils.h>
+#include <multipass/utils/permission_utils.h>
 
 #include <QString>
 #include <QSysInfo>
@@ -108,6 +109,8 @@ std::unique_ptr<const mp::DaemonConfig> mp::DaemonConfigBuilder::build()
     auto multiplexing_logger = std::make_shared<mpl::MultiplexingLogger>(std::move(logger));
     mpl::set_logger(multiplexing_logger);
 
+    MP_PLATFORM.setup_permission_inheritance();
+
     auto storage_path = MP_PLATFORM.multipass_storage_location();
     if (!storage_path.isEmpty())
         MP_UTILS.make_dir(storage_path, std::filesystem::perms::owner_all);
@@ -189,6 +192,17 @@ std::unique_ptr<const mp::DaemonConfig> mp::DaemonConfigBuilder::build()
                 std::make_unique<DefaultVMBlueprintProvider>(url_downloader.get(), cache_directory, manifest_ttl);
     }
 
+    // restrict permissions for all existing files and folders
+    if (!storage_path.isEmpty())
+    {
+        MP_PERMISSIONS.restrict_permissions(storage_path.toStdU16String());
+    }
+    else
+    {
+        MP_PERMISSIONS.restrict_permissions(data_directory.toStdU16String());
+        MP_PERMISSIONS.restrict_permissions(cache_directory.toStdU16String());
+    }
+
     return std::unique_ptr<const DaemonConfig>(new DaemonConfig{
         std::move(url_downloader), std::move(factory), std::move(image_hosts), std::move(vault),
         std::move(name_generator), std::move(ssh_key_provider), std::move(cert_provider), std::move(client_cert_store),

src/platform/platform_unix.cpp

@@ -59,8 +59,11 @@
 }
 
 bool mp::platform::Platform::set_permissions(const std::filesystem::path& path,
-                                             std::filesystem::perms permissions) const
+                                             std::filesystem::perms permissions,
+                                             bool) const
 {
+    // try_inherit is ignored on unix since it only pertains to ACLs
+
     std::error_code ec{};
     std::filesystem::permissions(path, permissions, ec);
 
@@ -74,6 +77,25 @@
     return !ec;
 }
 
+bool mp::platform::Platform::take_ownership(const std::filesystem::path& path) const
+{
+    return this->chown(path.u8string().c_str(), 0, 0) == 0;
+}
+
+void mp::platform::Platform::setup_permission_inheritance(bool restricted) const
+{
+    if (restricted)
+    {
+        // only user can read/write/execute
+        ::umask(~(S_IRUSR | S_IWUSR | S_IXUSR));
+    }
+    else
+    {
+        // typical default umask permissions
+        ::umask(S_IWGRP | S_IWOTH);
+    }
+}
+
 bool mp::platform::Platform::symlink(const char* target, const char* link, bool is_dir) const
 {
     return ::symlink(target, link) == 0;

src/sshfs_mount/sshfs_server.cpp

@@ -103,6 +103,8 @@ int main(int argc, char* argv[])
     auto standard_logger = std::make_shared<mpl::MultiplexingLogger>(std::move(logger));
     mpl::set_logger(standard_logger);
 
+    MP_PLATFORM.setup_permission_inheritance(false);
+
     try
     {
         auto watchdog = mpp::make_quit_watchdog(); // called while there is only one thread

src/utils/CMakeLists.txt

@@ -16,6 +16,7 @@ function(add_target TARGET_NAME)
   add_library(${TARGET_NAME} STATIC
     file_ops.cpp
     memory_size.cpp
+    permission_utils.cpp
     json_utils.cpp
     snap_utils.cpp
     standard_paths.cpp

src/utils/permission_utils.cpp

@@ -0,0 +1,102 @@
+/*
+ * Copyright (C) Canonical, Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <multipass/utils/permission_utils.h>
+
+#include <multipass/file_ops.h>
+#include <multipass/platform.h>
+
+namespace mp = multipass;
+namespace fs = mp::fs;
+
+namespace
+{
+void set_single_permissions(const fs::path& path, const fs::perms& permissions, bool try_inherit)
+{
+    if (!MP_PLATFORM.set_permissions(path, permissions, try_inherit))
+        throw std::runtime_error(fmt::format("Cannot set permissions for '{}'", path.string()));
+}
+
+void set_single_owner(const fs::path& path)
+{
+    if (!MP_PLATFORM.take_ownership(path))
+        throw std::runtime_error(fmt::format("Cannot set owner for '{}'", path.string()));
+}
+
+// only exists because MP_FILEOPS doesn't overload the throwing variaions of std::filesystem functions
+void throw_if_error(const fs::path& path, const std::error_code& ec)
+{
+    if (ec)
+        throw std::system_error(
+            ec,
+            fmt::format("System error occurred while handling permissions for '{}'", path.string()));
+}
+
+// recursively iterates over all files in folder and applies a function that takes a path
+template <class Func>
+void apply_on_files(const fs::path& path, Func&& func)
+{
+    std::error_code ec{};
+    if (!MP_FILEOPS.exists(path, ec) || ec)
+        throw std::runtime_error(fmt::format("Cannot handle permissions for nonexistent file '{}'", path.string()));
+
+    func(path, true);
+
+    // iterate over children of directory
+    if (MP_FILEOPS.is_directory(path, ec))
+    {
+        auto dir_iterator = MP_FILEOPS.recursive_dir_iterator(path, ec);
+        throw_if_error(path, ec);
+
+        if (!dir_iterator) [[unlikely]]
+            throw std::runtime_error(fmt::format("Cannot iterate over directory '{}'", path.string()));
+
+        while (dir_iterator->hasNext())
+        {
+            const auto& entry = dir_iterator->next();
+
+            func(entry.path(), false);
+        }
+    }
+
+    throw_if_error(path, ec);
+}
+} // namespace
+
+mp::PermissionUtils::PermissionUtils(const PrivatePass& pass) noexcept : Singleton{pass}
+{
+}
+
+void mp::PermissionUtils::set_permissions(const fs::path& path, const fs::perms& permissions) const
+{
+    apply_on_files(path, [&](const fs::path& apply_path, bool root_dir) {
+        set_single_permissions(apply_path, permissions, !root_dir);
+    });
+}
+
+void mp::PermissionUtils::take_ownership(const fs::path& path) const
+{
+    apply_on_files(path, [&](const fs::path& apply_path, bool) { set_single_owner(apply_path); });
+}
+
+void mp::PermissionUtils::restrict_permissions(const fs::path& path) const
+{
+    apply_on_files(path, [&](const fs::path& apply_path, bool root_dir) {
+        set_single_owner(apply_path);
+        set_single_permissions(apply_path, fs::perms::owner_all, !root_dir);
+    });
+}

src/utils/utils.cpp

@@ -27,6 +27,7 @@
 #include <multipass/ssh/ssh_session.h>
 #include <multipass/standard_paths.h>
 #include <multipass/utils.h>
+#include <multipass/utils/permission_utils.h>
 
 #include <QDir>
 #include <QFileInfo>
@@ -298,7 +299,7 @@ mp::Path mp::Utils::make_dir(const QDir& a_dir, const QString& name, std::filesy
 
     if (permissions != std::filesystem::perms::none)
     {
-        MP_PLATFORM.set_permissions(dir_path.toStdU16String(), permissions);
+        MP_PERMISSIONS.set_permissions(dir_path.toStdU16String(), permissions);
     }
 
     return dir_path;

tests/CMakeLists.txt

@@ -124,6 +124,7 @@ add_executable(multipass_tests
   test_sftp_utils.cpp
   test_file_ops.cpp
   test_recursive_dir_iter.cpp
+  test_permission_utils.cpp
 )
 
 target_include_directories(multipass_tests

tests/linux/test_platform_linux.cpp

@@ -633,7 +633,7 @@ TEST_F(PlatformLinux, create_alias_script_overwrites)
     EXPECT_CALL(*mock_utils, make_file_with_content(_, _, true)).Times(1);
     EXPECT_CALL(*mock_file_ops, get_permissions(_))
         .WillOnce(Return(mp::fs::perms::owner_read | mp::fs::perms::owner_write));
-    EXPECT_CALL(*mock_platform, set_permissions(_, _)).WillOnce(Return(true));
+    EXPECT_CALL(*mock_platform, set_permissions(_, _, _)).WillOnce(Return(true));
 
     // Calls the platform function directly since MP_PLATFORM is mocked.
     EXPECT_NO_THROW(
@@ -674,7 +674,7 @@ TEST_F(PlatformLinux, create_alias_script_throws_if_cannot_set_permissions)
     EXPECT_CALL(*mock_utils, make_file_with_content(_, _, true)).Times(1);
     EXPECT_CALL(*mock_file_ops, get_permissions(_))
         .WillOnce(Return(mp::fs::perms::owner_read | mp::fs::perms::owner_write));
-    EXPECT_CALL(*mock_platform, set_permissions(_, _)).WillOnce(Return(false));
+    EXPECT_CALL(*mock_platform, set_permissions(_, _, _)).WillOnce(Return(false));
 
     MP_EXPECT_THROW_THAT(
         mock_platform->Platform::create_alias_script("alias_name", mp::AliasDefinition{"instance", "command", "map"}),

tests/mock_permission_utils.h

@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) Canonical, Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef MULTIPASS_MOCK_PERMISSION_UTILS_H
+#define MULTIPASS_MOCK_PERMISSION_UTILS_H
+
+#include "common.h"
+#include "mock_singleton_helpers.h"
+
+#include <multipass/utils/permission_utils.h>
+
+namespace multipass::test
+{
+
+class MockPermissionUtils : public PermissionUtils
+{
+public:
+    MockPermissionUtils(const PrivatePass& pass) : PermissionUtils(pass)
+    {
+    }
+
+    MOCK_METHOD(void, set_permissions, (const fs::path& path, const fs::perms& permissions), (const, override));
+    MOCK_METHOD(void, take_ownership, (const fs::path& path), (const, override));
+    MOCK_METHOD(void, restrict_permissions, (const fs::path& path), (const, override));
+
+    MP_MOCK_SINGLETON_BOILERPLATE(MockPermissionUtils, PermissionUtils);
+};
+} // namespace multipass::test
+
+#endif // MULTIPASS_MOCK_PERMISSION_UTILS_H

tests/mock_platform.h

@@ -40,8 +40,10 @@ class MockPlatform : public platform::Platform
     MOCK_METHOD(bool, is_remote_supported, (const std::string&), (const, override));
     MOCK_METHOD(bool, is_backend_supported, (const QString&), (const, override));
     MOCK_METHOD(bool, is_alias_supported, (const std::string&, const std::string&), (const, override));
-    MOCK_METHOD(bool, set_permissions, (const std::filesystem::path&, std::filesystem::perms), (const, override));
     MOCK_METHOD(int, chown, (const char*, unsigned int, unsigned int), (const, override));
+    MOCK_METHOD(bool, set_permissions, (const std::filesystem::path&, std::filesystem::perms, bool), (const, override));
+    MOCK_METHOD(bool, take_ownership, (const std::filesystem::path&), (const, override));
+    MOCK_METHOD(void, setup_permission_inheritance, (bool), (const, override));
     MOCK_METHOD(bool, link, (const char*, const char*), (const, override));
     MOCK_METHOD(bool, symlink, (const char*, const char*, bool), (const, override));
     MOCK_METHOD(int, utime, (const char*, int, int), (const, override));

tests/test_alias_dict.cpp

@@ -28,6 +28,7 @@
 #include "file_operations.h"
 #include "json_test_utils.h"
 #include "mock_file_ops.h"
+#include "mock_permission_utils.h"
 #include "mock_platform.h"
 #include "mock_settings.h"
 #include "mock_vm_image_vault.h"
@@ -607,6 +608,9 @@ struct DaemonAliasTestsuite
 
     mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject<StrictMock>();
     mpt::MockSettings& mock_settings = *mock_settings_injection.first;
+
+    const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection =
+        mpt::MockPermissionUtils::inject<NiceMock>();
 };
 
 TEST_P(DaemonAliasTestsuite, purge_removes_purged_instance_aliases_and_scripts)

tests/test_client_common.cpp

@@ -22,6 +22,7 @@
 #include "mock_cert_store.h"
 #include "mock_client_rpc.h"
 #include "mock_daemon.h"
+#include "mock_permission_utils.h"
 #include "mock_standard_paths.h"
 #include "mock_utils.h"
 #include "stub_terminal.h"
@@ -60,6 +61,9 @@ struct TestClientCommon : public mpt::DaemonTestFixture
         std::make_unique<NiceMock<mpt::MockCertProvider>>()};
     std::unique_ptr<mpt::MockCertStore> mock_cert_store{std::make_unique<mpt::MockCertStore>()};
 
+    const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection =
+        mpt::MockPermissionUtils::inject<NiceMock>();
+
     const std::string server_address{"localhost:50052"};
     mpt::TempDir temp_dir;
 };