Fix handling of 16-byte IPMI username without null terminator
The IPMI spec defines username as a fixed 16-byte field,
which may not contain a null terminator if the name length
is exactly 16 characters.
Several places in the code assumed C-string semantics
(null terminated), causing incorrect user data reading and
comparisons.
This change:
1. Replaces incorrect
std::string(const char *, size_t pos, size_t count),
which does not exist, with length-safe
strnlen + std::string(char *, size_t count).
These are the relevant constructors exist:
std::string(const char* s, size_t count)
std::string(const std::string& str, size_t pos, size_t count).
The old code used the second constructor with char *,
which led to unexpected behaviors.
2. Replaces strncpy and unsafe std::fill casts with
explicit zero padding memset and memcpy.
3. Ensures reads stop at the first null or take the full
16 bytes in case there is no null.
This ensures storage, retrieval and comparison of all valid
IPMI usernames, including the edge case of exactly 16
characters with no null terminator.
Tested on bluefield BMC platform:
```
root@dpu-bmc:~# ipmitool user set name 10 firmware_updater
root@dpu-bmc:~# ipmitool user list 1
ID Name Call Link Auth IPMI Msg Channel Priv Limit
1 root false true true ADMINISTRATOR
2 true false false NO ACCESS
3 true false false NO ACCESS
4 true false false NO ACCESS
5 true false false NO ACCESS
6 true false false NO ACCESS
7 true false false NO ACCESS
8 true false false NO ACCESS
9 true false false NO ACCESS
10 firmware_updater true false false USER
11 true false false NO ACCESS
12 true false false NO ACCESS
13 true false false NO ACCESS
14 true false false NO ACCESS
15 true false false NO ACCESS
root@dpu-bmc:~# ipmitool user set name 10 ""
root@dpu-bmc:~# ipmitool user list 1
ID Name Call Link Auth IPMI Msg Channel Priv Limit
1 root false true true ADMINISTRATOR
2 true false false NO ACCESS
3 true false false NO ACCESS
4 true false false NO ACCESS
5 true false false NO ACCESS
6 true false false NO ACCESS
7 true false false NO ACCESS
8 true false false NO ACCESS
9 true false false NO ACCESS
10 true false false NO ACCESS
11 true false false NO ACCESS
12 true false false NO ACCESS
13 true false false NO ACCESS
14 true false false NO ACCESS
15 true false false NO ACCESS
root@dpu-bmc:~# ipmitool user set name 4 testuser
root@dpu-bmc:~# ipmitool user set password 4 "Aa1234567890!!"
Set User Password command successful (user 4)
root@dpu-bmc:~# ipmitool channel setaccess 1 4 link=on ipmi=on privilege=4
Set User Access (channel 1 id 4) successful.
root@dpu-bmc:~# ipmitool user enable 4
root@dpu-bmc:~# ipmitool channel getaccess 1 4
Maximum User IDs : 15
Enabled User IDs : 2
User ID : 4
User Name : testuser
Fixed Name : No
Access Available : callback
Link Authentication : enabled
IPMI Messaging : enabled
Privilege Level : ADMINISTRATOR
Enable Status : enabled
On host:
root@ldev-platform-12:~# ipmitool -I lanplus -H <bmc_ip> -U testuser -P 'Aa1234567890!!' -C 17 power status
Chassis Power is on
```
Change-Id: Ib678bdcca1d88289d5c8ce414876563d5fead4ad
Signed-off-by: Orit Kashany <okashany@nvidia.com>
diff --git a/user_channel/user_layer.cpp b/user_channel/user_layer.cpp
index ee147d8..ff1f965 100644
--- a/user_channel/user_layer.cpp
+++ b/user_channel/user_layer.cpp
@@ -74,13 +74,16 @@
Cc ipmiUserSetUserName(const uint8_t userId, const char* userName)
{
- std::string newUser(userName, 0, ipmiMaxUserName);
+ size_t len = strnlen(userName, ipmiMaxUserName);
+ std::string newUser(userName, len);
return getUserAccessObject().setUserName(userId, newUser);
}
Cc ipmiUserSetUserName(const uint8_t userId, const std::string& userName)
{
- std::string newUser(userName, 0, ipmiMaxUserName);
+ size_t len =
+ std::min(userName.size(), static_cast<size_t>(ipmiMaxUserName));
+ std::string newUser(userName, 0, len);
return getUserAccessObject().setUserName(userId, newUser);
}
diff --git a/user_channel/user_mgmt.cpp b/user_channel/user_mgmt.cpp
index 8c5a1bf..bf45db9 100644
--- a/user_channel/user_mgmt.cpp
+++ b/user_channel/user_mgmt.cpp
@@ -33,7 +33,9 @@
#include <xyz/openbmc_project/Common/error.hpp>
#include <xyz/openbmc_project/User/Common/error.hpp>
+#include <algorithm>
#include <cerrno>
+#include <cstring>
#include <fstream>
#include <regex>
#include <variant>
@@ -173,9 +175,8 @@
size_t usrIndex = 1;
for (; usrIndex <= ipmiMaxUsers; ++usrIndex)
{
- std::string curName(
- reinterpret_cast<char*>(userData->user[usrIndex].userName), 0,
- ipmiMaxUserName);
+ std::string curName =
+ safeUsernameString(userData->user[usrIndex].userName);
if (userName == curName)
{
break; // found the entry
@@ -219,14 +220,9 @@
}
case UserUpdateEvent::userRenamed:
{
- std::fill(
- static_cast<uint8_t*>(userData->user[usrIndex].userName),
- static_cast<uint8_t*>(userData->user[usrIndex].userName) +
- sizeof(userData->user[usrIndex].userName),
- 0);
- std::strncpy(
- reinterpret_cast<char*>(userData->user[usrIndex].userName),
- newUserName.c_str(), ipmiMaxUserName);
+ safeUsernameCopyToBuffer(
+ userData->user[usrIndex].userName,
+ sizeof(userData->user[usrIndex].userName), newUserName);
ipmiRenameUserEntryPassword(userName, newUserName);
break;
}
@@ -717,8 +713,8 @@
}
ipmi::SecureString passwd;
- passwd.assign(reinterpret_cast<const char*>(userPassword), 0,
- maxIpmi20PasswordSize);
+ size_t len = strnlen(userPassword, maxIpmi20PasswordSize);
+ passwd.assign(reinterpret_cast<const char*>(userPassword), len);
int retval = pamUpdatePasswd(userName.c_str(), passwd.c_str());
switch (retval)
@@ -752,8 +748,9 @@
userLock{*userMutex};
UserInfo* userInfo = getUserInfo(userId);
std::string userName;
- userName.assign(reinterpret_cast<char*>(userInfo->userName), 0,
- ipmiMaxUserName);
+
+ safeUsernameAssign(userName, userInfo->userName);
+
if (userName.empty())
{
lg2::debug("User name not set / invalid");
@@ -855,8 +852,7 @@
userLock{*userMutex};
UserInfo* userInfo = getUserInfo(userId);
std::string userName;
- userName.assign(reinterpret_cast<char*>(userInfo->userName), 0,
- ipmiMaxUserName);
+ safeUsernameAssign(userName, userInfo->userName);
if (userName.empty())
{
lg2::debug("User name not set / invalid");
@@ -905,9 +901,8 @@
size_t usrIndex = 1;
for (; usrIndex <= ipmiMaxUsers; ++usrIndex)
{
- std::string curName(
- reinterpret_cast<char*>(usersTbl.user[usrIndex].userName), 0,
- ipmiMaxUserName);
+ std::string curName =
+ safeUsernameString(usersTbl.user[usrIndex].userName);
if (userName == curName)
{
break; // found the entry
@@ -929,8 +924,7 @@
return ccParmOutOfRange;
}
UserInfo* userInfo = getUserInfo(userId);
- userName.assign(reinterpret_cast<char*>(userInfo->userName), 0,
- ipmiMaxUserName);
+ safeUsernameAssign(userName, userInfo->userName);
return ccSuccess;
}
@@ -1018,9 +1012,9 @@
return ccUnspecifiedError;
}
- std::memset(userInfo->userName, 0, sizeof(userInfo->userName));
- std::memcpy(userInfo->userName,
- static_cast<const void*>(userName.data()), userName.size());
+ safeUsernameCopyToBuffer(userInfo->userName, sizeof(userInfo->userName),
+ userName);
+
userInfo->userInSystem = true;
for (size_t chIndex = 0; chIndex < ipmiMaxChannels; chIndex++)
{
@@ -1045,14 +1039,9 @@
renameUserMethod, "PATH", userMgrObjBasePath);
return ccUnspecifiedError;
}
- std::fill(static_cast<uint8_t*>(userInfo->userName),
- static_cast<uint8_t*>(userInfo->userName) +
- sizeof(userInfo->userName),
- 0);
- std::memset(userInfo->userName, 0, sizeof(userInfo->userName));
- std::memcpy(userInfo->userName,
- static_cast<const void*>(userName.data()), userName.size());
+ safeUsernameCopyToBuffer(userInfo->userName, sizeof(userInfo->userName),
+ userName);
ipmiRenameUserEntryPassword(oldUser, userName);
userInfo->userInSystem = true;
@@ -1207,8 +1196,11 @@
"Corrupted IPMI user data file - invalid user info");
}
std::string userName = userInfo[jsonUserName].get<std::string>();
- std::strncpy(reinterpret_cast<char*>(usersTbl.user[usrIndex].userName),
- userName.c_str(), ipmiMaxUserName);
+
+ // Fixed-width username buffer in struct
+ safeUsernameCopyToBuffer(usersTbl.user[usrIndex].userName,
+ sizeof(usersTbl.user[usrIndex].userName),
+ userName);
std::vector<std::string> privilege =
userInfo[jsonPriv].get<std::vector<std::string>>();
@@ -1309,9 +1301,8 @@
for (size_t usrIndex = 1; usrIndex <= ipmiMaxUsers; ++usrIndex)
{
Json jsonUserInfo;
- jsonUserInfo[jsonUserName] = std::string(
- reinterpret_cast<char*>(usersTbl.user[usrIndex].userName), 0,
- ipmiMaxUserName);
+ jsonUserInfo[jsonUserName] =
+ safeUsernameString(usersTbl.user[usrIndex].userName);
std::vector<std::string> privilege(ipmiMaxChannels);
std::vector<bool> ipmiEnabled(ipmiMaxChannels);
std::vector<bool> linkAuthEnabled(ipmiMaxChannels);
@@ -1389,9 +1380,8 @@
// user index 0 is reserved, starts with 1
for (size_t usrIndex = 1; usrIndex <= ipmiMaxUsers; ++usrIndex)
{
- std::string curName(
- reinterpret_cast<char*>(userData->user[usrIndex].userName), 0,
- ipmiMaxUserName);
+ std::string curName =
+ safeUsernameString(userData->user[usrIndex].userName);
if (userName == curName)
{
lg2::debug("Username {USER_NAME} exists", "USER_NAME", userName);
@@ -1410,8 +1400,11 @@
lg2::error("No empty slots found");
return false;
}
- std::strncpy(reinterpret_cast<char*>(userData->user[freeIndex].userName),
- userName.c_str(), ipmiMaxUserName);
+
+ safeUsernameCopyToBuffer(userData->user[freeIndex].userName,
+ sizeof(userData->user[freeIndex].userName),
+ userName);
+
uint8_t priv =
static_cast<uint8_t>(UserAccess::convertToIPMIPrivilege(sysPriv)) &
privMask;
@@ -1433,14 +1426,10 @@
{
UsersTbl* userData = getUsersTblPtr();
- std::string userName(
- reinterpret_cast<char*>(userData->user[usrIdx].userName), 0,
- ipmiMaxUserName);
+ std::string userName = safeUsernameString(userData->user[usrIdx].userName);
ipmiClearUserEntryPassword(userName);
- std::fill(static_cast<uint8_t*>(userData->user[usrIdx].userName),
- static_cast<uint8_t*>(userData->user[usrIdx].userName) +
- sizeof(userData->user[usrIdx].userName),
- 0);
+ std::memset(userData->user[usrIdx].userName, 0,
+ sizeof(userData->user[usrIdx].userName));
for (size_t chIndex = 0; chIndex < ipmiMaxChannels; ++chIndex)
{
userData->user[usrIdx].userPrivAccess[chIndex].privilege = privNoAccess;
@@ -1655,9 +1644,8 @@
std::vector<std::string> usrGrps;
std::string usrPriv;
- std::string userName(
- reinterpret_cast<char*>(userData->user[usrIdx].userName), 0,
- ipmiMaxUserName);
+ std::string userName =
+ safeUsernameString(userData->user[usrIdx].userName);
sdbusplus::message::object_path tempUserPath(userObjBasePath);
tempUserPath /= userName;
std::string usersPath(tempUserPath);
diff --git a/user_channel/user_mgmt.hpp b/user_channel/user_mgmt.hpp
index 74166c2..ebfec02 100644
--- a/user_channel/user_mgmt.hpp
+++ b/user_channel/user_mgmt.hpp
@@ -21,13 +21,41 @@
#include <ipmid/api.hpp>
#include <sdbusplus/bus.hpp>
+#include <algorithm>
#include <cstdint>
+#include <cstring>
#include <ctime>
#include <variant>
namespace ipmi
{
+// Inline functions for safe username buffer operations
+inline size_t safeUsernameLength(const uint8_t* buf)
+{
+ return strnlen(reinterpret_cast<const char*>(buf), ipmiMaxUserName);
+}
+
+inline std::string safeUsernameString(const uint8_t* buf)
+{
+ return std::string(reinterpret_cast<const char*>(buf),
+ safeUsernameLength(buf));
+}
+
+inline void safeUsernameAssign(std::string& str, const uint8_t* buf)
+{
+ size_t len = safeUsernameLength(buf);
+ str.assign(reinterpret_cast<const char*>(buf), len);
+}
+
+inline void safeUsernameCopyToBuffer(uint8_t* buf, size_t bufSize,
+ const std::string& str)
+{
+ std::memset(buf, 0, bufSize);
+ const size_t n = std::min(str.size(), bufSize);
+ std::memcpy(buf, str.data(), n);
+}
+
using DbusUserPropVariant =
std::variant<std::vector<std::string>, std::string, bool>;