Fix to use mkstemp for temp shadow file creation

Do not rely on randomString() for tempShadowFile, as it uses '/' in random
set, and cause file creation error. Also, it's safe to use mkstemp to create
temp shadow file with random name suffixing shadow file name.

Change-Id: I0b80cc6d7c002e732e22f660e50b0701acac15fe
Signed-off-by: Richard Marian Thomaiyar <richard.marian.thomaiyar@linux.intel.com>

4 files changed, 41 insertions, 36 deletions

diff --git a/file.hpp b/file.hpp
index ae5cf85..7b22b3e 100644
--- a/file.hpp
+++ b/file.hpp
@@ -49,6 +49,23 @@ class File
             fp = fopen(name.c_str(), mode.c_str());
         }
 
+        /** @brief Opens file using provided file descriptor
+         *
+         *  @param[in] fd           - File descriptor
+         *  @param[in] name         - File name
+         *  @param[in] mode         - File open mode
+         *  @param[in] removeOnExit - File to be removed at exit or no
+         */
+        File(int fd,
+             const std::string& name,
+             const std::string& mode,
+             bool removeOnExit = false) :
+            name(name),
+            removeOnExit(removeOnExit)
+        {
+            fp = fdopen(fd, mode.c_str());
+        }
+
         ~File()
         {
             if (fp)
diff --git a/test/utest.cpp b/test/utest.cpp
index bdca968..33e0576 100644
--- a/test/utest.cpp
+++ b/test/utest.cpp
@@ -14,7 +14,6 @@ namespace fs = std::experimental::filesystem;
 
 constexpr auto path = "/dummy/user";
 constexpr auto testShadow = "/tmp/__tshadow__";
-constexpr auto shadowCopy = "/tmp/__tshadowCopy__";
 constexpr auto shadowCompare = "/tmp/__tshadowCompare__";
 
 // New password
@@ -64,11 +63,6 @@ class UserTest : public ::testing::Test
                 fs::remove(testShadow);
             }
 
-            if (fs::exists(shadowCopy))
-            {
-                fs::remove(shadowCopy);
-            }
-
             if (fs::exists(shadowCompare))
             {
                 fs::remove(shadowCompare);
@@ -103,8 +97,7 @@ class UserTest : public ::testing::Test
         /** @brief Applies the new password */
         auto applyPassword()
         {
-            return user.applyPassword(testShadow, shadowCopy,
-                                      password, salt);
+            return user.applyPassword(testShadow, password, salt);
         }
 };
 
@@ -168,22 +161,6 @@ TEST_F(UserTest, applyPassword)
     EXPECT_EQ(shadowEntry, shadowCompareEntry);
 }
 
-/** @brief Verifies the shadow copy file is removed
- */
-TEST_F(UserTest, checkShadowCopyRemove)
-{
-    // Update the password so that the temp file is in action
-    applyPassword();
-
-    // Compare the permission of 2 files
-    struct stat shadow{};
-    struct stat temp{};
-
-    stat(testShadow, &shadow);
-    stat(shadowCopy, &temp);
-    EXPECT_EQ(false, fs::exists(shadowCopy));
-}
-
 /** @brief Verifies the permissions are correct
  */
 TEST_F(UserTest, verifyShadowPermission)
diff --git a/user.cpp b/user.cpp
index f92fe10..447bfcd 100644
--- a/user.cpp
+++ b/user.cpp
@@ -58,22 +58,16 @@ void User::setPassword(std::string newPassword)
     salt.resize(SALT_LENGTH);
     salt = randomString(SALT_LENGTH);
 
-    auto tempShadowFile = std::string("/etc/__") +
-                          randomString(SALT_LENGTH) +
-                          std::string("__");
-
     // Apply the change. Updates could be directly made to shadow
     // but then any update must be contained within the boundary
     // of that user, else it would run into next entry and thus
     // corrupting it. Classic example is when a password is set on
     // a user ID that does not have a prior password
-    applyPassword(SHADOW_FILE, tempShadowFile,
-                  newPassword, salt);
+    applyPassword(SHADOW_FILE, newPassword, salt);
     return;
 }
 
 void User::applyPassword(const std::string& shadowFile,
-                         const std::string& tempFile,
                          const std::string& password,
                          const std::string& salt)
 {
@@ -91,14 +85,30 @@ void User::applyPassword(const std::string& shadowFile,
         return raiseException(errno, "Error opening shadow file");
     }
 
-    // Open the temp shadow file for writing
+    // open temp shadow file, by suffixing random name in shadow file name.
+    std::vector<char> tempFileName(shadowFile.begin(), shadowFile.end());
+    std::vector<char> fileTemplate = {
+                             '_', '_', 'X', 'X', 'X', 'X', 'X', 'X', '\0' };
+    tempFileName.insert(
+              tempFileName.end(), fileTemplate.begin(), fileTemplate.end());
+
+    int fd = mkstemp(tempFileName.data());
+    if (fd == -1)
+    {
+        return raiseException(errno, "Error creating temp shadow file");
+    }
+
+    std::string strTempFileName(tempFileName.data());
+    // Open the temp shadow file for writing from provided fd
     // By "true", remove it at exit if still there.
     // This is needed to cleanup the temp file at exception
-    phosphor::user::File temp(tempFile, "w", true);
+    phosphor::user::File temp(fd, strTempFileName, "w", true);
     if ((temp)() == NULL)
     {
+        close(fd);
         return raiseException(errno, "Error opening temp shadow file");
     }
+    fd = -1; // don't use fd anymore, as the File object owns it
 
     // Change the permission of this new temp file
     // to be same as shadow so that it's secure
@@ -154,9 +164,12 @@ void User::applyPassword(const std::string& shadowFile,
 
     // Done
     endspent();
+    // flush contents to file first, before renaming to avoid
+    // corruption during power failure
+    fflush((temp)());
 
     // Everything must be fine at this point
-    fs::rename(tempFile, shadowFile);
+    fs::rename(strTempFileName, shadowFile);
     return;
 }
 
diff --git a/user.hpp b/user.hpp
index c05cacc..ca8673f 100644
--- a/user.hpp
+++ b/user.hpp
@@ -113,12 +113,10 @@ class User : public Interface
          *         Writes shadow entries into a temp file
          *
          *  @param[in] shadowFile - shadow password file
-         *  @param[in] tempFile   - Temporary file
          *  @param[in] password   - clear text password
          *  @param[in] salt       - salt
          */
         void applyPassword(const std::string& shadowFile,
-                           const std::string& tempFile,
                            const std::string& password,
                            const std::string& salt);