Skip to content

Fix bugs in libhoth_extract_ot_bundle #209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sandumjacob wants to merge 1 commit into
base: main
Choose a base branch
from
+115 −0
Open

Fix bugs in libhoth_extract_ot_bundle #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions protocol/opentitan_version.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,21 @@ 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);

// 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) >
Copy link
Collaborator

@xorptr xorptr Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this check relies on 2 implicit conditions for correctness (which hold true currently):

  1. (OPENTITAN_OFFSET_HEADER_DATA + 4) <= image_size, so that offset can be read properly. This is currently true since OPENTITAN_OFFSET_HEADER_DATA < OPENTITAN_OFFSET_APP_FW and image_size >= smallest_fw
  2. OPENTITAN_OFFSET_VERSION_MINOR > OPENTITAN_OFFSET_VERSION_MAJOR so that ROM_EXT and APP version can be read properly.

Would it be a good idea to check these conditions explicitly (using static_assert or in some other manner)?

image_size) {
fprintf(stderr, "Image offset is invalid");
Copy link
Collaborator

@xorptr xorptr Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Would it be a good idea to print offset in the error messages?

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 caused integer overflow");
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 |
Expand Down
100 changes: 100 additions & 0 deletions protocol/opentitan_version_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,103 @@ TEST_F(LibHothTest, opentitan_version_test) {
EXPECT_EQ(response.app.slots[1].major, mock_response.app.slots[1].major);
EXPECT_EQ(response.app.slots[1].minor, mock_response.app.slots[1].minor);
}

TEST_F(LibHothTest, ExtractOtBundleBoundsCheckLargeOffset) {
size_t image_size = 70000;
std::vector<uint8_t> 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<uint8_t> 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<uint8_t> 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<uint8_t> 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);
}
Loading