Skip to content

SSH: fix download/upload with -1 exit status #2545

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

Merged
merged 2 commits into from
Mar 30, 2025

Conversation

kilimnik
Copy link
Contributor

Pwntools Pull Request

Thanks for contributing to Pwntools! Take a moment to look at CONTRIBUTING.md to make sure you're familiar with Pwntools development.

Please provide a high-level explanation of what this pull request is for.

The paramiko recv_exit_status function returns -1 is no return code is received, this can happen because the ssh server did not send an exit-status request.
The RFC 4254 states this request is recommended but not required, so an SSH Server would be working correctly but the upload and download functions of pwntools would fail.

I'm not sure if you consider this a bug and against which branch you want this targeted.

Testing

Pull Requests that introduce new code should try to add doctests for that code. See TESTING.md for more information.

Target Branch

Depending on what the PR is for, it needs to target a different branch.

You can always change the branch after you create the PR if it's against the wrong branch.

Branch Type of PR
dev New features, and enhancements
dev Documentation fixes and new tests
stable Bug fixes that affect the current stable branch
beta Bug fixes that affect the current beta branch, but not stable
dev Bug fixes for code that has never been released

Changelog

After creating your Pull Request, please add and push a commit that updates the changelog for the appropriate branch.
You can look at the existing changelog for examples of how to do this.

@peace-maker
Copy link
Member

Interesting. Do you see value in issuing a log.warn_once message to inform the user that there is some uncertainty in the success of the operation? Silently swallowing errors feels annoying to debug later if it's unexpected.

@kilimnik
Copy link
Contributor Author

I think that makes sense, yes, I'll add them

@peace-maker
Copy link
Member

Can you rebase and target the stable branch instead please? Sorry for not noticing your question earlier. We're not as strict about the target branches for PRs as we should be to allow better patch releases, but we can try to be good boys this time!

@kilimnik kilimnik changed the base branch from dev to stable March 29, 2025 09:12
@kilimnik
Copy link
Contributor Author

Sorry it took so long 😄

@peace-maker
Copy link
Member

Nice, thank you! CI is confused, sec.

@peace-maker peace-maker reopened this Mar 30, 2025
@peace-maker peace-maker enabled auto-merge (squash) March 30, 2025 10:13
@peace-maker peace-maker merged commit b60b2e8 into Gallopsled:stable Mar 30, 2025
23 of 24 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.

2 participants