[FIX #133] Various exception handler fixes for stability

[calebstewart]

Description of Changes

Fixes #133.

Added better exception handling in Manager.interactive which should now withstand dropped sessions.

Major Changes Implemented:

• Improved exception handling in across the framework.

Pre-Merge Tasks

• Formatted all modified files w/ python-black
• Sorted imports for modified files w/ isort
• Ran flake8 on repo, and fixed any new problems w/ modified files
• Ran pytest test cases
• Added brief summary of updates to CHANGELOG (under [Unreleased])

For issues with pre-merge tasks, see CONTRIBUTING.md

[calebstewart]

@Mitul16 if you're interested, want to give this a go and see if it fixes the uncaught exceptions you were seeing before? It seems to fix the ones I could reproduce, but I'm curious to see from your perspective. Thanks!

[Mitul16]

It doesn't look like the fix is in action, please do let me know. I would like to test it for sure.
😅

[calebstewart]

Sorry for the back and forth. There was a couple cases I missed from your report in the issue. I believe the last push covers all the bases, and I'd really appreciate your feedback now 😄

[Mitul16]

I think the following issue comes under current Issue - exception handling...

If I remember correctly, there was once a check in the module upload.py checking for write access to the destination path
If we try to upload a file to a destination to which the current session.current_user() doesn't have access to, then pwncat hangs and we have to kill the current instance

pwncat v0.3.1 - ./pwncat/commands/upload.py

60             access = pwncat.victim.access(args.destination)
61             if Access.DIRECTORY in access:
62                 args.destination = os.path.join(
63                     args.destination, os.path.basename(args.source)
64                 )
65             elif Access.PARENT_EXIST not in access:
66                 console.log(
67                     f"[cyan]{args.destination}[/cyan]: no such file or directory"
68                 )
69                 return

NOTE: the file to be uploaded should be more than the buffer size (2048 or 4096, or whatever is in use)
EDIT: the value being used is 1048576 (1MiB, taken from ./pwncat/data/gtfobins.json)

[calebstewart]

I think the following issue comes under current Issue - exception handling...

If I remember correctly, there was once a check in the module upload.py checking for write access to the destination path
If we try to upload a file to a destination to which the current session.current_user() doesn't have access to, then pwncat hangs and we have to kill the current instance

pwncat v0.3.1 - ./pwncat/commands/upload.py

60             access = pwncat.victim.access(args.destination)
61             if Access.DIRECTORY in access:
62                 args.destination = os.path.join(
63                     args.destination, os.path.basename(args.source)
64                 )
65             elif Access.PARENT_EXIST not in access:
66                 console.log(
67                     f"[cyan]{args.destination}[/cyan]: no such file or directory"
68                 )
69                 return

NOTE: the file to be uploaded should be more than the buffer size (2048 or 4096, or whatever is in use)

Yes, that used to be a thing, and was removed because I thought the Popen wrapper would catch the failed process, but I was obviously wrong. I've added back in the permission checks in Linux.open to ensure the files are readable/writable. I also needed to implement a custom LinuxPath class to handle readable() and writable() differently, since we can't enumerate users/groups, because those enumerations open files which causes an infinite loop. Instead, we use cached results from the id command to identify privileges to reading/writing.

[Mitul16]

Well, now it is breezing 😀

I have tested the following for an invalidated session :

• Switching to and from the remote terminal
• Up/Downloading a file
• Running an enumerate, escalate module

None of the above renders the pwncat instance to exit abruptly
Although I did manage to fix some exceptions myself 😅
But you did it much better, thanks a lot.

There is one little issue, I tried to upload rockyou.txt file and then closed the session from the victim machine
The good part is that pwncat didn't exit, but the newly invalidated sessions are still present and do not dispose when trying Ctrl-D to go to the remote terminal instead we get channel not connected and some traceback

[calebstewart]

Well, now it is breezing

I have tested the following for an invalidated session :

• Switching to and from the remote terminal
• Up/Downloading a file
• Running an enumerate, escalate module

None of the above renders the pwncat instance to exit abruptly
Although I did manage to fix some exceptions myself
But you did it much better, thanks a lot.

There is one little issue, I tried to upload rockyou.txt file and then closed the session from the victim machine
The good part is that pwncat didn't exit, but the newly invalidated sessions are still present and do not dispose when trying Ctrl-D to go to the remote terminal instead we get channel not connected and some traceback

Cool, I was able to replicate that pretty easily. The problem was that the finally: block was trying to close the Popen object, which caused a chain of ChannelClosed -> ChannelError. The ChannelError was caused by calling send with a closed channel. Instead, I changed the base Socket class to raise another ChannelClosed exception instead of a generic ChannelError in that case. This makes it so that the error propagates properly and closes the session.

[Mitul16]

@calebstewart I have a doubt, why have you used ChannelError(ERROR_MESSAGE) in some places instead of ChannelError(CHANNEL, ERROR_MESSAGE)?

In ./pwncat/channel/__init__.py, we have the ChannelError constructor like so

 35     def __init__(self, ch, msg="generic channel failure"):
 36         super().__init__(msg)
 37         self.channel = ch

Even in the classes extending the super class ChannelError, the constructor for ChannelError is called as

def __init__(self, ch):
    super().__init__(ch, CORRESPONDING_ERROR_MESSAGE)

The places where you have ChannelError(ERROR_MESSAGE) will not raise any exceptions as there is a default argument in its constructor because of this any ERROR_MESSAGE that you use (without passing a CHANNEL) is simply not shown, generic channel failure is shown instead.

[Mitul16]

Regarding my last comment, I have added a commit to the branch issue-133-uncaught-channelerror
If you haven't checked these issues yourself, I can create a PR

[calebstewart]

@calebstewart I have a doubt, why have you used ChannelError(ERROR_MESSAGE) in some places instead of ChannelError(CHANNEL, ERROR_MESSAGE)?

In ./pwncat/channel/__init__.py, we have the ChannelError constructor like so

 35     def __init__(self, ch, msg="generic channel failure"):
 36         super().__init__(msg)
 37         self.channel = ch

Even in the classes extending the super class ChannelError, the constructor for ChannelError is called as

def __init__(self, ch):
    super().__init__(ch, CORRESPONDING_ERROR_MESSAGE)

The places where you have ChannelError(ERROR_MESSAGE) will not raise any exceptions as there is a default argument in its constructor because of this any ERROR_MESSAGE that you use (without passing a CHANNEL) is simply not shown, generic channel failure is shown instead.

You're correct. That appears to be a typo, which would eventually cause an exception as the channel reference is used to cleanup disconnected channels on exception.

[calebstewart]

Regarding my last comment, I have added a commit to the branch issue-133-uncaught-channelerror
If you haven't checked these issues yourself, I can create a PR

I had not seen it. I went and found it. You can create a pull request if you'd like. I will say I don't agree with catching the ChannelError in create_session. These methods aren't front-end calls. They are APIs, and the exception should be propagated to the caller.

[calebstewart]

On second thought, I can just merge your changes into my branch. There's no need for a formal pull request. It's all working toward the same goal anyway.

[Mitul16]

Ah! You are back. I was testing ChannelError specifically and have found some exceptions
Here is one example that you can try

# In `pwncat` terminal prompt, password is `bandit0`
connect ssh://bandit0@bandit.labs.overthewire.org:2220

You may find that getpeername is not defined for the class Channel

[Mitul16]

Moreover, I wasn't sure upto how many parent layers (methods) to bring the error, I did find create_session a good place to catch the exception

[calebstewart]

Ah! You are back. I was testing ChannelError specifically and have found some exceptions
Here is one example that you can try

# In `pwncat` terminal prompt, password is `bandit0`
connect ssh://bandit0@bandit.labs.overthewire.org:2220

You may find that getpeername is not defined for the class Channel

That was a bug in the exception handler that slipped through the cracks for a long time. Good catch. Should be fixed in this branch now. Also, I'm pretty sure we're on different time zones 🤣

Moreover, I wasn't sure upto how many parent layers (methods) to bring the error, I did find create_session a good place to catch the exception

Totally understand your logic, but I would say the create_session method is the "entrypoint" call for the API, and should let the exceptions through. It should raise either a PlatformError indicating that something is wrong with the platform or a ChannelError indicating that something went wrong setting up the channel.

In any case, I'll get your changes merged in a moment.

[Mitul16]

This is the reason I didn't create a PR because of the way I have implemented this exception-handling
There is no need to merge from my branch, please take your time to implement the same with a valid approach

[calebstewart]

This is the reason I didn't create a PR because of the way I have implemented this exception-handling
There is no need to merge from my branch, please take your time to implement the same with a valid approach

Not a big deal. I have your fork added as a separate remote to my local clone, so I can easily merge your branch, then make the one slight modification and push back here. I'd prefer that, so that your contributions aren't lost in the git history. 👍

I've done that, and we should be good at this point at least with the exception problems we've found so far. Hopefully there aren't any more. 😭

[calebstewart]

Are you still seeing any spurious or weird exception issues or working on anything related to this PR? If not, I will merge this branch back into master.

[Mitul16]

EDIT: I didn't refresh the page to see your question

Yes, I did find one but it is not related to connections but file I/O instead
Using an invalid identity file for ssh protocol, gives rise (raise) to FileNotFoundError. This should easily be fixed as

./pwncat/channel/ssh.py

 56         if identity is not None:
 57             # Make sure the provided identity file is good to use
 58             if not os.path.exists(identity):
 59                 raise ChannelError(self, f"identity file `{identity}` not found!")
 60             elif os.access(identity, os.R_OK):
 61                 raise ChannelError(self, f"cannot read identity file `{identity}`")

Unfortunately there are a little more, but these are not related to ChannelError or Channel in particular
Try back in pwncat terminal prompt 😅

[calebstewart]

The back command was super simple fix, so I went ahead and just fixed that. It was raising the wrong exception type.

Regarding the SSH stuff, I'm going to say that should be a different issue. The whole setup process for SSH channels should be changing soon (see #122 and #91), and that exception doesn't actually break anything. It's just not a pleasant message for an actual exceptional condition. Plus this PR is getting pretty hefty as it is. lol

[Mitul16]

Yeah, I should have created a separate Issue here or posted the information on exisiting issues (#122 and #91) if applicable

[calebstewart]

Yeah, I should have created a separate Issue here or posted the information on exisiting issues (#122 and #91) if applicable

No problem. If/when you are comfortable that the exception issues are fixed, please mark this PR as reviewed, and then I'll merge. This and #140 are the two things I'd like to get merged before dropping v0.4.3. They're big fixes that should be ideally released ASAP. Thanks again for your help on this! These two issues have been huge steps forward in the stability of pwncat. 😄

[Mitul16]

Just a quick question, how are you trying to catch the ChannelError given that you have modified the create_session method to go hand-in-hand with the API?

I went ahead to test my modifications - two new exceptions raised in ./pwncat/channel/connect.py

48             try:
49                 client = socket.create_connection((host, port))
50             except socket.gaierror:
51                 raise ChannelError(self, "invalid host provided")
52             except ConnectionRefusedError:
53                 raise ChannelError(self, "connection refused, check your port")

EDIT: I can see that they are not handled as of now, I am using issue-133-uncaught-channelerror

[calebstewart]

Just a quick question, how are you trying to catch the ChannelError given that you have modified the create_session method to go hand-in-hand with the API?

I went ahead to test my modifications - two new exceptions raised in ./pwncat/channel/connect.py

48             try:
49                 client = socket.create_connection((host, port))
50             except socket.gaierror:
51                 raise ChannelError(self, "invalid host provided")
52             except ConnectionRefusedError:
53                 raise ChannelError(self, "connection refused, check your port")

Whatever calls create_session should be prepared to catch any exceptions it raises. It can raise ChannelError or PlatformError exceptions. In the standard entrypoint, these exceptions are caught at __main__.py:278 and a "connection failed" message is printed appropriately.

For the connect command used from the local prompt, you are correct. They are not being caught. I'll update that now.

[Mitul16]

Yes, because it is being executed from ./pwncat/commands/__init__.py instead

Various exceptions... 😅

[Mitul16]

Ok, I have reviewed all the changes except pwncat/manager.py.
There are a lot of modifications and I have not tested each and every little change.
Everything looks fine to me, I will try to fuzz pwncat/manager.py

[Mitul16]

NOTE: I had a temporary fix for back command in my master branch and I have removed that commit.
You may get into issues with local clone of my fork

[calebstewart]

Myself and @JohnHammond have been testing this all evening, and think it's ready for merging. I'm going to go ahead and merge this in as I feel like random bug fixes are getting tossed in (e.g. I just fixed a Windows problem that is only loosely related). The pull request is diverging more than I'd like. If there's more problems, feel free to open another issue and we'll work through it.