Skip to content
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.

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

Fix overflow in search_tree_size #336

Merged
merged 2 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
6 changes: 6 additions & 0 deletions Changes.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 1.8.1

* On very large databases, the calculation to determine the search tree
size could overflow. This was fixed and several additional guards
against overflows were added. Reported by Sami Salonen. GitHub #335.

## 1.8.0 - 2023-11-07

* `PACKAGE_VERSION` is now a private compile definition when building
Expand Down
4 changes: 1 addition & 3 deletions src/data-pool.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@
#include <stddef.h>
#include <stdlib.h>

static bool can_multiply(size_t const, size_t const, size_t const);

// Allocate an MMDB_data_pool_s. It initially has space for size
// MMDB_entry_data_list_s structs.
MMDB_data_pool_s *data_pool_new(size_t const size) {
Expand Down Expand Up @@ -43,7 +41,7 @@ MMDB_data_pool_s *data_pool_new(size_t const size) {
// the given max. max will typically be SIZE_MAX.
//
// We want to know if we'll wrap around.
static bool can_multiply(size_t const max, size_t const m, size_t const n) {
bool can_multiply(size_t const max, size_t const m, size_t const n) {
if (m == 0) {
return false;
}
Expand Down
1 change: 1 addition & 0 deletions src/data-pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ typedef struct MMDB_data_pool_s {
MMDB_entry_data_list_s *blocks[DATA_POOL_NUM_BLOCKS];
} MMDB_data_pool_s;

bool can_multiply(size_t const, size_t const, size_t const);
MMDB_data_pool_s *data_pool_new(size_t const);
void data_pool_destroy(MMDB_data_pool_s *const);
MMDB_entry_data_list_s *data_pool_alloc(MMDB_data_pool_s *const);
Expand Down
23 changes: 17 additions & 6 deletions src/maxminddb.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#endif
#include <windows.h>
#include <ws2ipdef.h>
#define SSIZE_MAX INTPTR_MAX
typedef ADDRESS_FAMILY sa_family_t;
#else
#include <arpa/inet.h>
Expand Down Expand Up @@ -289,18 +290,28 @@ int MMDB_open(const char *const filename, uint32_t flags, MMDB_s *const mmdb) {
goto cleanup;
}

uint32_t search_tree_size =
mmdb->metadata.node_count * mmdb->full_record_byte_size;
if (!can_multiply(SSIZE_MAX,
mmdb->metadata.node_count,
mmdb->full_record_byte_size)) {
status = MMDB_INVALID_METADATA_ERROR;
goto cleanup;
}
ssize_t search_tree_size = (ssize_t)mmdb->metadata.node_count *
(ssize_t)mmdb->full_record_byte_size;

mmdb->data_section =
mmdb->file_content + search_tree_size + MMDB_DATA_SECTION_SEPARATOR;
if (search_tree_size + MMDB_DATA_SECTION_SEPARATOR >
(uint32_t)mmdb->file_size) {
if (search_tree_size + MMDB_DATA_SECTION_SEPARATOR > mmdb->file_size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to check if this would overflow too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how plausibly this is an issue, but I did add a check. The first part of the updated check is likely redundant as finding the metadata would have failed if that is not true, but I left it for clarity.

status = MMDB_INVALID_METADATA_ERROR;
goto cleanup;
}
ssize_t data_section_size =
mmdb->file_size - search_tree_size - MMDB_DATA_SECTION_SEPARATOR;
horgh marked this conversation as resolved.
Show resolved Hide resolved
if (data_section_size > UINT32_MAX || data_section_size <= 0) {
status = MMDB_INVALID_METADATA_ERROR;
goto cleanup;
}
mmdb->data_section_size = (uint32_t)mmdb->file_size - search_tree_size -
MMDB_DATA_SECTION_SEPARATOR;
mmdb->data_section_size = (uint32_t)data_section_size;

// Although it is likely not possible to construct a database with valid
// valid metadata, as parsed above, and a data_section_size less than 3,
Expand Down
Loading