Add an "--all" option to the got command

gordonmessmer commented 6 months ago

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.

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

github-actions[bot] commented 6 months ago

🤖 Coverage update for 35ed738c35b51a12ccee57363aad8c1f6b8c98f6 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	35ed738c35b51a12ccee57363aad8c1f6b8c98f6
Score	71.5629%	71.5629% (0)

gordonmessmer commented 6 months ago

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 commented 6 months ago

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

gordonmessmer commented 6 months ago

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 commented 6 months ago

Attaching got-all and got output for the login process

got-all.txt got.txt

Grazfather commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

hugsy commented 6 months ago

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.

github-actions[bot] commented 6 months ago

🤖 Coverage update for 424e876ec3b1980bd0a49c6650c0a4c188a8159c 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	424e876ec3b1980bd0a49c6650c0a4c188a8159c
Score	71.4923%	71.4923% (0)

github-actions[bot] commented 6 months ago

🤖 Coverage update for 59b76cfac86dbcb02bcc62f02b484742745086f4 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	59b76cfac86dbcb02bcc62f02b484742745086f4
Score	71.4923%	71.4923% (0)

gordonmessmer commented 6 months ago

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?

Grazfather commented 6 months ago

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 commented 6 months ago

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

github-actions[bot] commented 6 months ago

🤖 Coverage update for 377fa942369737088488994341b5e696fb097fa4 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	377fa942369737088488994341b5e696fb097fa4
Score	71.4923%	71.4923% (0)

github-actions[bot] commented 6 months ago

🤖 Coverage update for f3539805ca2a68cd458d92aa9ac180183a295459 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	f3539805ca2a68cd458d92aa9ac180183a295459
Score	71.4923%	71.4923% (0)

Grazfather commented 6 months ago

That looks pretty good to me.

github-actions[bot] commented 6 months ago

🤖 Coverage update for 9d3c406961c859616d1c5a2475a34cc61ead2a63 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	9d3c406961c859616d1c5a2475a34cc61ead2a63
Score	71.4923%	71.4923% (0)

github-actions[bot] commented 6 months ago

🤖 Coverage update for 3e55c025c36c779c7aa75c7cd51949b1febfb48c 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	3e55c025c36c779c7aa75c7cd51949b1febfb48c
Score	71.4923%	71.4923% (0)

github-actions[bot] commented 6 months ago

🤖 Coverage update for 030fe4417f936a6443e5c3bda6d470e3f78b21f2 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	030fe4417f936a6443e5c3bda6d470e3f78b21f2
Score	71.4923%	71.4923% (0)

github-actions[bot] commented 6 months ago

🤖 Coverage update for 3f91ce04bbb964d54cfe1c45f323fece2017deee 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	3f91ce04bbb964d54cfe1c45f323fece2017deee
Score	71.4923%	71.4923% (0)

github-actions[bot] commented 6 months ago

🤖 Coverage update for f6038e467e27ef523643fb5b3be215a2cb84bbf3 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	f6038e467e27ef523643fb5b3be215a2cb84bbf3
Score	71.4923%	71.4923% (0)

gordonmessmer commented 6 months ago

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

github-actions[bot] commented 6 months ago

🤖 Coverage update for 94ab4773e8dc7b8470484d0ec548a8bc29d75212 🟢

	Old	New
Commit	18c1f7c433e320066808e31797f18dae30830982	94ab4773e8dc7b8470484d0ec548a8bc29d75212
Score	71.4923%	71.4923% (0)

Grazfather commented 6 months ago

Other than line length nitpicking looks good to me.

github-actions[bot] commented 6 months ago

🤖 Coverage update for 4a750cf7f7cab77d050a1b98053acb795ed3f3e9 🟢

	Old	New
Commit	2c26e33fd29e6a9caa4bdc07a43ec7386f7cbf35	4a750cf7f7cab77d050a1b98053acb795ed3f3e9
Score	71.4923%	71.4923% (0)

gordonmessmer commented 6 months ago

Thank you for your review. It sounds like this is just about ready, so I've squashed the commits.

github-actions[bot] commented 6 months ago

🤖 Coverage update for e18c40d24081f9c08c488c05e62c7d6f8265a579 🟢

	Old	New
Commit	2c26e33fd29e6a9caa4bdc07a43ec7386f7cbf35	e18c40d24081f9c08c488c05e62c7d6f8265a579
Score	71.4923%	71.4923% (0)

gordonmessmer commented 6 months ago

(Squashed again.)

github-actions[bot] commented 6 months ago

🤖 Coverage update for 0d9507d609d14ff0db054b257db7d1248ff75e57 🟢

	Old	New
Commit	bdf82195c99f863000c2a4820a727b217c853081	0d9507d609d14ff0db054b257db7d1248ff75e57
Score	71.4923%	71.4923% (0)

github-actions[bot] commented 6 months ago

🤖 Coverage update for 5c68a97a44cbf3d11556e83c1f54719c63a3d7d8 🟢

	Old	New
Commit	bdf82195c99f863000c2a4820a727b217c853081	5c68a97a44cbf3d11556e83c1f54719c63a3d7d8
Score	71.4923%	71.4923% (0)

gordonmessmer commented 6 months ago

I've attached a commit that partially fixes the use of the got command with remote debugging. However, the realpath() property still doesn't work some of the time, because self.path reflects the path the appears in /proc//maps, but the file is being downloaded to what looks like the path that appears in the output of ldd (and that, I would argue, is a bug). The path that appears in maps is the real path; the path that appears in the output of ldd might include a symlink, and isn't canonicalized with realpath()

So, for example, /tmp/tmpttzy8t_f/lib64/libc.so.6 is present locally, but maps refers to /usr/lib64/libc.so.6

Personally, I think this is a problem that should be solved in a separate PR. The patch is a little messy, and fixing the got command when remote debugging isn't strictly related to the --all option. If you agree, I'm happy to back that commit out and open a new PR.

gordonmessmer commented 6 months ago

I think that resolving remote use of got will require fixing bug https://sourceware.org/bugzilla/show_bug.cgi?id=23764

The current suggestion is that python code should use realpath instead using .filename, but that isn't possible for remote debugging.

I've requested an account for sourceware's bug system, but those require human approval, so it'll take a while to set that up. And since this looks like a gdb bug, I don't think it's going to be possible to fix remote use of got --all in the immediate future.

The patch that I've provided for remote debugging is probably still good, and appropriate, but as I said earlier: messy.

Grazfather commented 6 months ago

GDB bugs take years to fix. Does your last path work decently well? We could just add a note to the docs that it doesn't work well with remote sessions and make a note to fix it later.

gordonmessmer commented 6 months ago

GDB bugs take years to fix.

Yes, that's what I meant by "not in the immediate future." :)

Does your last path work decently well?

It allows the got command to work without the --all option, and with the --all option, libraries which are found in solib search paths that don't contain any symlinks will work.

But on RHEL systems, /lib64 is a symlink to /usr/lib64, so most solibs won't work if the remote host is RHEL or a related system.

I'll move it to a separate PR, though, because I think it bears some discussion about style and consistency.

Grazfather commented 6 months ago

Yeah, let's just add a note to the docs about its current state for this PR.

github-actions[bot] commented 6 months ago

🤖 Coverage update for 069771259de29ea789437690dc11246deb15d4ab 🟢

	Old	New
Commit	bdf82195c99f863000c2a4820a727b217c853081	069771259de29ea789437690dc11246deb15d4ab
Score	71.4923%	71.4923% (0)

hugsy / gef