Fix issue with permissions when copying dir contents

[Bug]           Permission denied error when trying to copy read-only directories recursively.
[Cause]         boost::filesystem::copy applied permissions immediately, which in some cases caused
                error when trying to write something inside processed directory.
[Solution]      Instead of using boost::filesystem::copy on directories, split action into three
                sub-actions:
                  * Create new directory with boost::filesystem::create_directory
                  * Call copyDirContentsRec() to copy contents of processed directory
                  * Apply source directory permissions and ownership
[Verification]  Build, install, run tests.

Change-Id: Ifdec110a595dcecd113abf4065dd1cdc03f2d3cb

diff --git a/common/utils/fs.cpp b/common/utils/fs.cpp
index a03c139..8a6441c 100644 (file)
--- a/common/utils/fs.cpp
+++ b/common/utils/fs.cpp
@@ -208,38 +208,56 @@ bool copyDirContentsRec(const boost::filesystem::path& src, const boost::filesys
     namespace fs = boost::filesystem;
 
     // TODO: Right now this function skips files which produce error when copying. Errors show up
-    // when:
-    //   a) fs::directory_iterator file(src) is created
-    //   b) fs::copy(...) is called
-    // In both cases lack of permissions is the issue.
+    // when fs::directory_iterator file(src) is created - lack of permissions is the issue.
     //
-    // In a) case we can't do much - SCS won't be able to read the directory and its contents. Such
-    // directories are not common in the filesystem, so they *probably* can be skipped.
-    //
-    // In b) case multiple directories have too strict permissions to be directly copied. This
-    // is a problem for some files crucial to container launch (ex. we cannot copy
-    // /usr/lib/systemd/systemd because /usr/lib has 555 permissions).
-    // To fix b) issue, copying must be done in two steps:
-    //   1. Copy file contents without permissions (this probably can be achieved by opening two
-    //      files in-code with fstream and programatically copying data from one file to another).
-    //   2. Apply all available file attributes from source (permissions, owner UID/GID, xattrs...)
+    // To fix lack of permissions, copying must be done as root. The easiest way would be to launch
+    // copyDirContents after fork() and setuid(0).
 
     try {
         for (fs::directory_iterator file(src);
              file != fs::directory_iterator();
              ++file) {
             fs::path current(file->path());
+            fs::path destination = dst / current.filename();
 
             boost::system::error_code ec;
-            fs::copy(current, dst / current.filename(), ec);
-            if(ec.value() != boost::system::errc::success) {
+
+            if (!fs::is_symlink(current) && fs::is_directory(current)) {
+                fs::create_directory(destination, ec);
+            } else {
+                fs::copy(current, destination, ec);
+            }
+
+            if (ec.value() != boost::system::errc::success) {
                 LOGW("Failed to copy " << current << ": " << ec.message());
+                continue;
             }
 
             if (!fs::is_symlink(current) && fs::is_directory(current)) {
-                if (!copyDirContentsRec(current, dst / current.filename())) {
+                if (!copyDirContentsRec(current, destination)) {
                     return false;
                 }
+
+                // apply permissions coming from source file/directory
+                fs::file_status stat = status(current);
+                fs::permissions(destination, stat.permissions(), ec);
+
+                if (ec.value() != boost::system::errc::success) {
+                    LOGW("Failed to set permissions for " << destination << ": " << ec.message());
+                }
+            }
+
+            // change owner
+            struct stat info;
+            ::stat(current.string().c_str(), &info);
+            if (fs::is_symlink(destination)) {
+                if (::lchown(destination.string().c_str(), info.st_uid, info.st_gid) < 0) {
+                    LOGW("Failed to change owner of symlink " << destination.string() << ": " << strerror(errno));
+                }
+            } else {
+                if (::chown(destination.string().c_str(), info.st_uid, info.st_gid) < 0) {
+                    LOGW("Failed to change owner of file " << destination.string() << ": " << strerror(errno));
+                }
             }
         }
     } catch (fs::filesystem_error& e) {

diff --git a/server/server.cpp b/server/server.cpp
index a9ad444..7b03f4c 100644 (file)
--- a/server/server.cpp
+++ b/server/server.cpp
@@ -202,9 +202,12 @@ bool Server::prepareEnvironment(const std::string& configPath, bool runAsRoot)
     // is introduced. The capability is needed to allow modify SMACK labels of
     // "/var/run/containers/<container>/run" mount point.
     // CAP_SYS_TTY_CONFIG is needed to activate virtual terminals through ioctl calls
+    // CAP_CHOWN is needed when creating new container from image to set owner/group for each file,
+    // directory or symlink
     return (runAsRoot || utils::dropRoot(uid, gid, {CAP_SYS_ADMIN,
                                                     CAP_MAC_OVERRIDE,
-                                                    CAP_SYS_TTY_CONFIG}));
+                                                    CAP_SYS_TTY_CONFIG,
+                                                    CAP_CHOWN}));
 }
 
 

