VirusTotal / yara

The pattern matching swiss knife
https://virustotal.github.io/yara/
BSD 3-Clause "New" or "Revised" License
8.13k stars 1.42k forks source link

v4.3.1 broke 8-byte alignment of some structs #1928

Closed vthib closed 1 year ago

vthib commented 1 year ago

Describe the bug

On v4.3.1, some YR_MATCH can be 4-byte aligned instead of 8-byte aligned, leading to non-aligned reads at least on Windows MSVC x86. This leads to panics in yara-rust as all structs are expected to be properly aligned.

This is a regression caused by 143edac.

The issue is that all notebook allocations start with this code:

// Round up the size to a multiple of 8, which also implies that the returned
// pointers are aligned to 8 bytes. The 8-byte alignment is required by some
// platforms (e.g. ARM and Sparc) that have strict alignment requirements when
// deferrencing pointers to types larger than a byte.
size = (size + 7) & ~0x7;

But this is only true as long as the initial page pointer of the page is already 8-byte aligned. This used to be true before 143edac, as the definition of the page was:

struct YR_NOTEBOOK_PAGE
{
  // Amount of bytes in the page that are actually used.
  size_t used;
  // Pointer to next page.
  YR_NOTEBOOK_PAGE* next;
  // Page's data.
  uint8_t data[0];
};

Since the page is allocated with malloc which guarantees a 8-byte alignment, the data pointer was also 8-byte aligned (at least on 32-bit and 64-bit systems, not on smaller ones).

But the aforementioned commit brought a new field in the struct:

struct YR_NOTEBOOK_PAGE
{
  // Size of this page.
  size_t size;
  // Amount of bytes in the page that are actually used.
  size_t used;
  // Pointer to next page.
  YR_NOTEBOOK_PAGE* next;
  // Page's data.
  uint8_t data[0];
};

This does not break anything on 64-bit systems, since all fields are 8-byte long. But this breaks on 32-bits system, where the data pointer is now 4-byte aligned.

To Reproduce

For example, adding this assert in scan.c:

new_match = yr_notebook_alloc(
  context->matches_notebook, sizeof(YR_MATCH));
assert(((intptr_t) new_match) & 0x7 == 0);

Then running the tests on a 32-bit system will lead to the assert failing.

Expected behavior

As documented in the notebook, all allocations should be 8-byte aligned.

plusvic commented 1 year ago

Should be fixed in #1930