-
Notifications
You must be signed in to change notification settings - Fork 769
Fix elf stack, pe stack and pe sections memory protection #1601
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
base: dev
Are you sure you want to change the base?
Conversation
|
Hi there, welcome and thanks for the contribution. Setting stack pages as non-executable is also inaccurate since stack pages may be either executable or not based on how the file was compiled. Stack attributes may be found in the As for mapping PE sections with their respective attributes, this was already implemented for UEFI PE loader here, I just forgot to port it to the Windows PE loader... Do you think you could extract the correct stack attributes and use them? |
|
I'm on it |
|
Tried porting your function to QlLoaderPE, changed some stuff but couldn't manage to add sections infos for each one of them, plus when executing some pe binaries(qiling\examples\rootfs\x8664_windows\bin\x8664_clipboard_test.exe, to be exact), I get this error message when changing the memory protection of a section: |
elicn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the adjustments.
Please see my comments.
qiling/loader/elf.py
Outdated
| stack_perm = UC_PROT_NONE | ||
| for seg in elffile.iter_segments('PT_GNU_STACK'): | ||
| stack_perm = QlLoaderELF.seg_perm_to_uc_prot(seg['p_flags']) | ||
|
|
||
| QlLoaderELF.seg_perm_to_uc_prot(stack_perm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why iterating through segments where there is just one (or none)?
It would make more sense to get the segment by name and then extract its flags.
Also, in case segment is found, stack_perm already holds UC Protection value, not sure what happens on line 99 (converting again? where the returned value goes?)
qiling/loader/pe.py
Outdated
| self.ql.log.info(f'PE entry point at {self.entry_point:#x}') | ||
|
|
||
| self.ql.mem.map(image_base, image_size, info=f'{image_name}') | ||
| _map_sections_(self.ql, pe, image_base, image_size, image_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested on the UEFI PE loader, where this code was taken from, some PE files might not be suitable to be mapped like that. Setting permissions by page is doable only when the file segments are page-aligned; if they are not, one page may contain multiple segments with different permissions, we have to map the file as a whole (all RWX). We should maintain the same behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, should I just leave it as it is with rwx for every section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The point is that executables could be either compatible with this kind of mapping or not based on whether their sections are page-aligned [or not]. If they are not compatible, the only way is to map them as a whole with RWX permissions. Follow the UEFI PE loader to see how it works.
| # done manipulating pe file; write its contents into memory | ||
| self.ql.mem.write(image_base, bytes(pe.get_memory_mapped_image())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe file contents cannot be written till this point, mainly because of the pe.parse_data_directories 2 lines above that still manipulates the file contents.
|
Hoping this is good now |
|
|
||
| # setup program stack | ||
| stack_seg = elffile.iter_segments('PT_GNU_STACK') | ||
| stack_perm = QlLoaderELF.seg_perm_to_uc_prot(next(stack_seg)['p_flags']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there is no such segment in the file? The code should be prepared to handle this situation (that is, default to RWX).
| self.dll_size += dll_len | ||
| _map_sections_(self.ql, dll, dll_base, dll_len, dll_name) | ||
| _map_pe_(self.ql, dll, dll_base, dll_len, dll_name) | ||
| self.ql.mem.write(dll_base, bytes(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not going to work... Mapping a PE file section be section does not necessarily form a continuous block in memory, so we cannot just mem.write the file as a whole.
As far I can see, there should be two methods: one that maps and writes the PE file section by section, and one that maps and writes the PE file as a whole. Just like it is on the UEFI PE loader.
If this is too much to handle, it is OK. We can stay with the stack permission adjustment and leave the PE mapping by sections to another time.
Checklist
Which kind of PR do you create?
Coding convention?
Extra tests?
Changelog?
Target branch?
One last thing
When running some pe binaries and having them throwing unhandled exceptions, I noticed that all all the protections are RWX by default.
That could be used to detect qiling from a malware perspective and here's how it can be done: