Use SecureString where there is data to be cleansed
SecureString has quite a few places it should be used in the user
management code.
Tested: ran set password, test password, and other commands
Change-Id: Ia53bc914d25f7965c3e72d5cf18346e0fa9339b9
Signed-off-by: Vernon Mauery <vernon.mauery@linux.intel.com>
diff --git a/user_channel/passwd_mgr.cpp b/user_channel/passwd_mgr.cpp
index c5323fe..9b232b5 100644
--- a/user_channel/passwd_mgr.cpp
+++ b/user_channel/passwd_mgr.cpp
@@ -87,13 +87,13 @@
}
}
-std::string PasswdMgr::getPasswdByUserName(const std::string& userName)
+SecureString PasswdMgr::getPasswdByUserName(const std::string& userName)
{
checkAndReload();
auto iter = passwdMapList.find(userName);
if (iter == passwdMapList.end())
{
- return std::string();
+ return SecureString();
}
return iter->second;
}
@@ -235,7 +235,7 @@
void PasswdMgr::initPasswordMap(void)
{
// TODO phosphor-host-ipmid#170 phosphor::user::shadow::Lock lock{};
- std::vector<uint8_t> dataBuf;
+ SecureString dataBuf;
if (readPasswdFileData(dataBuf) != 0)
{
@@ -246,14 +246,14 @@
if (dataBuf.size() != 0)
{
// populate the user list with password
- char* outPtr = reinterpret_cast<char*>(dataBuf.data());
+ char* outPtr = dataBuf.data();
char* nToken = NULL;
char* linePtr = strtok_r(outPtr, "\n", &nToken);
size_t lineSize = 0;
while (linePtr != NULL)
{
size_t userEPos = 0;
- std::string lineStr(linePtr);
+ SecureString lineStr(linePtr);
if ((userEPos = lineStr.find(":")) != std::string::npos)
{
lineSize = lineStr.size();
@@ -267,12 +267,10 @@
// Update the timestamp
fileLastUpdatedTime = getUpdatedFileTime();
- // Clear sensitive data
- OPENSSL_cleanse(dataBuf.data(), dataBuf.size());
return;
}
-int PasswdMgr::readPasswdFileData(std::vector<uint8_t>& outBytes)
+int PasswdMgr::readPasswdFileData(SecureString& outBytes)
{
std::array<uint8_t, maxKeySize> keyBuff;
std::ifstream keyFile(encryptKeyFileName, std::ios::in | std::ios::binary);
@@ -344,10 +342,11 @@
size_t outBytesLen = 0;
// Resize to actual data size
- outBytes.resize(inBytesLen + EVP_MAX_BLOCK_LENGTH);
+ outBytes.resize(inBytesLen + EVP_MAX_BLOCK_LENGTH, '\0');
if (encryptDecryptData(false, EVP_aes_128_cbc(), key.data(), keyLen, iv,
ivLen, inBytes, inBytesLen, mac, &macLen,
- outBytes.data(), &outBytesLen) != 0)
+ reinterpret_cast<unsigned char*>(outBytes.data()),
+ &outBytesLen) != 0)
{
log<level::DEBUG>("Error in decryption");
return -EIO;
@@ -370,7 +369,7 @@
size_t inBytesLen = 0;
size_t isUsrFound = false;
const EVP_CIPHER* cipher = EVP_aes_128_cbc();
- std::vector<uint8_t> dataBuf;
+ SecureString dataBuf;
// Read the encrypted file and get the file data
// Check user existance and return if not exist.
@@ -386,7 +385,7 @@
dataBuf.size() + newUserName.size() + EVP_CIPHER_block_size(cipher);
}
- std::vector<uint8_t> inBytes(inBytesLen);
+ SecureString inBytes(inBytesLen, '\0');
if (inBytesLen != 0)
{
char* outPtr = reinterpret_cast<char*>(dataBuf.data());
@@ -396,7 +395,7 @@
{
size_t userEPos = 0;
- std::string lineStr(linePtr);
+ SecureString lineStr(linePtr);
if ((userEPos = lineStr.find(":")) != std::string::npos)
{
if (userName.compare(lineStr.substr(0, userEPos)) == 0)
@@ -405,7 +404,7 @@
if (!newUserName.empty())
{
bytesWritten += std::snprintf(
- reinterpret_cast<char*>(&inBytes[0]) + bytesWritten,
+ &inBytes[0] + bytesWritten,
(inBytesLen - bytesWritten), "%s%s\n",
newUserName.c_str(),
lineStr.substr(userEPos, lineStr.size()).data());
@@ -413,9 +412,9 @@
}
else
{
- bytesWritten += std::snprintf(
- reinterpret_cast<char*>(&inBytes[0]) + bytesWritten,
- (inBytesLen - bytesWritten), "%s\n", lineStr.data());
+ bytesWritten += std::snprintf(&inBytes[0] + bytesWritten,
+ (inBytesLen - bytesWritten),
+ "%s\n", lineStr.data());
}
}
linePtr = strtok_r(NULL, "\n", &nToken);
@@ -522,10 +521,10 @@
size_t outBytesLen = 0;
if (inBytesLen != 0)
{
- if (encryptDecryptData(true, EVP_aes_128_cbc(), key.data(), keyLen,
- iv.data(), ivLen, inBytes.data(), inBytesLen,
- mac.data(), &macLen, outBytes.data(),
- &outBytesLen) != 0)
+ if (encryptDecryptData(
+ true, EVP_aes_128_cbc(), key.data(), keyLen, iv.data(), ivLen,
+ reinterpret_cast<unsigned char*>(inBytes.data()), inBytesLen,
+ mac.data(), &macLen, outBytes.data(), &outBytesLen) != 0)
{
log<level::DEBUG>("Error while encrypting the data");
return -EIO;
diff --git a/user_channel/passwd_mgr.hpp b/user_channel/passwd_mgr.hpp
index 4c94480..8704f83 100644
--- a/user_channel/passwd_mgr.hpp
+++ b/user_channel/passwd_mgr.hpp
@@ -17,6 +17,7 @@
#include <openssl/evp.h>
#include <ctime>
+#include <ipmid/types.hpp>
#include <string>
#include <unordered_map>
#include <vector>
@@ -45,7 +46,7 @@
* @return password string. will return empty string, if unable to locate
* the user
*/
- std::string getPasswdByUserName(const std::string& userName);
+ SecureString getPasswdByUserName(const std::string& userName);
/** @brief Update / clear username and password entry for the specified
* user
@@ -61,7 +62,7 @@
private:
using UserName = std::string;
- using Password = std::string;
+ using Password = SecureString;
std::unordered_map<UserName, Password> passwdMapList;
std::time_t fileLastUpdatedTime;
@@ -87,7 +88,7 @@
*
* @return error response
*/
- int readPasswdFileData(std::vector<uint8_t>& outBytes);
+ int readPasswdFileData(SecureString& outBytes);
/** @brief Updates special password file by clearing the password entry
* for the user specified.
*
diff --git a/user_channel/user_layer.cpp b/user_channel/user_layer.cpp
index df04dc5..bb9ac38 100644
--- a/user_channel/user_layer.cpp
+++ b/user_channel/user_layer.cpp
@@ -33,7 +33,7 @@
return ccSuccess;
}
-std::string ipmiUserGetPassword(const std::string& userName)
+SecureString ipmiUserGetPassword(const std::string& userName)
{
return passwdMgr.getPasswdByUserName(userName);
}
@@ -95,7 +95,7 @@
}
Cc ipmiSetSpecialUserPassword(const std::string& userName,
- const std::string& userPassword)
+ const SecureString& userPassword)
{
return getUserAccessObject().setSpecialUserPassword(userName, userPassword);
}
diff --git a/user_channel/user_layer.hpp b/user_channel/user_layer.hpp
index 01b6634..82e0d31 100644
--- a/user_channel/user_layer.hpp
+++ b/user_channel/user_layer.hpp
@@ -17,6 +17,7 @@
#include <bitset>
#include <ipmid/api.hpp>
+#include <ipmid/types.hpp>
#include <string>
namespace ipmi
@@ -88,7 +89,7 @@
*
* @return password or empty string
*/
-std::string ipmiUserGetPassword(const std::string& userName);
+SecureString ipmiUserGetPassword(const std::string& userName);
/** @brief The IPMI call to clear password entry associated with specified
* username
@@ -166,7 +167,7 @@
* @return ccSuccess for success, others for failure.
*/
Cc ipmiSetSpecialUserPassword(const std::string& userName,
- const std::string& userPassword);
+ const SecureString& userPassword);
/** @brief get user name
*
diff --git a/user_channel/user_mgmt.cpp b/user_channel/user_mgmt.cpp
index 182d17d..452be90 100644
--- a/user_channel/user_mgmt.cpp
+++ b/user_channel/user_mgmt.cpp
@@ -751,7 +751,7 @@
}
Cc UserAccess::setSpecialUserPassword(const std::string& userName,
- const std::string& userPassword)
+ const SecureString& userPassword)
{
if (pamUpdatePasswd(userName.c_str(), userPassword.c_str()) != PAM_SUCCESS)
{
diff --git a/user_channel/user_mgmt.hpp b/user_channel/user_mgmt.hpp
index 6dbda25..833f6a1 100644
--- a/user_channel/user_mgmt.hpp
+++ b/user_channel/user_mgmt.hpp
@@ -241,7 +241,7 @@
* @return ccSuccess for success, others for failure.
*/
Cc setSpecialUserPassword(const std::string& userName,
- const std::string& userPassword);
+ const SecureString& userPassword);
/** @brief to set user privilege and access details
*
diff --git a/user_channel/usercommands.cpp b/user_channel/usercommands.cpp
index c48c476..f6190d7 100644
--- a/user_channel/usercommands.cpp
+++ b/user_channel/usercommands.cpp
@@ -358,28 +358,33 @@
}
else if (req->operation == testPassword)
{
- auto password = ipmiUserGetPassword(userName);
- std::string testPassword(
- reinterpret_cast<const char*>(req->userPassword), 0,
- passwordLength);
- // Note: For security reasons password size won't be compared and
- // wrong password size completion code will not be returned if size
- // doesn't match as specified in IPMI specification.
- if (password != testPassword)
+ SecureString password = ipmiUserGetPassword(userName);
+ SecureString testPassword(
+ reinterpret_cast<const char*>(req->userPassword), passwordLength);
+ // constant time string compare: always compare exactly as many bytes
+ // as the length of the input, resizing the actual password to match,
+ // maintaining a knowledge if the sizes differed originally
+ static const std::array<char, maxIpmi20PasswordSize> empty = {'\0'};
+ size_t cmpLen = testPassword.size();
+ bool pwLenDiffers = password.size() != cmpLen;
+ const char* cmpPassword = nullptr;
+ if (pwLenDiffers)
+ {
+ cmpPassword = empty.data();
+ }
+ else
+ {
+ cmpPassword = password.data();
+ }
+ bool pwBad = CRYPTO_memcmp(cmpPassword, testPassword.data(), cmpLen);
+ pwBad |= pwLenDiffers;
+ if (pwBad)
{
log<level::DEBUG>("Test password failed",
entry("USER-ID=%d", (uint8_t)req->userId));
- // Clear sensitive data
- OPENSSL_cleanse(testPassword.data(), testPassword.length());
- OPENSSL_cleanse(password.data(), password.length());
-
return static_cast<Cc>(
IPMISetPasswordReturnCodes::ipmiCCPasswdFailMismatch);
}
- // Clear sensitive data
- OPENSSL_cleanse(testPassword.data(), testPassword.length());
- OPENSSL_cleanse(password.data(), password.length());
-
return ccSuccess;
}
return ccInvalidFieldRequest;