Error handling in `GefRemoteSessionManager.sync` #1045

ValekoZ · 2024-01-12T20:28:33Z

Description

def sync(self, src: str, dst: Optional[str] = None) -> bool: should not raise an error when failing to download a file, but return false instead.

Then, error handling should be managed better in functions that call this sync function

This issue was found while reproducing #1044

Before

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/dave/gef.py", line 11283, in target_remote_posthook
    if not gef.session.remote.setup():
  File "/dave/gef.py", line 11093, in setup
    self.__setup_remote()
  File "/dave/gef.py", line 11138, in __setup_remote
    if not self.sync(fpath, str(self.file)):
  File "/dave/gef.py", line 11059, in sync
    gdb.execute(f"remote get '{src}' '{tgt.absolute()}'")
gdb.error: Remote I/O error: No such file or directory
Error while executing Python code.

After

[!] '/proc/36123/exe' could not be fetched on the remote system.
[!] Failed to create a proper environment for RemoteSession(target=':0', local='/tmp/tmp6gn18rer', pid=36123, qemu_user=False)

Checklist

My code follows the code style of this project.
My change includes a change to the documentation, if required.
If my change adds new code, adequate tests have been added.
I have read and agree to the CONTRIBUTING document.

… env

In case of failure, we still want to initialize every possible thing to still provide the best user experience as possible. The only consequences of an early return here is to make everything un-initialized, even if we could init other things after the return

hugsy

Good PR @ValekoZ Thanks! I like your approach to re-use the existing tbh so I'll wait for your changes before merging the one I just made (#1046 and #1047)

gef.py

hugsy · 2024-01-14T02:13:04Z

gef.py

+        if self.file.exists():
+            gef.binary = Elf(self.lfile)


This is problematic as it will prevent any function/command depending on gef.binary for instance checksec) to work at all. Are you doing this check because of the support for rr? If so, we probably still need the way I mentioned to detect we're running rr so that we can gracefully handle that case.
I'm not super sure the best way about this, but that if is definitely problematic.

This is not related to rr, actually this has never failed in my tests, but raising an exception in the setup of the session would mean that the session would be running (since it is actually managed by gdb itself) but the gef session's wrapper would be completely broken.

I think having some commands not working (like checksec) is better than having nothing working.

But I agree with you that something needs to be done to prevent having a "bad" fail when running those commands, and instead having a clean error message

hugsy · 2024-01-14T02:18:12Z

gef.py

+        if not self.sync(str(self.file)):
+            warn(f"'{fpath}' could not be fetched on the remote system.")
+            success = False


I think here is a good reason to throw: not having the executable will be very problematic (as explained below) and hard to recover from.

Suggested change

if not self.sync(str(self.file)):

warn(f"'{fpath}' could not be fetched on the remote system.")

success = False

if not self.sync(str(self.file)):

raise FileNotFoundError

#1045 (comment)

hugsy · 2024-01-14T02:22:35Z

gef.py

+        if not self.sync(f"/proc/{self.pid}/maps"):
+            warn(f"'{fpath}' could not be fetched on the remote system.")
+            success = False


Suggested change

if not self.sync(f"/proc/{self.pid}/maps"):

warn(f"'{fpath}' could not be fetched on the remote system.")

success = False

try:

self.sync(f"/proc/{self.pid}/maps")

except gdb.error as e:

warn(f"sync('{fpath}' failed, reason: {str(e)}")

[...] setup the mock /proc/maps here[...]

Waiting for what you think about this before changing anything :)
#1045 (comment)

gef.py

hugsy · 2024-01-14T02:27:40Z

gef.py

+        warn(f"Failed to create a proper environment for {gef.session.remote}")
+        return False


Raising is better here too, especially since we're in a callback function. Failing setup is not something easily recoverable.

Also need to None the session.

Suggested change

warn(f"Failed to create a proper environment for {gef.session.remote}")

return False

gef.session.remote = None

raise EnvironmentError("Failed to create a remote environment")

This function is directly called from gdb:

pi if target_remote_posthook(): ...