diff --git a/tests/unit_tests/utils/ut-fs.cpp b/tests/unit_tests/utils/ut-fs.cpp
index 43be58a..a1eea40 100644 (file)
--- a/tests/unit_tests/utils/ut-fs.cpp
+++ b/tests/unit_tests/utils/ut-fs.cpp
@@ -64,6 +64,8 @@ const std::string FILE_DIR_RANDOM_2 =
     boost::filesystem::unique_path("testDir-%%%%").string();
 const std::string FILE_DIR_RANDOM_3 =
     boost::filesystem::unique_path("testDir-%%%%").string();
+const std::string FILE_DIR_RANDOM_4 =
+    boost::filesystem::unique_path("testDir-%%%%").string();
 const std::string FILE_NAME_RANDOM_1 =
     boost::filesystem::unique_path("testFile-%%%%").string();
 const std::string FILE_NAME_RANDOM_2 =
@@ -146,25 +148,47 @@ BOOST_AUTO_TEST_CASE(MoveFileTest)
 BOOST_AUTO_TEST_CASE(CopyDirContentsTest)
 {
     namespace fs = boost::filesystem;
-    std::string src, src_inner, dst, dst_inner;
+    std::string src, src_inner, src_inner2, dst, dst_inner, dst_inner2;
     boost::system::error_code ec;
 
     src = TMP_PATH + "/" + FILE_DIR_RANDOM_1;
     src_inner = src + "/" + FILE_DIR_RANDOM_3;
+    src_inner2 = src + "/" + FILE_DIR_RANDOM_4;
 
     dst = TMP_PATH + "/" + FILE_DIR_RANDOM_2;
     dst_inner = dst + "/" + FILE_DIR_RANDOM_3;
+    dst_inner2 = dst + "/" + FILE_DIR_RANDOM_4;
+
+    // template dir structure:
+    // |-src
+    //    |-FILE_NAME_RANDOM_1
+    //    |-FILE_NAME_RANDOM_2
+    //    |-src_inner (rw directory)
+    //    |  |-FILE_NAME_RANDOM_1
+    //    |
+    //    |-src_inner2 (ro directory)
+    //       |-FILE_NAME_RANDOM_1
+    //       |-FILE_NAME_RANDOM_2
 
     // create entire structure with files
     BOOST_REQUIRE(fs::create_directory(src, ec));
     BOOST_REQUIRE(ec.value() == 0);
     BOOST_REQUIRE(fs::create_directory(src_inner, ec));
     BOOST_REQUIRE(ec.value() == 0);
+    BOOST_REQUIRE(fs::create_directory(src_inner2, ec));
+    BOOST_REQUIRE(ec.value() == 0);
 
     BOOST_REQUIRE(saveFileContent(src + "/" + FILE_NAME_RANDOM_1, FILE_CONTENT));
     BOOST_REQUIRE(saveFileContent(src + "/" + FILE_NAME_RANDOM_2, FILE_CONTENT_2));
     BOOST_REQUIRE(saveFileContent(src_inner + "/" + FILE_NAME_RANDOM_1, FILE_CONTENT_3));
+    BOOST_REQUIRE(saveFileContent(src_inner2 + "/" + FILE_NAME_RANDOM_1, FILE_CONTENT_3));
+    BOOST_REQUIRE(saveFileContent(src_inner2 + "/" + FILE_NAME_RANDOM_2, FILE_CONTENT_2));
 
+    // change permissions of src_inner2 directory
+    fs::permissions(src_inner2, fs::owner_read, ec);
+    BOOST_REQUIRE(ec.value() == 0);
+
+    // create dst directory
     BOOST_REQUIRE(fs::create_directory(dst, ec));
     BOOST_REQUIRE(ec.value() == 0);
 
@@ -176,10 +200,19 @@ BOOST_AUTO_TEST_CASE(CopyDirContentsTest)
     BOOST_CHECK(fs::exists(dst + "/" + FILE_NAME_RANDOM_2));
     BOOST_CHECK(fs::exists(dst_inner));
     BOOST_CHECK(fs::exists(dst_inner + "/" + FILE_NAME_RANDOM_1));
+    BOOST_CHECK(fs::exists(dst_inner2));
+    BOOST_CHECK(fs::exists(dst_inner2 + "/" + FILE_NAME_RANDOM_1));
+    BOOST_CHECK(fs::exists(dst_inner2 + "/" + FILE_NAME_RANDOM_2));
 
     BOOST_CHECK_EQUAL(readFileContent(dst + "/" + FILE_NAME_RANDOM_1), FILE_CONTENT);
     BOOST_CHECK_EQUAL(readFileContent(dst + "/" + FILE_NAME_RANDOM_2), FILE_CONTENT_2);
     BOOST_CHECK_EQUAL(readFileContent(dst_inner + "/" + FILE_NAME_RANDOM_1), FILE_CONTENT_3);
+    BOOST_CHECK_EQUAL(readFileContent(dst_inner2 + "/" + FILE_NAME_RANDOM_1), FILE_CONTENT_3);
+    BOOST_CHECK_EQUAL(readFileContent(dst_inner2 + "/" + FILE_NAME_RANDOM_2), FILE_CONTENT_2);
+
+    fs::file_status st;
+    BOOST_REQUIRE_NO_THROW(st = fs::status(fs::path(dst_inner2)));
+    BOOST_CHECK(fs::owner_read == st.permissions());
 }
 
 BOOST_AUTO_TEST_SUITE_END()