add size checking for input payload data
verify input data size before accessing to prevent out of bound access.
Tested:
ipmitool with lanplus works same as without the change.
ipmitool -I lanplus -U xx -P xx -H ip -C 17 sol info
ipmitool -I lanplus -U xx -P xx -H ip -C 17 sensor list
ipmitool -I lanplus -U xx -P xx -H ip -C 17 sdr list
ipmitool -I lanplus -U xx -P xx -H ip -C 17 user list 1
Signed-off-by: Zhikui Ren <zhikui.ren@intel.com>
Change-Id: I5025aa2666c8873b7c63f8323a932c0480b59304
diff --git a/command/open_session.cpp b/command/open_session.cpp
index 5bfdce0..9ba00ec 100644
--- a/command/open_session.cpp
+++ b/command/open_session.cpp
@@ -14,10 +14,15 @@
std::vector<uint8_t> openSession(const std::vector<uint8_t>& inPayload,
const message::Handler& handler)
{
-
- std::vector<uint8_t> outPayload(sizeof(OpenSessionResponse));
auto request =
reinterpret_cast<const OpenSessionRequest*>(inPayload.data());
+ if (inPayload.size() != sizeof(*request))
+ {
+ std::vector<uint8_t> errorPayload{IPMI_CC_REQ_DATA_LEN_INVALID};
+ return errorPayload;
+ }
+
+ std::vector<uint8_t> outPayload(sizeof(OpenSessionResponse));
auto response = reinterpret_cast<OpenSessionResponse*>(outPayload.data());
// Per the IPMI Spec, messageTag and remoteConsoleSessionID are always
diff --git a/command/payload_cmds.cpp b/command/payload_cmds.cpp
index c5d64fb..c8e682e 100644
--- a/command/payload_cmds.cpp
+++ b/command/payload_cmds.cpp
@@ -20,19 +20,18 @@
std::vector<uint8_t> activatePayload(const std::vector<uint8_t>& inPayload,
const message::Handler& handler)
{
- std::vector<uint8_t> outPayload(sizeof(ActivatePayloadResponse));
auto request =
reinterpret_cast<const ActivatePayloadRequest*>(inPayload.data());
+ if (inPayload.size() != sizeof(*request))
+ {
+ std::vector<uint8_t> errorPayload{IPMI_CC_REQ_DATA_LEN_INVALID};
+ return errorPayload;
+ }
+
+ std::vector<uint8_t> outPayload(sizeof(ActivatePayloadResponse));
auto response =
reinterpret_cast<ActivatePayloadResponse*>(outPayload.data());
- if (inPayload.size() != sizeof(ActivatePayloadRequest))
- {
- response->completionCode = ipmi::ccReqDataLenInvalid;
- outPayload.resize(sizeof(response->completionCode));
- return outPayload;
- }
-
response->completionCode = IPMI_CC_OK;
// SOL is the payload currently supported for activation.
@@ -113,20 +112,19 @@
std::vector<uint8_t> deactivatePayload(const std::vector<uint8_t>& inPayload,
const message::Handler& handler)
{
- std::vector<uint8_t> outPayload(sizeof(DeactivatePayloadResponse));
auto request =
reinterpret_cast<const DeactivatePayloadRequest*>(inPayload.data());
+ if (inPayload.size() != sizeof(*request))
+ {
+ std::vector<uint8_t> errorPayload{IPMI_CC_REQ_DATA_LEN_INVALID};
+ return errorPayload;
+ }
+
+ std::vector<uint8_t> outPayload(sizeof(DeactivatePayloadResponse));
auto response =
reinterpret_cast<DeactivatePayloadResponse*>(outPayload.data());
-
response->completionCode = IPMI_CC_OK;
- if (inPayload.size() != sizeof(DeactivatePayloadRequest))
- {
- response->completionCode = IPMI_CC_REQ_DATA_LEN_INVALID;
- return outPayload;
- }
-
// SOL is the payload currently supported for deactivation
if (static_cast<uint8_t>(message::PayloadType::SOL) != request->payloadType)
{
@@ -187,9 +185,15 @@
std::vector<uint8_t> getPayloadStatus(const std::vector<uint8_t>& inPayload,
const message::Handler& handler)
{
- std::vector<uint8_t> outPayload(sizeof(GetPayloadStatusResponse));
auto request =
reinterpret_cast<const GetPayloadStatusRequest*>(inPayload.data());
+ if (inPayload.size() != sizeof(*request))
+ {
+ std::vector<uint8_t> errorPayload{IPMI_CC_REQ_DATA_LEN_INVALID};
+ return errorPayload;
+ }
+
+ std::vector<uint8_t> outPayload(sizeof(GetPayloadStatusResponse));
auto response =
reinterpret_cast<GetPayloadStatusResponse*>(outPayload.data());
@@ -215,17 +219,19 @@
std::vector<uint8_t> getPayloadInfo(const std::vector<uint8_t>& inPayload,
const message::Handler& handler)
{
- std::vector<uint8_t> outPayload(sizeof(GetPayloadInfoResponse));
auto request =
reinterpret_cast<const GetPayloadInfoRequest*>(inPayload.data());
+
+ if (inPayload.size() != sizeof(*request))
+ {
+ std::vector<uint8_t> errorPayload{IPMI_CC_REQ_DATA_LEN_INVALID};
+ return errorPayload;
+ }
+
+ std::vector<uint8_t> outPayload(sizeof(GetPayloadInfoResponse));
auto response =
reinterpret_cast<GetPayloadInfoResponse*>(outPayload.data());
- if (inPayload.size() != sizeof(GetPayloadInfoRequest))
- {
- response->completionCode = IPMI_CC_REQ_DATA_LEN_INVALID;
- return outPayload;
- }
// SOL is the payload currently supported for payload status & only one
// instance of SOL is supported.
if (static_cast<uint8_t>(message::PayloadType::SOL) !=
diff --git a/command/rakp12.cpp b/command/rakp12.cpp
index 9a31dfe..099c5dc 100644
--- a/command/rakp12.cpp
+++ b/command/rakp12.cpp
@@ -26,8 +26,15 @@
std::vector<uint8_t> RAKP12(const std::vector<uint8_t>& inPayload,
const message::Handler& handler)
{
- std::vector<uint8_t> outPayload(sizeof(RAKP2response));
auto request = reinterpret_cast<const RAKP1request*>(inPayload.data());
+ // verify inPayload minimum size
+ if (inPayload.size() < (sizeof(*request) - userNameMaxLen))
+ {
+ std::vector<uint8_t> errorPayload{IPMI_CC_REQ_DATA_LEN_INVALID};
+ return errorPayload;
+ }
+
+ std::vector<uint8_t> outPayload(sizeof(RAKP2response));
auto response = reinterpret_cast<RAKP2response*>(outPayload.data());
// Session ID zero is reserved for Session Setup
diff --git a/command/session_cmds.cpp b/command/session_cmds.cpp
index a08e8f2..945d8d9 100644
--- a/command/session_cmds.cpp
+++ b/command/session_cmds.cpp
@@ -17,10 +17,15 @@
setSessionPrivilegeLevel(const std::vector<uint8_t>& inPayload,
const message::Handler& handler)
{
-
- std::vector<uint8_t> outPayload(sizeof(SetSessionPrivLevelResp));
auto request =
reinterpret_cast<const SetSessionPrivLevelReq*>(inPayload.data());
+ if (inPayload.size() != sizeof(*request))
+ {
+ std::vector<uint8_t> errorPayload{IPMI_CC_REQ_DATA_LEN_INVALID};
+ return errorPayload;
+ }
+
+ std::vector<uint8_t> outPayload(sizeof(SetSessionPrivLevelResp));
auto response =
reinterpret_cast<SetSessionPrivLevelResp*>(outPayload.data());
response->completionCode = IPMI_CC_OK;
@@ -207,14 +212,29 @@
std::vector<uint8_t> closeSession(const std::vector<uint8_t>& inPayload,
const message::Handler& handler)
{
- std::vector<uint8_t> outPayload(sizeof(CloseSessionResponse));
+ // minimum inPayload size is reqSessionId (uint32_t)
+ // maximum inPayload size is struct CloseSessionRequest
+ if (inPayload.size() != sizeof(uint32_t) &&
+ inPayload.size() != sizeof(CloseSessionRequest))
+ {
+ std::vector<uint8_t> errorPayload{IPMI_CC_REQ_DATA_LEN_INVALID};
+ return errorPayload;
+ }
+
auto request =
reinterpret_cast<const CloseSessionRequest*>(inPayload.data());
+
+ std::vector<uint8_t> outPayload(sizeof(CloseSessionResponse));
auto response = reinterpret_cast<CloseSessionResponse*>(outPayload.data());
uint32_t reqSessionId = request->sessionID;
uint8_t ipmiNetworkInstance = 0;
uint8_t currentSessionPriv = 0;
- uint8_t reqSessionHandle = request->sessionHandle;
+ uint8_t reqSessionHandle = session::invalidSessionHandle;
+
+ if (inPayload.size() == sizeof(CloseSessionRequest))
+ {
+ reqSessionHandle = request->sessionHandle;
+ }
if (reqSessionId == session::sessionZero &&
reqSessionHandle == session::invalidSessionHandle)
diff --git a/command/sol_cmds.cpp b/command/sol_cmds.cpp
index a8fa410..fda3e91 100644
--- a/command/sol_cmds.cpp
+++ b/command/sol_cmds.cpp
@@ -17,6 +17,12 @@
std::vector<uint8_t> payloadHandler(const std::vector<uint8_t>& inPayload,
const message::Handler& handler)
{
+ // Check inPayload size is at least Payload
+ if (inPayload.size() < sizeof(Payload))
+ {
+ return std::vector<uint8_t>();
+ }
+
auto request = reinterpret_cast<const Payload*>(inPayload.data());
auto solDataSize = inPayload.size() - sizeof(Payload);
diff --git a/crypt_algo.cpp b/crypt_algo.cpp
index bf46c4e..c51465f 100644
--- a/crypt_algo.cpp
+++ b/crypt_algo.cpp
@@ -23,6 +23,14 @@
const size_t sessHeaderLen,
const size_t payloadLen) const
{
+ // verify packet size minimal: sessHeaderLen + payloadLen
+ // and payloadLen is more than AESCBC128ConfHeader
+ if (packet.size() < (sessHeaderLen + payloadLen) ||
+ payloadLen < AESCBC128ConfHeader)
+ {
+ throw std::runtime_error("Invalid data length");
+ }
+
auto plainPayload =
decryptData(packet.data() + sessHeaderLen,
packet.data() + sessHeaderLen + AESCBC128ConfHeader,
diff --git a/message_parsers.cpp b/message_parsers.cpp
index 95fcfaf..6703fc3 100644
--- a/message_parsers.cpp
+++ b/message_parsers.cpp
@@ -82,7 +82,6 @@
std::shared_ptr<Message> unflatten(std::vector<uint8_t>& inPacket)
{
- // Check if the packet has atleast the Session Header
if (inPacket.size() < sizeof(SessionHeader_t))
{
throw std::runtime_error("IPMI1.5 Session Header Missing");
@@ -100,6 +99,13 @@
auto payloadLen = header->payloadLength;
+ // Confirm the number of data bytes received correlates to
+ // the packet length in the header
+ if (inPacket.size() < (sizeof(SessionHeader_t) + payloadLen))
+ {
+ throw std::runtime_error("Invalid data length");
+ }
+
(message->payload)
.assign(inPacket.data() + sizeof(SessionHeader_t),
inPacket.data() + sizeof(SessionHeader_t) + payloadLen);
@@ -275,6 +281,12 @@
auto sessTrailerPos = sizeof(SessionHeader_t) + payloadLen + paddingLen;
+ // verify packet size includes trailer struct starts at sessTrailerPos
+ if (packet.size() < (sessTrailerPos + sizeof(SessionTrailer_t)))
+ {
+ return false;
+ }
+
auto trailer = reinterpret_cast<const SessionTrailer_t*>(packet.data() +
sessTrailerPos);