-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Add a "got-audit" command to look for signs of GOT tampering #1094
Conversation
I have not yet added images referenced in the documentation; those links still point at images referenced in the "got" documentation. I'll add images and squash commits before you merge, if you decide to accept this patch. |
@gordonmessmer |
I've started work on that, as well as the additional packages that will be needed to ship this in Fedora. In the meantime, are there changes you want to see in the command or its implementation? |
gef.py
Outdated
@@ -4661,8 +4661,8 @@ def add_setting(self, name: str, value: Tuple[Any, type, str], description: str | |||
|
|||
def __setitem__(self, name: str, value: Union[Any, Tuple[Any, str]]) -> None: | |||
# make sure settings are always associated to the root command (which derives from GenericCommand) | |||
if "GenericCommand" not in [x.__name__ for x in self.__class__.__bases__]: | |||
return | |||
#if "GenericCommand" not in [x.__name__ for x in self.__class__.__bases__]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually forgotten to look at that closer before sending the PR...
self.__class__.__bases__
only contains the immediate parent classes, and that meant that GotAuditCommand wouldn't have any settings because GenericCommand isn't in that list. I think I can revert this, and just add GenericCommand as a second parent class in GotAuditCommand (which will look super weird to future maintainers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, GotAuditCommand doesn't provide an __init__
, so GotCommand's runs. GotCommand's __init__
should set values for function_resolved and function_not_resolved, but those don't actually get set because GenericCommand isn't an immediate parent of the class that's actually being instantiated, and the instance ends up with no settings at all.
I'm not sure I understand the intent of this check, and maybe checking isinstance() instead would be better?
gef.py
Outdated
# retrieve symbols using nm | ||
lines = gef_execute_external([nm, "-D", elf_file], as_list=True) | ||
for line in lines: | ||
words = line.split(' ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
words = line.split(' ') | |
words = line.split() |
gef.py
Outdated
words = line.split(' ') | ||
# Record the symbol if it is in the text section or | ||
# an indirect function or weak symbol | ||
if words[-2] in ('T', 'i', 'I', 'v', 'V', 'w', 'W'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might trigger an IndexError
if there's any change to the nm
output
you need to add some extra validations of the output
gef.py
Outdated
_cmdline_ = "got-audit" | ||
_syntax_ = f"{_cmdline_} [FUNCTION_NAME ...] " | ||
_example_ = "got-audit read printf exit" | ||
_symbols_ = collections.defaultdict(list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add specific type hints
gef.py
Outdated
_syntax_ = f"{_cmdline_} [FUNCTION_NAME ...] " | ||
_example_ = "got-audit read printf exit" | ||
_symbols_ = collections.defaultdict(list) | ||
_paths_ = collections.defaultdict(list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too
gef.py
Outdated
if elf_file not in self._symbols_[sym]: | ||
self._symbols_[sym].append(elf_file) | ||
self._paths_[elf_file].append(sym) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing whitespace, you need to apply pre-commit run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may need to re-visit that... seems pre-commit doesn't work correctly under Python 3.12, and I haven't found a fix yet.
gef.py
Outdated
# Build a list of the symbols provided by each path, and | ||
# a list of paths that provide each symbol. | ||
for section in gef.memory.maps: | ||
if (hasattr(section, "path") and section.path not in self._paths_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Section
always has the path
attribute, so this check will always return true. This part should be necessary
gef.py
Outdated
for section in gef.memory.maps: | ||
if (hasattr(section, "path") and section.path not in self._paths_ | ||
and pathlib.Path(section.path).is_file() | ||
and Permission.EXECUTE in section.permission): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and Permission.EXECUTE in section.permission): | |
and section.permission & Permission.EXECUTE): |
gef.py
Outdated
self.get_symbols_from_path(section.path) | ||
return super().do_invoke(argv) | ||
|
||
def build_line(self, name, color, address_val, got_address): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing type hints
gef.py
Outdated
line = Color.colorify(f"{name}", color) | ||
found = 0 | ||
for section in gef.memory.maps: | ||
if section.contains(got_address) and hasattr(section, "path"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we try usually go the "never nester" type of approach, i.e. test what's failing first and exit asap. so here that'd be
if section.contains(got_address) and hasattr(section, "path"): | |
if not section.contains(got_address): | |
continue |
gef.py
Outdated
@@ -10979,7 +11039,7 @@ def __init__(self) -> None: | |||
self.aliases: List[GefAlias] = [] | |||
self.modules: List[FileFormat] = [] | |||
self.constants = {} # a dict for runtime constants (like 3rd party file paths) | |||
for constant in ("python3", "readelf", "file", "ps"): | |||
for constant in ("python3", "readelf", "nm", "file", "ps"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to add nm
as a binary deps in docs/install.md
as well
tests/commands/got_audit.py
Outdated
@@ -0,0 +1,42 @@ | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job adding those tests and documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code itself looks ok, some minor improvements.
Once those fixed and moved to gef-extras, it'll be merged quickly
I think that takes care of the changes you've requested, but I have a minor snag with moving the command to gef-extras. This audit needs to be run under This PR still isn't ready to merge, because the docs refer to images of the old "got" command. But, would it be possible to merge the got-audit command into gef and then continue with additional work to make gef-extras work correctly in batch mode and then move the got-audit command from gef to gef-extras? |
I am fixing this! |
Is there an issue or PR I can follow for the generate_glibc_args_json.py changes? |
Description
The "got-audit" command examines the symbols in the GOT, and prints a list of symbols and the path of the file that provides the mapped memory that the value points to. Additionally, it will print errors if a symbol is provided by multiple shared libraries, or if a symbol in the GOT points to a library that doesn't provide it.
To prevent future attacks similar to the liblzma attack, I'd like to implement a tool to audit the GOT of a running process which can be used in Fedora (and elsewhere) test infrastructure to look for signs of tampering.