iirc raising an exception here creates a full stack trace in the gdb console.
If this fails, there are many warning that are raised and that helps to know what happened, without disturbing the user (we actually want to warn him that everything will not work as intended, but since we try our best to make everything work as good as possible we don't want to print errors and stacktraces in most situations)

gef.py

I still think we don't want to raise anything here, but there was a reset missing :)

Even if we don't raise an exception (still discussing about it) we need to manage properly the cases were the binary was not setup.

github-actions · 2024-01-30T23:21:17Z

🤖 Coverage update for `814e198` 🔴

	Old	New
Commit	`b56bf9d`	`814e198`
Score	71.5816%	71.5623% (-0.0193)

gef.py

Grazfather · 2024-01-31T00:31:32Z

gef.py

+            with environ.open("wb") as fd:
+                fd.write(b"PATH=/bin\x00HOME=/tmp\x00")
+
+        fpath = f"/proc/{self.pid}/cmdline"


IT would be nice if we could just tell gef to use a buffer instead of having to write stuff to disk when faking stuff. It would be even nicer if we didn't have to fake it.

- hugsy#1045 (comment) - hugsy#1045 (comment) - hugsy#1045 (comment) - hugsy#1045 (comment)

github-actions · 2024-02-20T10:19:16Z

🤖 Coverage update for `47bb65c` 🟢

	Old	New
Commit	`b56bf9d`	`47bb65c`
Score	71.7996%	71.7996% (0)

hugsy · 2024-03-10T17:45:17Z

Catching up on this

@ValekoZ there's been many changes since this PR (which now make a conflict with gef in main). Can you update this PR (or close if doesn't apply anymore) ?

Thanks!

ValekoZ · 2024-04-02T08:05:12Z

Most of the work seems to have been rewritten in other PRs, we can close this one :)

ValekoZ added 4 commits January 12, 2024 20:55

Catch fetch error to handle it correctly

eac7308

Check if setup of qemu/remote worked before continuing setting up the…

b694a81

… env

Better error handling in calling functions

b36d907

ValekoZ force-pushed the fetch-error branch from bbbdbc6 to 68036b2 Compare January 12, 2024 23:11

ValekoZ added 2 commits January 13, 2024 00:15

Remove debug print

54c818b

Resync maps at each stops

129f8ab

hugsy requested changes Jan 14, 2024

View reviewed changes

hugsy requested a review from Grazfather January 14, 2024 02:32

ValekoZ added 4 commits January 31, 2024 00:12

Fix hugsy#1045 (comment)

cec56e1

Fix hugsy#1045 (comment)

7265976

About hugsy#1045 (comment)

98519c9

I still think we don't want to raise anything here, but there was a reset missing :)

Fix hugsy#1045 (comment)

814e198

Even if we don't raise an exception (still discussing about it) we need to manage properly the cases were the binary was not setup.

Grazfather reviewed Jan 31, 2024

View reviewed changes

Fix typos

47bb65c

- hugsy#1045 (comment) - hugsy#1045 (comment) - hugsy#1045 (comment) - hugsy#1045 (comment)

ValekoZ closed this Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error handling in `GefRemoteSessionManager.sync` #1045

Error handling in `GefRemoteSessionManager.sync` #1045

ValekoZ commented Jan 12, 2024 •

edited

Loading

hugsy left a comment

hugsy Jan 14, 2024

ValekoZ Jan 30, 2024

hugsy Jan 14, 2024

ValekoZ Jan 30, 2024

hugsy Jan 14, 2024

ValekoZ Jan 30, 2024

hugsy Jan 14, 2024

ValekoZ Jan 30, 2024

github-actions bot commented Jan 30, 2024

Grazfather Jan 31, 2024

github-actions bot commented Feb 20, 2024

hugsy commented Mar 10, 2024

ValekoZ commented Apr 2, 2024

		warn(f"Failed to create a proper environment for {gef.session.remote}")
		return False

Error handling in GefRemoteSessionManager.sync #1045

Error handling in GefRemoteSessionManager.sync #1045

Conversation

ValekoZ commented Jan 12, 2024 • edited Loading

Description

Before

After

Checklist

hugsy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 30, 2024

🤖 Coverage update for 814e198 🔴

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2024

🤖 Coverage update for 47bb65c 🟢

hugsy commented Mar 10, 2024

ValekoZ commented Apr 2, 2024

Error handling in `GefRemoteSessionManager.sync` #1045

Error handling in `GefRemoteSessionManager.sync` #1045

ValekoZ commented Jan 12, 2024 •

edited

Loading

🤖 Coverage update for `814e198` 🔴

🤖 Coverage update for `47bb65c` 🟢