commit 714e3888a8438161fdc855d0c7e83e4a79815364
author Brandon Kim <brandonkim@google.com> Mon Jun 22 12:36:32 2020 -0700
committer Brandon Kim <brandonkim@google.com> Wed Jul 08 13:52:50 2020 -0700
tree 965e3ca648ecbe466d217c9a6b522c7f4d04892b
parent 570fcc6d289f0e201d4796fe19fabfed8a4995fe

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>

test/tools_blob_unittest.cpp

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. */