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 mmap command that executes the mmap syscall in the inferior #1952

Merged
merged 16 commits into from
Dec 14, 2023

Conversation

mbrla0
Copy link
Contributor

@mbrla0 mbrla0 commented Dec 12, 2023

This PR adds a mmap command that calls the mmap system call in the context of the inferior from inside pwndbg, allowing users to dynamically change the virtual memory layout of the program being analyzed.

Example usage:

# mmap 0x10000 bytes at any addr with RWX (default) permissions
mmap 0 0x10000

# mmap 4096 bytes at 0xBEEF000 with RW perms
mmap 0xBEEF000 0x1000 PROT_READ|PROT_WRITE MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS

It achieves this in a manner similar to how the mprotect command is currently implemented, with a few extra safeguards. Additionally, it moves that functionality into a new module located at pwndbg/gdblib/shellcode.py, and splits it into a function that allows for execution of arbitrary blobs of machine code in the context of the inferior (exec_shellcode), and, based on it, a function that allows for convenient invocation of system calls (exec_syscall).

The mmap command also notifies users of unaligned addresses (though it does not change those addresses on their behalf), and that checks for possibly overwritten mappings when MAP_FIXED is used and warns users of them.

Finally, it also makes it so that mprotect uses the exec_syscall function, rather than have its own separate implementation.

A few questions that I feel need to be addressed:

  • Should we be this worried about making sure users don't overwrite existing mappings accidentally, or should we be more lax?
  • Should tests specifically for mmap be added?

 - Additionally, moves syscall execution and general inferior-scoped code
   execution facilities into a single, new file, in 'pwndbg/gdblib/shellcode.py'
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (427bf8c) 60.64% compared to head (897e888) 60.67%.

Files Patch % Lines
pwndbg/commands/mmap.py 67.85% 19 Missing and 8 partials ⚠️
pwndbg/gdblib/shellcode.py 88.57% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1952      +/-   ##
==========================================
+ Coverage   60.64%   60.67%   +0.03%     
==========================================
  Files         181      183       +2     
  Lines       22315    22420     +105     
  Branches     2094     2113      +19     
==========================================
+ Hits        13532    13603      +71     
- Misses       8072     8093      +21     
- Partials      711      724      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@disconnect3d disconnect3d changed the title Add support for the mmap command Add mmap command that executes the mmap syscall in the inferior Dec 13, 2023
"PROT_READ": 0x1,
"PROT_WRITE": 0x2,
"PROT_EXEC": 0x4,
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe also support RWX or rwx?

"""Heuristic to convert PROT_EXEC|PROT_WRITE to integer value."""
prot_int = 0
for k, v in prot_dict.items():
if k in protstr:
Copy link
Member

Choose a reason for hiding this comment

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

Fwiw PROT_EXECasdf will also work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, which isn't optimal, but I figured that if it's good enough for mprotect, it's probably good enough here too. But I could make it stricter.

Copy link
Member

Choose a reason for hiding this comment

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

its probably okay for now

)

page = pwndbg.lib.memory.Page(addr, int(length), 0, 0)
collisions = []
Copy link
Member

Choose a reason for hiding this comment

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

In theory, there should never be more than a single collision (in practice if we have broken vmmap info there will be). We can safely assume a single collision here. Its fine if we print a single one if there are more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I follow. If any range is permissible for our mmap, it could, in the worst case, technically collide with all of the mappings, no? Nothing's really stopping you from trying to call this with MAP_FIXED for [0, 0xffffffffffffffff[ (I know in practice there are limitations on covering the whole range, but this same logic applies for smaller ranges too).

Copy link
Member

Choose a reason for hiding this comment

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

Oh... you are right. For some reason I was thinking that we map a single address instead of a range. Forgive my stupidness :)

@disconnect3d
Copy link
Member

Should we be this worried about making sure users don't overwrite existing mappings accidentally, or should we be more lax?

We can be more lax tbh, such commands will always be "best effort" and are there just as a utility to support "interesting use cases" :D

Should tests specifically for mmap be added?

Generally yes. This can be done in separate PR too

@mbrla0
Copy link
Contributor Author

mbrla0 commented Dec 13, 2023

We can be more lax tbh, such commands will always be "best effort" and are there just as a utility to support "interesting use cases" :D

Sure, I just feel that the testing I'm doing might be excessive. I'm thinking of disabling the more expensive range collision checks when run with --force instead of still running them and printing it as a warning, to possibly speed up cases where people are using the command in their scripts. How does that sound?

Generally yes. This can be done in separate PR too

Shouldn't be too much trouble adding it now

@mbrla0
Copy link
Contributor Author

mbrla0 commented Dec 13, 2023

I've added some tests, but it's still missing a test for file based mappings. Should add them later today.

@disconnect3d disconnect3d merged commit dfd5f95 into pwndbg:dev Dec 14, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants