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

Error handling in GefRemoteSessionManager.sync #1045

Closed
wants to merge 11 commits into from
71 changes: 47 additions & 24 deletions gef.py
Original file line number Diff line number Diff line change
Expand Up @@ -3551,6 +3551,10 @@ def continue_handler(_: "gdb.ContinueEvent") -> None:

def hook_stop_handler(_: "gdb.StopEvent") -> None:
"""GDB event handler for stop cases."""
if gef.session.remote:
ValekoZ marked this conversation as resolved.
Show resolved Hide resolved
gef.session.remote.maps.unlink()
gef.session.remote.sync(f"/proc/{gef.session.remote.pid}/maps")

reset_all_caches()
gdb.execute("context")
return
Expand Down Expand Up @@ -6045,12 +6049,12 @@ def do_invoke(self, _: List[str], **kwargs: Any) -> None:

dbg(f"[remote] initializing remote session with {gef.session.remote.target} under {gef.session.remote.root}")
if not gef.session.remote.connect(args.pid):
gef.session.remote_initializing = False
ValekoZ marked this conversation as resolved.
Show resolved Hide resolved
raise EnvironmentError(f"Cannot connect to remote target {gef.session.remote.target}")
if not gef.session.remote.setup():
gef.session.remote_initializing = False
ValekoZ marked this conversation as resolved.
Show resolved Hide resolved
raise EnvironmentError(f"Failed to create a proper environment for {gef.session.remote.target}")

gef.session.remote_initializing = False
reset_all_caches()
gdb.execute("context")
return

Expand Down Expand Up @@ -11056,7 +11060,12 @@ def sync(self, src: str, dst: Optional[str] = None) -> bool:
return True
tgt.parent.mkdir(parents=True, exist_ok=True)
dbg(f"[remote] downloading '{src}' -> '{tgt}'")
gdb.execute(f"remote get '{src}' '{tgt.absolute()}'")

try:
gdb.execute(f"remote get '{src}' '{tgt.absolute()}'")
ValekoZ marked this conversation as resolved.
Show resolved Hide resolved
except gdb.error:
return False

return tgt.exists()

def connect(self, pid: int) -> bool:
Expand Down Expand Up @@ -11085,18 +11094,25 @@ def connect(self, pid: int) -> bool:

def setup(self) -> bool:
# setup remote adequately depending on remote or qemu mode
success = True

if self.in_qemu_user():
dbg(f"Setting up as qemu session, target={self.__qemu}")
self.__setup_qemu()
if not self.__setup_qemu():
success = False
else:
dbg(f"Setting up as remote session")
self.__setup_remote()
if not self.__setup_remote():
success = False

# refresh gef to consider the binary
reset_all_caches()
gef.binary = Elf(self.lfile)

if self.file.exists():
gef.binary = Elf(self.lfile)
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


reset_architecture()
return True
return success

def __setup_qemu(self) -> bool:
# setup emulated file in the chroot
Expand Down Expand Up @@ -11133,27 +11149,32 @@ def __setup_qemu(self) -> bool:
return True

def __setup_remote(self) -> bool:
success = True

# get the file
fpath = f"/proc/{self.pid}/exe"
if not self.sync(fpath, str(self.file)):
err(f"'{fpath}' could not be fetched on the remote system.")
return False
if not self.sync(str(self.file)):
warn(f"'{fpath}' could not be fetched on the remote system.")
success = False
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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


# pseudo procfs
for _file in ("maps", "environ", "cmdline"):
fpath = f"/proc/{self.pid}/{_file}"
if not self.sync(fpath):
err(f"'{fpath}' could not be fetched on the remote system.")
return False
if not self.sync(f"/proc/{self.pid}/maps"):
warn(f"'{fpath}' could not be fetched on the remote system.")
success = False
Copy link
Owner

Choose a reason for hiding this comment

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

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[...]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


# makeup a fake mem mapping in case we failed to retrieve it
maps = self.root / f"proc/{self.pid}/maps"
if not maps.exists():
# makeup a fake mem mapping in case we failed to retrieve it
ValekoZ marked this conversation as resolved.
Show resolved Hide resolved
maps = self.root / f"proc/{self.pid}/maps"
with maps.open("w") as fd:
fname = self.file.absolute()
mem_range = "00000000-ffffffff" if is_32bit() else "0000000000000000-ffffffffffffffff"
fd.write(f"{mem_range} rwxp 00000000 00:00 0 {fname}\n")
return True

# pseudo procfs
for _file in ("environ", "cmdline"):
ValekoZ marked this conversation as resolved.
Show resolved Hide resolved
fpath = f"/proc/{self.pid}/{_file}"
if not self.sync(fpath):
warn(f"'{fpath}' could not be fetched on the remote system.")
success = False

return success

def remote_objfile_event_handler(self, evt: "gdb.NewObjFileEvent") -> None:
dbg(f"[remote] in remote_objfile_handler({evt.new_objfile.filename if evt else 'None'}))")
Expand Down Expand Up @@ -11281,7 +11302,10 @@ def target_remote_posthook():

gef.session.remote = GefRemoteSessionManager("", 0)
if not gef.session.remote.setup():
raise EnvironmentError(f"Failed to create a proper environment for {gef.session.remote}")
warn(f"Failed to create a proper environment for {gef.session.remote}")
return False
Comment on lines +11325 to +11326
Copy link
Owner

Choose a reason for hiding this comment

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

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")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)


return True

if __name__ == "__main__":
if sys.version_info[0] == 2:
Expand Down Expand Up @@ -11372,8 +11396,7 @@ def target_remote_posthook():
f"`{disable_tr_overwrite_setting}` in the config.")
hook = f"""
define target hookpost-{{}}
pi target_remote_posthook()
context
pi if target_remote_posthook(): gdb.execute("context")
pi if calling_function() != "connect": warn("{warnmsg}")
end
"""
Expand Down
Loading