From 1f5a002f4b596522cf46eaad3cfd072a44b5848d Mon Sep 17 00:00:00 2001 From: Richard Marian Thomaiyar Date: Sat, 16 Dec 2017 15:11:47 +0530 Subject: 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 --- file.hpp | 17 +++++++++++++++++ test/utest.cpp | 25 +------------------------ user.cpp | 33 +++++++++++++++++++++++---------- user.hpp | 2 -- 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 tempFileName(shadowFile.begin(), shadowFile.end()); + std::vector 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); -- cgit v1.2.1