Add a "got-audit" command to look for signs of GOT tampering #1094

gordonmessmer · 2024-04-24T02:59:57Z

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.

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.

gordonmessmer · 2024-04-24T03:02:30Z

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.

hugsy · 2024-04-24T16:15:39Z

@gordonmessmer
This is an interesting command, but I'd suggest to move this command to gef-extras: gef is moving to the direction of mostly being used as core library.

gordonmessmer · 2024-04-25T06:32:13Z

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?

hugsy · 2024-04-25T15:38:44Z

gef.py

@@ -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__]:


why this change?

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)

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?

hugsy · 2024-04-25T15:41:27Z

gef.py

+        # retrieve symbols using nm
+        lines = gef_execute_external([nm, "-D", elf_file], as_list=True)
+        for line in lines:
+            words = line.split(' ')


Suggested change

words = line.split(' ')

words = line.split()

hugsy · 2024-04-25T15:42:49Z

gef.py

+            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'):


this might trigger an IndexError if there's any change to the nm output
you need to add some extra validations of the output

hugsy · 2024-04-25T15:43:52Z

gef.py

+    _cmdline_ = "got-audit"
+    _syntax_ = f"{_cmdline_} [FUNCTION_NAME ...] "
+    _example_ = "got-audit read printf exit"
+    _symbols_ = collections.defaultdict(list)


Add specific type hints

hugsy · 2024-04-25T15:44:00Z

gef.py

+    _syntax_ = f"{_cmdline_} [FUNCTION_NAME ...] "
+    _example_ = "got-audit read printf exit"
+    _symbols_ = collections.defaultdict(list)
+    _paths_ = collections.defaultdict(list)


hugsy · 2024-04-25T15:45:40Z

gef.py

+                if elf_file not in self._symbols_[sym]:
+                    self._symbols_[sym].append(elf_file)
+                self._paths_[elf_file].append(sym)
+


trailing whitespace, you need to apply pre-commit run

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.

hugsy · 2024-04-25T15:47:27Z

gef.py

+        # 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_


Section always has the path attribute, so this check will always return true. This part should be necessary

hugsy · 2024-04-25T15:48:29Z

gef.py

+        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):


Suggested change

and Permission.EXECUTE in section.permission):

and section.permission & Permission.EXECUTE):

hugsy · 2024-04-25T15:48:49Z

gef.py

+                self.get_symbols_from_path(section.path)
+        return super().do_invoke(argv)
+
+    def build_line(self, name, color, address_val, got_address):


missing type hints

hugsy · 2024-04-25T15:51:37Z

gef.py

+        line = Color.colorify(f"{name}", color)
+        found = 0
+        for section in gef.memory.maps:
+            if section.contains(got_address) and hasattr(section, "path"):


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

Suggested change

if section.contains(got_address) and hasattr(section, "path"):

if not section.contains(got_address):

continue

hugsy · 2024-04-25T15:52:27Z

gef.py

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


you need to add nm as a binary deps in docs/install.md as well

hugsy · 2024-04-25T15:53:35Z

tests/commands/got_audit.py

@@ -0,0 +1,42 @@
+"""


great job adding those tests and documentation

hugsy

Code itself looks ok, some minor improvements.
Once those fixed and moved to gef-extras, it'll be merged quickly

github-actions · 2024-04-26T00:05:21Z

🤖 Coverage update for `a096636` 🟢

	Old	New
Commit	`92f45ba`	`a096636`
Score	71.5835%	71.5835% (0)

gordonmessmer · 2024-04-26T00:19:54Z

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 gdb --batch without any input, but generate_glibc_args_json.py may prompt "file x86_64.json exists, overwrite? [y/N]".

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?

github-actions · 2024-04-26T06:01:21Z

🤖 Coverage update for `cdf3c87` 🟢

	Old	New
Commit	`92f45ba`	`cdf3c87`
Score	71.5835%	71.5835% (0)

therealdreg · 2024-04-26T08:56:42Z

but generate_glibc_args_json.py may prompt "file x86_64.json exists, overwrite? [y/N]".

I am fixing this!

gordonmessmer · 2024-04-26T20:06:29Z

Is there an issue or PR I can follow for the generate_glibc_args_json.py changes?

gordonmessmer added 3 commits April 1, 2024 23:47

If a GOT entry is mapped to a section with a path, print the path.

0963833

Implement "got-audit" command.

2c32a88

Add tests and docs.

5d187e9

gordonmessmer marked this pull request as draft April 24, 2024 03:00

gordonmessmer marked this pull request as ready for review April 24, 2024 04:18

gordonmessmer marked this pull request as draft April 24, 2024 04:18

hugsy reviewed Apr 25, 2024

View reviewed changes

tests/commands/got_audit.py

@@ -0,0 +1,42 @@

"""

Copy link

Owner

hugsy Apr 25, 2024 •

edited

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

hugsy requested changes Apr 25, 2024

View reviewed changes

Clean-ups from PR review.

a096636

Add images for documentation.

cdf3c87

gordonmessmer marked this pull request as ready for review April 26, 2024 06:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a "got-audit" command to look for signs of GOT tampering #1094

Add a "got-audit" command to look for signs of GOT tampering #1094

gordonmessmer commented Apr 24, 2024

gordonmessmer commented Apr 24, 2024 •

edited

hugsy commented Apr 24, 2024 •

edited

gordonmessmer commented Apr 25, 2024

hugsy Apr 25, 2024

gordonmessmer Apr 25, 2024 •

edited

gordonmessmer Apr 25, 2024

hugsy Apr 25, 2024

hugsy Apr 25, 2024

hugsy Apr 25, 2024

hugsy Apr 25, 2024

hugsy Apr 25, 2024

gordonmessmer Apr 26, 2024

hugsy Apr 25, 2024

hugsy Apr 25, 2024

hugsy Apr 25, 2024

hugsy Apr 25, 2024

hugsy Apr 25, 2024

hugsy Apr 25, 2024 •

edited

hugsy left a comment

github-actions bot commented Apr 26, 2024

gordonmessmer commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

therealdreg commented Apr 26, 2024 •

edited

gordonmessmer commented Apr 26, 2024

	and Permission.EXECUTE in section.permission):
	and section.permission & Permission.EXECUTE):

	if section.contains(got_address) and hasattr(section, "path"):
	if not section.contains(got_address):
	continue

Add a "got-audit" command to look for signs of GOT tampering #1094

Are you sure you want to change the base?

Add a "got-audit" command to look for signs of GOT tampering #1094

Conversation

gordonmessmer commented Apr 24, 2024

Description

gordonmessmer commented Apr 24, 2024 • edited

hugsy commented Apr 24, 2024 • edited

gordonmessmer commented Apr 25, 2024

Choose a reason for hiding this comment

gordonmessmer Apr 25, 2024 • edited

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

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

hugsy Apr 25, 2024 • edited

Choose a reason for hiding this comment

hugsy left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 26, 2024

🤖 Coverage update for a096636 🟢

gordonmessmer commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

🤖 Coverage update for cdf3c87 🟢

therealdreg commented Apr 26, 2024 • edited

gordonmessmer commented Apr 26, 2024

gordonmessmer commented Apr 24, 2024 •

edited

hugsy commented Apr 24, 2024 •

edited

gordonmessmer Apr 25, 2024 •

edited

hugsy Apr 25, 2024 •

edited

🤖 Coverage update for `a096636` 🟢

🤖 Coverage update for `cdf3c87` 🟢

therealdreg commented Apr 26, 2024 •

edited