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

Fixed logic bug in add_to_mem #81

Closed
wants to merge 0 commits into from
Closed

Conversation

astewart-bah
Copy link
Contributor

removing the raise RopException("memory add fails - 1") as it causes good chains to be discarded in my internal examples. If this is necessary for other applications, I am happy to help come up with a better solution.

swapping the for loop and try block fixes a logic error.

As it was, each of the loops calls both _add_mem_with_gadget() and verify(). Both functions can raise multiple exceptions. This means that if the first gadget tried does not work, no more will be tried because any of the exceptions in the _add_mem_with_gadget() and verify() functions will cause the "try" to fail in add_to_mem().

@Kyle-Kyle
Copy link
Collaborator

yeah. the retry loop part is definitely a bug. The patch will address it correctly. I'll happily accept the patch for that part.

But about the patch in the verify function, the removed the logic is the logic that verifies the results of the add_mem chain, without it, it will pass chains that do not add to the memory correctly.
This means, there is something wrong with how we verify the chains.

Actually, I have a guess, is the test case on 64bit architecture? x86_64?

@astewart-bah
Copy link
Contributor Author

It is x86_64.
here is the output of "file" for the binary:

ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 3.2.0, with debug_info, not stripped

@Kyle-Kyle
Copy link
Collaborator

Kyle-Kyle commented Feb 14, 2024

the issue is caused by using the wrong endianness when storing the test value 0x42424242 to memory.
The PR #83 fixes it.
After the PR got merged.
Can you plz rebase the PR so I can merge the fix for add_to_mem?
Thank you in advance!

@Kyle-Kyle
Copy link
Collaborator

FYI, #83 is merged :)
thank you again in advance!

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