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 process scanning on macos by properly listing memory regions #2033

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

vthib
Copy link
Contributor

@vthib vthib commented Jan 27, 2024

The process scanning on macos was broken since 4.2.0 in this commit: 605b2ed

Logic was added to properly iterate on memory chunks if a memory region is too big, but this completely broke the iteration. To retrieve a memory region, the code basically does:

current_begin = 0;
for (;;) {
    vm_address_t addr = current_begin;

    mach_vm_region(..., &addr, &size, ...);

    size_t chunk_size = size - (current_begin - addr);
    ...
}

It assumes that addr is smaller than current_begin, which is never the case outside of chunking. This leads to an iteration on completely wrong regions, leading to read that either fail because the starting addr or length is wrong, or that succeed, but do not cover the whole region.

For example, with a process memory that looks like:

1065a6000-1065f2000
1065f2000-1065f6000
1065f6000-1065fa000
...
600000000000-600008000000
600008000000-600010000000
...
7fe537f00000-7fe538000000
7fe538000000-7fe538800000

The very first call would have current_begin=0 and addr=0x1065a6000, leading to underflow when computing chunk_size, and trying to iterate on the region 0x00000000-0x40000000 (as the max memory chunk is 1GB per default). this will fail to fetch, and will then iterate on 0x40000000-0x80000000, then 0x80000000-0xC0000000, then 0xC0000000-0x100000000. We will finally reach the very first real memory region, but fail to properly compute the region size with another underflow, etc.

This commit fixes the issue by distinguishing the two cases:

  • either addr > current_begin : current_begin is not in any memory region, and we must fast forward.
  • or addr <= current_begin: we can compute the current chunk by diffing the two values.

This bug was not detected by any tests because the test_process_scan was not running on macos: trying to scan the sh process always fails even when root, and the test was thus skipped. I hacked the test locally to check if it worked by replacing the sh process by something else that we can attach to, and it showed the scanning fails. With this commit, the test does pass.

I did not commit any modification to the test as I used a custom binary for this and it needs to be run as root, but it would probably be a good idea to adapt it.

The process scanning on macos was broken since 4.2.0 in this commit:

605b2ed

Logic was added to properly iterate on memory chunks if a memory region
was too big, but this completely broke the iteration. To retrieve a
memory region, the code basically does:

```
current_begin = 0;
for (;;) {
    vm_address_t addr = current_begin;

    mach_vm_region(..., &addr, &size, ...);

    size_t chunk_size = size - (current_begin - addr);
    ...
}
```

It assumes that `addr` is smaller than `current_begin`, which is never
the case outside of chunking. This leads to an iteration on completely
wrong regions, leading to read that either fail because the starting
addr or length is wrong, or that succeed, but do not cover the whole
region.

For example, with a process memory that looks like:

```
1065a6000-1065f2000
1065f2000-1065f6000
1065f6000-1065fa000
...
600000000000-600008000000
600008000000-600010000000
...
7fe537f00000-7fe538000000
7fe538000000-7fe538800000
```

The very first call would have `current_begin=0` and `addr=0x1065a6000`,
leading to underflow when computing `chunk_size`, and trying to iterate
on the region `0x00000000-0x40000000` (as the max memory chunk is 1GB
per default). this will fail to fetch, and will then iterate on
`0x40000000-0x80000000`, then `0x80000000-0xC0000000`, then
`0xC0000000-0x100000000`. We will finally reach the very first real
memory region, but fail to properly compute the region size with another
underflow, etc.

This commit fixes the issue by distinguishing the two cases:
- either `addr > current_begin` : `current_begin` is not in any memory
  region, and we must fast forward.
- or `addr <= current_begin`: we can compute the current chunk by
  diffing the two values.

This was not tested because the test_process_scan was not running on
macos: trying to scan the sh process always fails even when root, and
the test was thus skipped. I hacked the test locally to check if it
worked by replacing the sh process by something else that we can attach
to, and it showed the scanning fails. With this commit, the test does
pass.

I did not commit any modification to the test as I used a custom binary
for this and it needs to be run as root, but it would probably be a good
idea to adapt it.
@plusvic plusvic merged commit 8695c84 into VirusTotal:master Jan 30, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants