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

Extract libraries from Docker image #2479

Merged
merged 5 commits into from
Oct 7, 2024

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.

pwnlib/commandline/template.py Outdated Show resolved Hide resolved
pwnlib/commandline/template.py Outdated Show resolved Hide resolved
pwnlib/commandline/template.py Outdated Show resolved Hide resolved
pwnlib/commandline/template.py Outdated Show resolved Hide resolved
pwnlib/commandline/template.py Outdated Show resolved Hide resolved

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.

pwnlib/commandline/template.py Outdated Show resolved Hide resolved
pwnlib/commandline/template.py Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@lcian lcian force-pushed the feat/template-detect-docker-files branch from ca46f37 to 7086616 Compare October 3, 2024 19:40
@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
14 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