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 ".." to the list of local path prefixes in get_file() #2013

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

magnified103
Copy link
Contributor

@magnified103 magnified103 commented Feb 5, 2024

This commit adds "../" to the list of local path prefixes.

Reproduce (assume that patchelf is in PATH)

ln -s /lib64/ld-linux-x86-64.so.2 ./ld-linux-x86-64.so.2
mkdir bin && cd bin
echo "int main() {}" > main.c && gcc -o main main.c
patchelf --set-interpreter ../ld-linux-x86-64.so.2 ./main
gdb -ex "set exception-debugger on" -ex "entry" -ex "got -a" ./main

Output:

Reading symbols from ./main...
(No debugging symbols found in ./main)
Temporary breakpoint 1 at 0x555555555040
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Temporary breakpoint 1, 0x0000555555555040 in _start ()
Filtering out read-only entries (display them with -r or --show-readonly)

State of the GOT of ***/bin/main:
GOT protection: Partial RELRO | Found 0 GOT entries passing the filter

Traceback (most recent call last):
  File "***/pwndbg/pwndbg/commands/__init__.py", line 181, in __call__
    return self.function(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "***/pwndbg/pwndbg/commands/__init__.py", line 328, in _OnlyWhenRunning
    return function(*a, **kw)
           ^^^^^^^^^^^^^^^^^^
  File "***/pwndbg/pwndbg/commands/got.py", line 100, in got
    _got(path, accept_readonly, symbol_filter)
  File "***/pwndbg/pwndbg/commands/got.py", line 113, in _got
    local_path = pwndbg.gdblib.file.get_file(path, try_local_path=True)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "***/pwndbg/pwndbg/gdblib/file.py", line 60, in get_file
    assert path.startswith(("/", "./")) or path.startswith(
AssertionError: get_file called with incorrect path

If that is an issue, you can report it on https://github.com/pwndbg/pwndbg/issues
(Please don't forget to search if it hasn't been reported before)
To generate the report and open a browser, you may run `bugreport --run-browser`
PS: Pull requests are welcome
> ***/pwndbg/pwndbg/gdblib/file.py(60)get_file()
-> assert path.startswith(("/", "./")) or path.startswith(
(Pdb) p path
'../ld-linux-x86-64.so.2'
(Pdb) 

Copy link

codecov bot commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eb3c654) 59.53% compared to head (da54e26) 57.70%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2013      +/-   ##
==========================================
- Coverage   59.53%   57.70%   -1.83%     
==========================================
  Files         187      187              
  Lines       23482    23482              
  Branches     2277     2277              
==========================================
- Hits        13979    13551     -428     
- Misses       8756     9222     +466     
+ Partials      747      709      -38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@disconnect3d
Copy link
Member

@magnified103 How do we reproduce this issue?

@magnified103
Copy link
Contributor Author

magnified103 commented Feb 6, 2024

@magnified103 How do we reproduce this issue?

I have edited the original comment for clarity.

@disconnect3d
Copy link
Member

I guess the problem may still be there if u have ld.so in ../../ and so on?

@magnified103
Copy link
Contributor Author

I guess the problem may still be there if u have ld.so in ../../ and so on?

It runs fine when I set the interpreter to paths like ../../. I guess that check just differentiates between relative path and the "target:" prefix (a local directory named "target:" may exists).

@disconnect3d
Copy link
Member

For what is worth, when I tried to reproduce it, my checksec binary fails as it loads libunicorn which wants to mmap 1GB+ memory and my VPS doesn't have as much. LOL:

root@pwndbg:~# strace -e openat,mmap checksec /root/x/bin/main 2>&1 | tail -n 10
openat(AT_FDCWD, "/usr/lib/python3/dist-packages/mpmath-0.0.0.egg-info/PKG-INFO", O_RDONLY|O_CLOEXEC) = 7
openat(AT_FDCWD, "/usr/local/lib/python3.10/dist-packages/unicorn/lib/libunicorn.so.2", O_RDONLY|O_CLOEXEC) = 7
mmap(NULL, 22447520, PROT_READ, MAP_PRIVATE|MAP_DENYWRITE, 7, 0) = 0x7f7a2b574000
mmap(0x7f7a2b910000, 13496320, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 7, 0x39c000) = 0x7f7a2b910000
mmap(0x7f7a2c5ef000, 3039232, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 7, 0x107b000) = 0x7f7a2c5ef000
mmap(0x7f7a2c8d5000, 1601536, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 7, 0x1360000) = 0x7f7a2c8d5000
mmap(0x7f7a2ca5c000, 525728, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f7a2ca5c000
mmap(NULL, 1073741824, PROT_READ|PROT_WRITE|PROT_EXEC, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory)
Could not allocate dynamic translator buffer
+++ exited with 1 +++

@magnified103
Copy link
Contributor Author

I'm no gdb expert, but is there a better way to handle those paths? Matching prefixes to toggle some flags is ugly (does it come from gdb output?).

By the way, does the failing CI check relate to the patch itself?

@disconnect3d disconnect3d merged commit 904624d into pwndbg:dev Feb 7, 2024
14 of 15 checks passed
@disconnect3d
Copy link
Member

disconnect3d commented Feb 7, 2024

Yeah the CI is irrelevant to this patch. I've validated this bug after fixing a Pwntools bug locally (Gallopsled/pwntools#2347, and unrelated to that: Gallopsled/pwntools#2345) and this fix works well.

For what is worth, I think we may want to remove the assertion overall? I am not sure why we need it in the first place right now. It is probably there to guard us against unexpected input?

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants