diff --git a/protocol/opentitan_version.c b/protocol/opentitan_version.c index b4b4902..45fb94d 100644 --- a/protocol/opentitan_version.c +++ b/protocol/opentitan_version.c @@ -50,6 +50,11 @@ int libhoth_extract_ot_bundle(const uint8_t* image, size_t image_size, return -1; } + static_assert( + OPENTITAN_OFFSET_APP_FW + sizeof(struct opentitan_image_version) >= + OPENTITAN_OFFSET_HEADER_DATA + 4, + "Smallest FW size must be enough to cover header data offset"); + // Check if the image has the correct magic number char magic[] = "_OTFWUPDATE_"; for (int i = 0; i < (sizeof(magic) - 1); i++) { @@ -66,6 +71,26 @@ int libhoth_extract_ot_bundle(const uint8_t* image, size_t image_size, image[OPENTITAN_OFFSET_HEADER_DATA + 1] << 8 | image[OPENTITAN_OFFSET_HEADER_DATA + 2] << 16 | image[OPENTITAN_OFFSET_HEADER_DATA + 3] << 24); + + // Check that version offsets are within bounds + static_assert( + OPENTITAN_OFFSET_VERSION_MINOR >= OPENTITAN_OFFSET_VERSION_MAJOR, + "Version minor offset must be >= major offset"); + + // Checks that the image offset doesn't cause + // the offset calculations to read beyond the image buffer. + if ((offset + OPENTITAN_OFFSET_APP_FW + OPENTITAN_OFFSET_VERSION_MINOR + 4) > + image_size) { + fprintf(stderr, "Image offset %u is invalid", offset); + return -1; + } + // Checks that the image offset won't overflow the offset calculations + if ((offset + OPENTITAN_OFFSET_APP_FW + OPENTITAN_OFFSET_VERSION_MINOR + 4) < + offset) { + fprintf(stderr, "Image offset %u caused integer overflow", offset); + return -1; + } + rom_ext->major = image[offset + OPENTITAN_OFFSET_VERSION_MAJOR] | image[offset + OPENTITAN_OFFSET_VERSION_MAJOR + 1] << 8 | image[offset + OPENTITAN_OFFSET_VERSION_MAJOR + 2] << 16 | diff --git a/protocol/opentitan_version_test.cc b/protocol/opentitan_version_test.cc index 845d68a..d7e0ead 100644 --- a/protocol/opentitan_version_test.cc +++ b/protocol/opentitan_version_test.cc @@ -180,3 +180,103 @@ TEST_F(LibHothTest, opentitan_image_compare_test) { EXPECT_FALSE(libhoth_update_complete(&resp, &romext, &romext)); EXPECT_FALSE(libhoth_update_complete(&resp, &app, &app)); } + +TEST_F(LibHothTest, ExtractOtBundleBoundsCheckLargeOffset) { + size_t image_size = 70000; + std::vector image(image_size, 0); + struct opentitan_image_version rom_ext; + struct opentitan_image_version app; + + // Magic + const char* magic = "_OTFWUPDATE_"; + memcpy(image.data(), magic, strlen(magic)); + + // Offset = 65536, causes + // 65536 + OPENTITAN_OFFSET_APP_FW + OPENTITAN_OFFSET_VERSION_MINOR + 4 + // = 65536 + 65536 + 840 + 4 = 131916 + // to be > image_size, triggering bounds check. + uint32_t offset = 65536; + image[OPENTITAN_OFFSET_HEADER_DATA] = offset & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 1] = (offset >> 8) & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 2] = (offset >> 16) & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 3] = (offset >> 24) & 0xff; + + // Expect call to fail with -1 + EXPECT_EQ(libhoth_extract_ot_bundle(image.data(), image_size, &rom_ext, &app), + -1); +} + +TEST_F(LibHothTest, ExtractOtBundleBoundsCheckSmallImage) { + size_t image_size = 66379; + std::vector image(image_size, 0); + struct opentitan_image_version rom_ext; + struct opentitan_image_version app; + + // Magic + const char* magic = "_OTFWUPDATE_"; + memcpy(image.data(), magic, strlen(magic)); + + // Offset = 0, but image_size is too small for reads because + // 0 + OPENTITAN_OFFSET_APP_FW + OPENTITAN_OFFSET_VERSION_MINOR + 4 + // = 0 + 65536 + 840 + 4 = 66380 + // is > image_size, triggering bounds check. + uint32_t offset = 0; + image[OPENTITAN_OFFSET_HEADER_DATA] = offset & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 1] = (offset >> 8) & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 2] = (offset >> 16) & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 3] = (offset >> 24) & 0xff; + + // Expect call to fail with -1 + EXPECT_EQ(libhoth_extract_ot_bundle(image.data(), image_size, &rom_ext, &app), + -1); +} + +TEST_F(LibHothTest, ExtractOtBundleImageTooSmall) { + size_t image_size = 100; + std::vector image(image_size, 0); + struct opentitan_image_version rom_ext; + struct opentitan_image_version app; + + // Magic + const char* magic = "_OTFWUPDATE_"; + memcpy(image.data(), magic, strlen(magic)); + + // image_size is 100, which is smaller than + // OPENTITAN_OFFSET_APP_FW + sizeof(struct opentitan_image_version) + // = 65536 + 64 = 65600 + uint32_t offset = 0; + image[OPENTITAN_OFFSET_HEADER_DATA] = offset & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 1] = (offset >> 8) & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 2] = (offset >> 16) & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 3] = (offset >> 24) & 0xff; + + // Expect call to fail with -1 + EXPECT_EQ(libhoth_extract_ot_bundle(image.data(), image_size, &rom_ext, &app), + -1); +} + +TEST_F(LibHothTest, ExtractOtBundleIntegerOverflow) { + size_t image_size = 0xFFFFFFFF; + std::vector image(66380, 0); // small image buffer is fine + struct opentitan_image_version rom_ext; + struct opentitan_image_version app; + + // Magic + const char* magic = "_OTFWUPDATE_"; + memcpy(image.data(), magic, strlen(magic)); + + // Offset = 0xFFFFFFFF, causes + // offset + OPENTITAN_OFFSET_APP_FW + OPENTITAN_OFFSET_VERSION_MINOR + 4 + // to wrap around. + // 0xFFFFFFFF + 65536 + 840 + 4 = 66379 (mod 2^32) + // 66379 < 0xFFFFFFFF should trigger overflow check + uint32_t offset = 0xFFFFFFFF; + image[OPENTITAN_OFFSET_HEADER_DATA] = offset & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 1] = (offset >> 8) & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 2] = (offset >> 16) & 0xff; + image[OPENTITAN_OFFSET_HEADER_DATA + 3] = (offset >> 24) & 0xff; + + // Expect call to fail with -1 + EXPECT_EQ(libhoth_extract_ot_bundle(image.data(), image_size, &rom_ext, &app), + -1); +}