blob_handler: Add check to avoid OOB memory access
There's a potential for OOB memory access in statGeneric if
sendIpmiPayload returns a small or empty respones. Added a check to guard
against it and added unit tests.
Change-Id: I224cd3cb391c2f8bbde87aec478bf4042fbdfd47
Signed-off-by: Brandon Kim <brandonkim@google.com>
diff --git a/src/ipmiblob/blob_handler.cpp b/src/ipmiblob/blob_handler.cpp
index f7b0151..e9b82fd 100644
--- a/src/ipmiblob/blob_handler.cpp
+++ b/src/ipmiblob/blob_handler.cpp
@@ -241,6 +241,11 @@
const std::vector<std::uint8_t>& request)
{
StatResponse meta;
+ static constexpr std::size_t blobStateSize = sizeof(meta.blob_state);
+ static constexpr std::size_t metaSize = sizeof(meta.size);
+ static constexpr std::size_t metaOffset = blobStateSize + metaSize;
+ static constexpr std::size_t minRespSize =
+ metaOffset + sizeof(std::uint8_t);
std::vector<std::uint8_t> resp;
try
@@ -252,14 +257,35 @@
throw;
}
- std::memcpy(&meta.blob_state, &resp[0], sizeof(meta.blob_state));
- std::memcpy(&meta.size, &resp[sizeof(meta.blob_state)], sizeof(meta.size));
- int offset = sizeof(meta.blob_state) + sizeof(meta.size);
- std::uint8_t len = resp[offset];
+ // Avoid out of bounds memcpy below
+ if (resp.size() < minRespSize)
+ {
+ std::fprintf(stderr,
+ "Invalid response length, Got %li which is less than "
+ "minRespSize %li\n",
+ resp.size(), minRespSize);
+ throw BlobException("Invalid response length");
+ }
+
+ std::memcpy(&meta.blob_state, &resp[0], blobStateSize);
+ std::memcpy(&meta.size, &resp[blobStateSize], metaSize);
+ std::uint8_t len = resp[metaOffset];
+
+ auto metaDataLength = resp.size() - minRespSize;
+ if (metaDataLength != len)
+ {
+ std::fprintf(stderr,
+ "Metadata length did not match actual length, Got %li "
+ "which does not equal expected length %i\n",
+ metaDataLength, len);
+ throw BlobException("Metadata length did not match actual length");
+ }
+
if (len > 0)
{
- std::copy(resp.begin() + offset + sizeof(len), resp.end(),
- std::back_inserter(meta.metadata));
+ meta.metadata.resize(len);
+ std::copy(resp.begin() + minRespSize, resp.end(),
+ meta.metadata.begin());
}
return meta;
diff --git a/test/tools_blob_unittest.cpp b/test/tools_blob_unittest.cpp
index 3b64d57..a9ec639 100644
--- a/test/tools_blob_unittest.cpp
+++ b/test/tools_blob_unittest.cpp
@@ -1,3 +1,4 @@
+#include <ipmiblob/blob_errors.hpp>
#include <ipmiblob/blob_handler.hpp>
#include <ipmiblob/test/crc_mock.hpp>
#include <ipmiblob/test/ipmi_interface_mock.hpp>
@@ -214,6 +215,41 @@
EXPECT_EQ(metadata, meta.metadata);
}
+TEST_F(BlobHandlerTest, getStatWithWrongMetadataLength)
+{
+ /* Stat fails when wrong metadata length is received */
+ auto ipmi = CreateIpmiMock();
+ IpmiInterfaceMock* ipmiMock =
+ reinterpret_cast<IpmiInterfaceMock*>(ipmi.get());
+ BlobHandler blob(std::move(ipmi));
+
+ std::vector<std::uint8_t> request = {
+ 0xcf, 0xc2,
+ 0x00, static_cast<std::uint8_t>(BlobOEMCommands::bmcBlobStat),
+ 0x00, 0x00,
+ 'a', 'b',
+ 'c', 'd',
+ 0x00};
+
+ /* return blob_state: 0xffff, size: 0x00, len: 1, metadata 0x3445 */
+ std::vector<std::uint8_t> resp = {0xcf, 0xc2, 0x00, 0x00, 0x00, 0xff, 0xff,
+ 0x00, 0x00, 0x00, 0x00, 0x01, 0x34, 0x45};
+
+ std::vector<std::uint8_t> reqCrc = {'a', 'b', 'c', 'd', 0x00};
+ std::vector<std::uint8_t> respCrc = {0xff, 0xff, 0x00, 0x00, 0x00,
+ 0x00, 0x01, 0x34, 0x45};
+ EXPECT_CALL(crcMock, generateCrc(ContainerEq(reqCrc)))
+ .WillOnce(Return(0x00));
+ EXPECT_CALL(crcMock, generateCrc(ContainerEq(respCrc)))
+ .WillOnce(Return(0x00));
+
+ EXPECT_CALL(*ipmiMock,
+ sendPacket(ipmiOEMNetFn, ipmiOEMBlobCmd, ContainerEq(request)))
+ .WillOnce(Return(resp));
+
+ EXPECT_THROW(blob.getStat("abcd"), BlobException);
+}
+
TEST_F(BlobHandlerTest, getStatNoMetadata)
{
/* Stat received no metadata. */
@@ -230,7 +266,7 @@
'c', 'd',
0x00};
- /* return blob_state: 0xffff, size: 0x00, metadata 0x3445 */
+ /* return blob_state: 0xffff, size: 0x00, metadata 0x0000 */
std::vector<std::uint8_t> resp = {0xcf, 0xc2, 0x00, 0x00, 0x00, 0xff,
0xff, 0x00, 0x00, 0x00, 0x00, 0x00};
@@ -268,7 +304,7 @@
0x00, 0x00,
0x01, 0x00};
- /* return blob_state: 0xffff, size: 0x00, metadata 0x3445 */
+ /* return blob_state: 0xffff, size: 0x00, metadata 0x0000 */
std::vector<std::uint8_t> resp = {0xcf, 0xc2, 0x00, 0x00, 0x00, 0xff,
0xff, 0x00, 0x00, 0x00, 0x00, 0x00};
@@ -292,6 +328,62 @@
EXPECT_EQ(metadata, meta.metadata);
}
+TEST_F(BlobHandlerTest, getSessionStatEmptyResp)
+{
+ /* The get session stat fails after getting empty response */
+ auto ipmi = CreateIpmiMock();
+ IpmiInterfaceMock* ipmiMock =
+ reinterpret_cast<IpmiInterfaceMock*>(ipmi.get());
+ BlobHandler blob(std::move(ipmi));
+
+ std::vector<std::uint8_t> request = {
+ 0xcf, 0xc2,
+ 0x00, static_cast<std::uint8_t>(BlobOEMCommands::bmcBlobSessionStat),
+ 0x00, 0x00,
+ 0x01, 0x00};
+
+ std::vector<std::uint8_t> resp;
+ std::vector<std::uint8_t> reqCrc = {0x01, 0x00};
+
+ EXPECT_CALL(crcMock, generateCrc(ContainerEq(reqCrc)))
+ .WillOnce(Return(0x00));
+
+ EXPECT_CALL(*ipmiMock,
+ sendPacket(ipmiOEMNetFn, ipmiOEMBlobCmd, ContainerEq(request)))
+ .WillOnce(Return(resp));
+
+ EXPECT_THROW(blob.getStat(0x0001), BlobException);
+}
+
+TEST_F(BlobHandlerTest, getSessionStatInvalidHeader)
+{
+ /* The get session stat fails after getting a response shorter than the
+ * expected headersize but with the expected OEN
+ */
+ auto ipmi = CreateIpmiMock();
+ IpmiInterfaceMock* ipmiMock =
+ reinterpret_cast<IpmiInterfaceMock*>(ipmi.get());
+ BlobHandler blob(std::move(ipmi));
+
+ std::vector<std::uint8_t> request = {
+ 0xcf, 0xc2,
+ 0x00, static_cast<std::uint8_t>(BlobOEMCommands::bmcBlobSessionStat),
+ 0x00, 0x00,
+ 0x01, 0x00};
+
+ std::vector<std::uint8_t> resp = {0xcf, 0xc2, 0x00, 0x00};
+ std::vector<std::uint8_t> reqCrc = {0x01, 0x00};
+
+ EXPECT_CALL(crcMock, generateCrc(ContainerEq(reqCrc)))
+ .WillOnce(Return(0x00));
+
+ EXPECT_CALL(*ipmiMock,
+ sendPacket(ipmiOEMNetFn, ipmiOEMBlobCmd, ContainerEq(request)))
+ .WillOnce(Return(resp));
+
+ EXPECT_THROW(blob.getStat(0x0001), BlobException);
+}
+
TEST_F(BlobHandlerTest, openBlobSucceeds)
{
/* The open blob succeeds. */