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

Add procmem slicing and refactor to make proper MemoryBuffer interface #122

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

psrok1
Copy link
Member

@psrok1 psrok1 commented Apr 19, 2024

After looking at #119 I found that bb4c7d48773b21c62885d0206c9414176c254e6104b4a0ffe730aa570b424948 is not actually b = open("x.exe").read(); b = b * 100, but set of different binaries/

The main problem was that procmempe.from_memory(p, base=0x100) doesn't make m that starts from 0x100. It just makes reference to the same memory and sets imgbase to 0x100. It means that first binary bb4c7d48773b21c62885d0206c9414176c254e6104b4a0ffe730aa570b424948 was ripped n times, so carving was not really working as intended 😅

In this PR I moved the "memory management" part to the separate set of classes MemoryBuffer and implemented proper slicing.

ProcessMemory has implemented one extra method slicev that allows to get a slice of memory view over MemoryBuffer. The only limitation is that slice must be within single region.

Along with #121 reducing memory footprint, it should work a bit more correctly.

@psrok1 psrok1 requested a review from msm-cert April 19, 2024 12:30
@psrok1 psrok1 force-pushed the refactor/memory-buffer-and-slicev branch from c7fadb2 to a32b73d Compare May 9, 2024 13:59
@psrok1
Copy link
Member Author

psrok1 commented May 10, 2024

This requires a bit different approach.

Main issue is that memoryview holds a pointer to the underlying buffer and need to be explicitly released before releasing the main buffer

  File "/usr/local/lib/python3.10/site-packages/malduck/extractor/extract_manager.py", line 105, in push_file
    with ProcessMemory.from_file(filepath, base=base) as p:
  File "/usr/local/lib/python3.10/site-packages/malduck/procmem/procmem.py", line 106, in __exit__
    self.close()
  File "/usr/local/lib/python3.10/site-packages/malduck/procmem/procmem.py", line 121, in close
    self.memory.release()
  File "/usr/local/lib/python3.10/site-packages/malduck/procmem/membuf.py", line 100, in release
    self.mapped_buf.close()
BufferError: cannot close exported pointers exist

In many modules and components we don't keep track on all derived procmems and we rely on GC there. When we release memory allocated by procmem.from_file, we just assume that there are no active references to procmems produced by procmem.from_memory that are referencing the same memory.

@psrok1 psrok1 marked this pull request as draft May 10, 2024 13:49
@psrok1 psrok1 marked this pull request as ready for review May 13, 2024 10:13
@psrok1 psrok1 force-pushed the refactor/memory-buffer-and-slicev branch from ebf71a2 to b85efbb Compare May 13, 2024 11:41
@psrok1 psrok1 removed the request for review from msm-cert May 15, 2024 15:03
@psrok1 psrok1 marked this pull request as draft May 15, 2024 16:39
@psrok1
Copy link
Member Author

psrok1 commented May 15, 2024

Added reference tracking of mmap slices in ab61f36 and c684022

When MmapMemoryBuffer is going to be closed and underlying mmap is going to be released, MMapMemoryBuffer has weakrefset to all slice objects (MmapSliceMemoryBuffer) that keep reference to that mmap and are not already GC'd/explicitly released. This weakrefset is used to call release() on all these slices, so we can safely release the main mmap allocation.

@psrok1 psrok1 marked this pull request as ready for review May 16, 2024 10:43
@psrok1 psrok1 requested a review from msm-cert May 16, 2024 10:43
@psrok1 psrok1 removed the request for review from msm-cert May 17, 2024 10:35
@psrok1 psrok1 marked this pull request as draft May 17, 2024 10:35
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.

1 participant