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>
diff --git a/file.hpp b/file.hpp
index ae5cf85..7b22b3e 100644
--- a/file.hpp
+++ b/file.hpp
@@ -49,6 +49,23 @@
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 @@
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 @@
fs::remove(testShadow);
}
- if (fs::exists(shadowCopy))
- {
- fs::remove(shadowCopy);
- }
-
if (fs::exists(shadowCompare))
{
fs::remove(shadowCompare);
@@ -103,8 +97,7 @@
/** @brief Applies the new password */
auto applyPassword()
{
- return user.applyPassword(testShadow, shadowCopy,
- password, salt);
+ return user.applyPassword(testShadow, password, salt);
}
};
@@ -168,22 +161,6 @@
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 @@
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 @@
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 @@
// 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 @@
* 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);