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

Add a flatten argument to ssh.libs #2268

Merged
merged 19 commits into from Feb 9, 2024
Merged

Add a flatten argument to ssh.libs #2268

merged 19 commits into from Feb 9, 2024

Conversation

ValekoZ
Copy link
Contributor

@ValekoZ ValekoZ commented Sep 12, 2023

This option makes us able to avoid getting the file tree of the remote server and just downloads the desired files in the output folder:

>>> shell = ssh(host='example.org')
>>> shell.libs('/bin/sh', directory = 'libs', flatten = True)
{'/path/to/cwd/libs/libc.so.6': 22708292161536, '/path/to/cwd/libs/ld-linux-x86-64.so.2': 22708294139904, '/path/to/cwd/libs/dash': 0}

This option makes us able to avoid getting the file tree of the remote
server and just downloads the desired files in the output folder
@peace-maker
Copy link
Member

We should emit a warning on duplicate files when the filename is equal but the remote path was different.

@ValekoZ
Copy link
Contributor Author

ValekoZ commented Sep 12, 2023

Do you think it should just emit a warning and skip the download ? Or should it renames the file to something like {filename}_duplicate and then {filename}_duplicate2, etc. ?

@peace-maker
Copy link
Member

I guess renaming them is better instead of discarding used libraries, but I'd switch to the unflattened structure anyways if this happens. We should log a warning in any case. I don't know how stable the order of the loaded libraries is in ldd and if you can be sure you always want the first matching library on every run.

@peace-maker
Copy link
Member

Can you document that fallback behaviour in the argument description please?

@ValekoZ
Copy link
Contributor Author

ValekoZ commented Sep 14, 2023

Yep, I just did it, sorry I forgot that

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.

Cool, it would be awesome if you could add tests for the function testing withflatten on and off and with and without duplicates.

We're using doctests, check out other examples in the function docs comments of other functions in the file!

pwnlib/tubes/ssh.py Outdated Show resolved Hide resolved
pwnlib/tubes/ssh.py Outdated Show resolved Hide resolved
@Arusekk Arusekk linked an issue Sep 23, 2023 that may be closed by this pull request
@ValekoZ
Copy link
Contributor Author

ValekoZ commented Oct 11, 2023

I just tried to create unit tests for this functionnality, but I was wondering what design did you like better ?
I have the choice between creating minimal ELF files dynamically using lief for creating a binary with 2 libs with the same filename, or just dropping the binaries I created in the repo so that I don't have to add a dependency for the tests

@peace-maker
Copy link
Member

I think adding another small binary to the pwnlib/data/elf folder seems fair. It doesn't have to be runnable I guess, so referencing duplicate not-existing libraries might be fine.

This commit adds:

- The `no_duplicate` binary that depends on
    - `a/lib.so`
    - `b/lib2.so`
- The `duplicate` binary that depends on
    - `a/lib.so`
    - `b/lib.so`

Then, the doctest tries to pull the libs from both binaries with
different `flatten` values.
@ValekoZ
Copy link
Contributor Author

ValekoZ commented Oct 23, 2023

Can you check that I added the tests correctly ? It says all tests passed but I'm not even sure the tests were run .. 😅

@peace-maker
Copy link
Member

I think it's saner to abort on duplicate libraries instead of silently falling back to unflattened output. The locations would be different than what the script expects and it would break anyways.

but I'd switch to the unflattened structure anyways if this happens

I was talking about me using this feature, not how I'd imagine it to behave automatically. Sorry for the confusion.

You can test if the tests are run by adding something obviously broken and see if it breaks.

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, the CI ran now and revealed some problems with your tests. Can you strip the binaries to reduce size please?


Examples:
>>> s = ssh(host='example.pwnme')
>>> s.cwd = f'{pwnlib.data.elf.path}/ssh_libs'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create an __init__.py file and use pwnlib.data.elf.ssh_libs.get("duplicate") etc. below. Check the the __init__.py in one of the other sub folders. The cwd has to be writeable and it apparently isn't in CI, when you change to the pwnlib data directory.

@ValekoZ
Copy link
Contributor Author

ValekoZ commented Dec 6, 2023

Apparently, the elf I pushed are corrupted (I can't run ldd on them), I'll try to fix that ^^

@ValekoZ
Copy link
Contributor Author

ValekoZ commented Dec 6, 2023

Sorry for all the commits, I can't run the tests on my computer (it doesn't work, I don't know why)
I still have an issue and I don't understand why, I stop for today I'll check that later ^^

@peace-maker
Copy link
Member

@Arusekk can you have a look please?

@peace-maker
Copy link
Member

Bleh, I've tried to get the tests to work nicely but don't want to wrestle with -rpath $ORIGIN and friends anymore. I'll add this now and deal with the test again later.

@peace-maker peace-maker merged commit 47dfc57 into Gallopsled:dev Feb 9, 2024
11 of 12 checks passed
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.

Add a flatten argument to ssh.libs
3 participants