Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds Sigreturn-Oriented Programming (SROP) functionality to the angrop library, enabling users to build ROP chains that leverage the sigreturn system call to set arbitrary register values. The PR introduces two new files implementing the SROP framework and integrates them into the existing chain builder infrastructure.
Changes:
- Added
SigreturnFrameclass to serialize sigreturn frames for multiple architectures (i386, amd64, ARM, MIPS, AARCH64) - Added
SigreturnBuilderclass withsigreturn()andsigreturn_syscall()methods for building SROP chains - Extended architecture definitions with sigreturn syscall numbers for i386 and amd64
- Modified
do_syscall()to support astack_recoverparameter for SROP-specific behavior - Added test coverage for amd64 SROP chains
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| angrop/sigreturn.py | New file implementing SigreturnFrame class with architecture-specific register layouts and serialization |
| angrop/chain_builder/sigreturn.py | New file implementing SigreturnBuilder with methods to construct SROP chains |
| angrop/arch.py | Added sigreturn_num attribute to base class and set values for X86 (0x77) and AMD64 (0xf) |
| angrop/chain_builder/init.py | Integrated SigreturnBuilder and exposed sigreturn/sigreturn_syscall methods, modified do_syscall signature |
| angrop/chain_builder/func_caller.py | Added stack_recover parameter to _func_call to support non-stack-recovering gadgets |
| angrop/chain_builder/sys_caller.py | Propagated stack_recover parameter through do_syscall method |
| tests/test_ropchain.py | Added test_sigreturn_chain_amd64 test case and imported SigreturnFrame |
| docs/pythonapi.md | Added documentation example for sigreturn usage |
| uv.lock | Added uv lock file with Python version requirement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| chain._values = chain._values[:offset_words] | ||
| chain.payload_len = offset_words * self.project.arch.bytes | ||
| elif offset_words < 0: # drop values to fit offset. | ||
| l.warning("Negative offset, %d frame values would be dropped.",-offset_words) |
There was a problem hiding this comment.
The warning message uses an incorrect format. The code has l.warning("Negative offset, %d frame values would be dropped.",-offset_words) but the comma should come after the closing quote and there should be a space after the comma. The correct format should be: l.warning("Negative offset, %d frame values would be dropped.", -offset_words).
| l.warning("Negative offset, %d frame values would be dropped.",-offset_words) | |
| l.warning("Negative offset, %d frame values would be dropped.", -offset_words) |
| self._registers = _REGISTERS[arch_name] | ||
| self._values = {reg: 0 for reg in self._registers.values()} | ||
| self._values.update(_DEFAULTS.get(arch_name, {})) | ||
| self._word_size = 8 if arch_name == "amd64" else 4 |
There was a problem hiding this comment.
The word size determination only handles amd64 as 8 bytes, defaulting all other architectures to 4 bytes. However, aarch64 also uses 8-byte word size. This will cause incorrect frame serialization for aarch64. Consider using a more comprehensive check or a mapping dictionary for word sizes per architecture.
There was a problem hiding this comment.
word_size is just arch_bytes. Can you use that instead? right now it is wrong for other arches like aarch64
| "i386": {"cs": 0x73, "ss": 0x7b}, | ||
| "i386_on_amd64": {"cs": 0x23, "ss": 0x2b}, | ||
| "arm": {"trap_no": 0x6, "cpsr": 0x40000010, "VFPU-magic": 0x56465001, "VFPU-size": 0x120}, | ||
| "mips": {}, |
There was a problem hiding this comment.
Missing default values for mipsel architecture. The _DEFAULTS dictionary has an entry for "mips" (empty dict), but not for "mipsel", which is defined in _REGISTERS and _ARCH_NAME_MAP. This could cause issues when creating sigreturn frames for MIPSEL architecture if default values are needed.
| "mips": {}, | |
| "mips": {}, | |
| "mipsel": {}, |
There was a problem hiding this comment.
can you make sure all the references are cleanly aligned?
| if 0 < offset_words < len(chain._values): # should pad to offset(rsp at syscall) | ||
| chain._values = chain._values[:offset_words] | ||
| chain.payload_len = offset_words * self.project.arch.bytes | ||
| elif offset_words < 0: # drop values to fit offset. | ||
| l.warning("Negative offset, %d frame values would be dropped.",-offset_words) | ||
| frame_words = frame_words[-offset_words:] | ||
| elif offset_words > len(chain._values): | ||
| for _ in range(offset_words - len(chain._values)): | ||
| chain.add_value(filler) |
There was a problem hiding this comment.
Potential logic error: when offset_words equals 0, the frame_words are appended directly without any padding or truncation. However, this case is not explicitly handled in the if-elif chain. While the code will still work (falling through to the final loop), it would be clearer to explicitly handle the offset_words == 0 case or add a comment explaining that no adjustment is needed in this case.
angrop/chain_builder/sigreturn.py
Outdated
| frame.update(**registers) | ||
|
|
||
| syscall_num = self.arch.sigreturn_num # syscall(sigreturn) | ||
| chain = self.chain_builder.do_syscall(syscall_num, [],stack_recover=False, needs_return=False) # dummy args |
There was a problem hiding this comment.
Missing space after comma in function call. The code has [] followed immediately by stack_recover without a space after the comma.
| chain = self.chain_builder.do_syscall(syscall_num, [],stack_recover=False, needs_return=False) # dummy args | |
| chain = self.chain_builder.do_syscall(syscall_num, [], stack_recover=False, needs_return=False) # dummy args |
|
This srop module looks amazing! I would love to merge it! But I have a few comments:
|
|
also, maybe we want to print |
the usage for |
|
Ah. I see what's going on! this should be placed before the chain add_value logic. Because if we don't need to return, then why do we specify a |
Ok, I'll merge the needs_return and stack_recover together and fix this. |
I'm implementing this so please merge later (? |
sure. let me know when you are ready :D |
Done. Now: >>> chain = rop.sigreturn_syscall(0x3b, [next(elf.search(b"/bin/sh\x00")), 0, 0])
>>> chain.pp()
0x000000000040017b: push rax; push rbp; mov rbp, rsp; mov eax, 0xf; syscall ; nop ; pop rbp; ret
<SigreturnFrame for amd64>
rdi : 0x00000000004001ca
rax : 0x000000000000003b
rip : 0x0000000000400185
csgsfs : 0x0000000000000033Oh, sth went wrong |
|
Your testcase failure is caused by the badbyte tests. The badbyte write scheduling algorithm is not optimal, which caused it to exhaustively check usable gadgets. And eventually find a weird gadget that triggers a bug in angrop. |
|
Sometime this testcase could run successfully but sometime it fails. It's a weird one. |
|
I've rebased the commit now. |
|
oops, failed again :( |
usage: