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
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
25 changes: 25 additions & 0 deletions protocol/opentitan_version.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Expand All @@ -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 |
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 @@ -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<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