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.

Sign up for GitHub

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

Jump to bottom

[target-remote] Basic support for the target remote command #1020

Merged
merged 8 commits into from Dec 16, 2023
Merged

[target-remote] Basic support for the target remote command #1020

merged 8 commits into from Dec 16, 2023

Conversation

ValekoZ
Copy link
Contributor

@ValekoZ ValekoZ commented Dec 6, 2023

Description

As mentioned in Gallopsled/pwntools#2264, gef does not work properly with many tools that rely on the target remote command.
In this PR, I propose a fix that uses a remote posthook in order to instantiate and setup the GefRemoteSessionManager after the connection being established.

Note that this isn't a perfect solution since we do not have all the information needed for a proper instantiation of the GefRemoteSessionManager, but it seems to be a good workaround in order to make tools like pwntools work correctly with gef.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 6, 2023

🤖 Coverage Update

  • Commit: 295cbf7
  • Current Coverage: 71.4300%
  • New Coverage: 71.4300%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Grazfather
Grazfather reviewed Dec 7, 2023
View reviewed changes
Copy link
Collaborator

@Grazfather Grazfather left a comment

Choose a reason for hiding this comment

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

Good idea. We will want to remove the warning when using target remote

gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
@ValekoZ
Copy link
Contributor Author

ValekoZ commented Dec 7, 2023 •
edited

I'm not sure if we really want to remove the warning, since the initialization is partial (we don't initialize the host and port fields). Those fields are actually not used in the gef core, but if a plugin or a user wants to use them, it would probably fail :/
Btw, I just noticed that the warning is displayed before we refresh the context, so we don't see it. Please tell me if you really want to delete it or if you want me to move it after the context command 😄

@ValekoZ
Copy link
Contributor Author

ValekoZ commented Dec 7, 2023

Oh and btw, do you want me to implement the same thing for target extended-remote ? I do not really know what this does, but I guess it is not that different ? I can try to check if this seems feasible

ValekoZ added a commit to ValekoZ/gef that referenced this pull request Dec 7, 2023
@ValekoZ
[target-remote] Comment the code added by the PR
33d927e 
Fix for hugsy#1020 (comment)
ValekoZ added a commit to ValekoZ/gef that referenced this pull request Dec 7, 2023
@ValekoZ
[target-remote] Fix: Use double quotes instead of single quotes
e157cfe 
Fix for hugsy#1020 (comment)
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 7, 2023

🤖 Coverage Update

  • Commit: 295cbf7
  • Current Coverage: 71.4300%
  • New Coverage: 71.4300%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

@Grazfather
Copy link
Collaborator

  1. Sure, we can move it, and adjust it, say that it's best effort or something.
  2. Extended remote is just remote with more abilities, such as restarting the target and reconnecting to the server.
  3. Adding it for that would be great.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 7, 2023

🤖 Coverage Update

  • Commit: 295cbf7
  • Current Coverage: 71.4300%
  • New Coverage: 71.4300%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

@ValekoZ
Copy link
Contributor Author

ValekoZ commented Dec 10, 2023

I'm not sure if I broke the pipeline ? The errors do not seem to be related to my code, but apparently it appeared just after my last commit ?

@Grazfather
Copy link
Collaborator

CI is broken because of it installing python3.12 and that changing some stuff. We will get it sorted.

Sorry for the slow work on this PR. Holiday time can be busy.

@Grazfather
Copy link
Collaborator

If you rebase onto main after #1022 the tests should pass.

@github-actions GitHub Actions
Copy link

🤖 Coverage Update

  • Commit: 15b09cf
  • Current Coverage: 71.4300%
  • New Coverage: 71.4300%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

hugsy
hugsy requested changes Dec 14, 2023
View reviewed changes
Copy link
Owner

@hugsy hugsy left a comment •
edited

Choose a reason for hiding this comment

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

Hi @ValekoZ

This look just great to me! Thanks for the contribution!

Do you mind adding a simple test (for instance in https://github.com/hugsy/gef/blob/main/tests/commands/gef_remote.py) for it? It'll help making sure we don't break anything in the future.
Also a note in the docs mentioning we override the native target commands. I'm normally not a fan of overriding native commands, but I think it's fair that target remote is special. So just having a doc explaining it gives us a quick answer when people asks why it acts differently.

After that, it'll be good for merging.

@ValekoZ
Copy link
Contributor Author

ValekoZ commented Dec 14, 2023

Maybe we should add an option to disable the post-hook for those who want the default behavior ?

@github-actions GitHub Actions
Copy link

🤖 Coverage Update

  • Commit: 15b09cf
  • Current Coverage: 71.4300%
  • New Coverage: 71.4300%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

@hugsy
Copy link
Owner

hugsy commented Dec 14, 2023 •
edited

Maybe we should add an option to disable the post-hook for those who want the default behavior ?

Probably a good idea now that you mention it.
Also CI validation is failing, need an empty line in gef-remote.py

Don't worry about the error from the coverage message, seems like a Github issue.

@github-actions GitHub Actions
Copy link

🤖 Coverage Update

  • Commit: 15b09cf
  • Current Coverage: 71.4300%
  • New Coverage: 71.4300%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

hugsy
hugsy approved these changes Dec 16, 2023
View reviewed changes
Copy link
Owner

@hugsy hugsy left a comment •
edited

Choose a reason for hiding this comment

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

👍 , tests are passing too (ignoring the CI failure, my fault)
LGTM, thanks for your patience @ValekoZ

I think it's ready to merge, any pending request on your end @Grazfather ?

@Grazfather Grazfather merged commit f7a2105 into hugsy:main Dec 16, 2023
5 of 6 checks passed
@hugsy hugsy added this to the 2024.01 milestone Dec 22, 2023
@hugsy hugsy mentioned this pull request Feb 16, 2024
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Grazfather Grazfather Grazfather left review comments

@hugsy hugsy hugsy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2024.01
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ValekoZ @Grazfather @hugsy