Skip to content

Conversation

@lcian
Copy link
Contributor

@lcian lcian commented Oct 1, 2024

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.

@lcian lcian changed the title feat: extract libraries from Docker image Extract libraries from Docker image Oct 1, 2024
Copy link
Member

@peace-maker peace-maker left a 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.


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]
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@lcian
Copy link
Contributor Author

lcian commented Oct 3, 2024

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 pwn template as well.
If I understand correctly, you would add the compilation from source step directly in the doctests, right?

@lcian lcian requested a review from peace-maker October 3, 2024 21:05
@peace-maker peace-maker merged commit cfc021d into Gallopsled:dev Oct 7, 2024
11 checks passed
@peace-maker
Copy link
Member

Yes, sounds great! Yes in the doctests or some Makefile

peace-maker pushed a commit to peace-maker/pwntools that referenced this pull request Oct 26, 2024
* feat: extract libraries from Docker image

* docs: update CHANGELOG.md

* fix: python2.7 compatibility

* address comments

* address linter
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