-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Extract libraries from Docker image #2479
Extract libraries from Docker image #2479
Conversation
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.
Nice, thank you for tackling this. Docker is available on the Github Runners, so maybe preparing a test folder with a Dockerfile and sourcecode we compile in CI is the way to go to test this? I want to change the existing binaries in pwnlib/data/elf to be compiled from source too. I'm fine to take this without tests and add them later if you don't feel like it.
We can think about migrating this to the new virtualization layer #2434 once it's fully here if applicable.
pwnlib/commandline/template.py
Outdated
|
||
for filename, basename in zip([libc, ld], [libc_basename, ld_basename]): | ||
if is_jailed: | ||
cat_command = ["/bin/sh", "-c", "chroot /srv /bin/sh -c '/bin/cat %s'" % filename] |
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.
Have you considered using docker cp
? It would require to leave the container running briefly but seems cleaner and avoids starting multiple containers quickly.
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.
Yep, I have considered it.
I was thinking that if an exception is thrown after we run the container, we would need to either leave it running or wrap the whole function in a try: ... except Exception
to make sure we clean it up, which I both wanted to avoid.
If you still think it would be better to go with docker cp
, I can implement it that way.
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.
We can optimize if this proves to be too slow.
ca46f37
to
7086616
Compare
Thanks for the review @peace-maker! I have implemented your suggestions. Please let me know your thoughts. About the testing, I think I could add tests in a separate PR with other tests for |
Yes, sounds great! Yes in the doctests or some Makefile |
* feat: extract libraries from Docker image * docs: update CHANGELOG.md * fix: python2.7 compatibility * address comments * address linter
Pwntools Pull Request
Implements #2313.
Supports both normal Dockerfiles as well as the ones using pwn.red/jail, the most common way to sandbox pwn challenges.
Testing
I tested the changes manually on a chall of each kind (no Dockerfile, Dockerfile and Dockerfile using pwn.red/jail as the base image).
I'm open to adding automated tests but would need some input on how to do so, I don't think we want to add random chall binaries to the repo just to test this.