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

Add a "got-all" command #1101

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Conversation

gordonmessmer
Copy link
Contributor

@gordonmessmer gordonmessmer commented May 10, 2024

Description

This change adds a "got-all" command which expands on the existing "got" command by providing data about relocations in mapped shared object files in addition to the relocations specific to the main executable.

Particularly for auditing purposes, users may be interested in the state of all relocations, not only those for the primary executable file.

  • 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.

Copy link

🤖 Coverage update for 35ed738 🟢

Old New
Commit 18c1f7c 35ed738
Score 71.5629% 71.5629% (0)

@gordonmessmer
Copy link
Contributor Author

This draft is functional, but incomplete. It does not include documentation or tests. In its current condition, it exists only to ask whether the maintainers are interested in expanding the "got" command to provide information about relocations in mapped shared object files or not. And, if so, what the output from that command might look like. The current implementation merely prints relocations for all files, without indicating which file's relocations are being printed.

I'm also not sure whether such a command needs to accept a filter, allowing the user to print relocations for specific files, other than the primary executable file.

Please let me know what you think, and whether or not I should finish work on this command.

@Grazfather
Copy link
Collaborator

Can you please show its use? Couldn't you just add this as an argument to the existing got command?

@gordonmessmer
Copy link
Contributor Author

I can show its output, but it tends to be quite long. I'll try attaching it as a file.

The existing got command will show the state of relocatable symbols present in the primary binary executable. That's not all of the relocations, though. For example, the login binary is linked to libpam.so.0 on GNU/Linux systems, and libpam uses dlopen() to load libraries from /usr/lib64/security (or a similar path). The existing got command will not show any instance of dlopen, because there's a separate offset table for each ELF library in addition to the offset table for the main binary.

Arguments to the current got command reduce its output by filtering for matching symbols. The proposed got-all command makes the output much longer, by printing all of the offset tables for each mapped shared object in addition to the binary executable.

@gordonmessmer
Copy link
Contributor Author

Attaching got-all and got output for the login process

got-all.txt
got.txt

@Grazfather
Copy link
Collaborator

Yes I understand, but we could add an argument to have it run got 'deeper'. I see this as a recursive GOT command, but with depth one. We could add a --all argument, or even a --depth argument that limits how deep it looks into libraries' libraries.

@gordonmessmer
Copy link
Contributor Author

You could do that, but since the existing behavior is for all arguments to act as filters, the change would not be fully backward compatible, which is largely why I didn't pursue that path.

If that's your preference, I can continue developing in that direction.

@Grazfather
Copy link
Collaborator

I believe it would be, but maybe it would be breaking searching for symbols that contain -all. @hugsy care to weigh in?

@hugsy
Copy link
Owner

hugsy commented May 21, 2024

I see this as a recursive GOT command, but with depth one. We could add a --all argument, or even a --depth argument that limits how deep it looks into libraries' libraries.

I'm with @Grazfather on this, I don't feel like this doesn't deserve its own standalone command, but if you wish to make it so, then I'd suggest moving it to gef-extras instead.
In any case, yes tests and docs are lacking, to explain more usage to users.

Copy link

🤖 Coverage update for 424e876 🟢

Old New
Commit 18c1f7c 424e876
Score 71.4923% 71.4923% (0)

Copy link

🤖 Coverage update for 59b76cf 🟢

Old New
Commit 18c1f7c 59b76cf
Score 71.4923% 71.4923% (0)

@gordonmessmer
Copy link
Contributor Author

I've modified the implementation to use an optional flag to the got command.

The current implementation prints the GOT for each shared object without labeling them, which isn't very user-friendly. What would be idiomatic for GEF?

else:
self.do_invoke_for(gef.session.file, args.symbols)

def do_invoke_for(self, file: str, argv: List[str]) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's name this something better. It might not even need to be a separate function, but you might want to still break some of the functionality into helper functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any suggestions?


res = gdb.execute("got --all printf", to_string=True)
self.assertIn("printf", res)
self.assertNotIn("strcpy", res)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What can we test that will show up in got --all but won't show up without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably the C library path, which is why I'd asked how to label the shared objects in a way that would be idiomatic for GEF.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These checks should make sure that they show up UNDER the libc part.

You could get the res, assert /libc is in there, split on it, and then make sure printf and strcpy are on the second part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split doesn't really work, because it would return two sections: everything before libc and everything after. AFAIK, the order isn't guaranteed, so the program binary could be in either one.

(Also, printf and strcpy are expected in the program's section, but not in libc's, because those aren't relocatable symbols in libc.)

If you'd like, I can rewrite this to parse the sections... I'll upload a commit that does that. If you don't like it, I'll drop that commit and roll back to the current state.

@Grazfather
Copy link
Collaborator

you could do something like

title = Color.colorify(lib_name, "yellow bold") # idc about the colour much, but should be bold

or maybe

title = titlify(lib_name)

though this is used more for drawing the context window.

@gordonmessmer
Copy link
Contributor Author

Using titlify:

gef➤  got --all malloc
──────────────────────────────────────────── /usr/bin/sleep ────────────────────────────────────────────

GOT protection: Full RelRO | GOT functions: 45
 
[0x55555555bf00] malloc@GLIBC_2.2.5  →  0x7ffff7e64670
───────────────────────────────────────── /usr/lib64/libc.so.6 ─────────────────────────────────────────

GOT protection: Full RelRO | GOT functions: 18
 
[0x7ffff7fa0d68] malloc@@GLIBC_2.2.5  →  0x7ffff7e64670
─────────────────────────────────── /usr/lib64/ld-linux-x86-64.so.2 ───────────────────────────────────

GOT protection: Full RelRO | GOT functions: 0

Copy link

🤖 Coverage update for 377fa94 🟢

Old New
Commit 18c1f7c 377fa94
Score 71.4923% 71.4923% (0)

Copy link

🤖 Coverage update for f353980 🟢

Old New
Commit 18c1f7c f353980
Score 71.4923% 71.4923% (0)

@Grazfather
Copy link
Collaborator

That looks pretty good to me.

Copy link

🤖 Coverage update for 9d3c406 🟢

Old New
Commit 18c1f7c 9d3c406
Score 71.4923% 71.4923% (0)

Copy link

🤖 Coverage update for 3e55c02 🟢

Old New
Commit 18c1f7c 3e55c02
Score 71.4923% 71.4923% (0)

Copy link

🤖 Coverage update for 030fe44 🟢

Old New
Commit 18c1f7c 030fe44
Score 71.4923% 71.4923% (0)

Copy link

🤖 Coverage update for 3f91ce0 🟢

Old New
Commit 18c1f7c 3f91ce0
Score 71.4923% 71.4923% (0)

Copy link

🤖 Coverage update for f6038e4 🟢

Old New
Commit 18c1f7c f6038e4
Score 71.4923% 71.4923% (0)

@gordonmessmer
Copy link
Contributor Author

I'm happier with this test setup.

There's still the matter of the do_invoke_for function name, if that's important to you. Otherwise, if you are happy with this, I can squash it before merging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants