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

Fix issues related with debugging #1323

Merged
merged 10 commits into from
Dec 29, 2019
Merged

Fix issues related with debugging #1323

merged 10 commits into from
Dec 29, 2019

Conversation

bet4it
Copy link
Contributor

@bet4it bet4it commented Jul 14, 2019

  • We don't need to use os.environ when env is not set, and when remote debugging this will lead to a confusion.
  • We shouldn't file the local executable when remote debugging, gdb will download it automatically by exec-file related commands.
  • Unify the way to validate argv. (Should we put them in one place to avoid code duplication?)
  • Fix encoding related issues.

After this pulll request, some tests like

pwntools/pwnlib/gdb.py

Lines 387 to 402 in 544ae93

# Connect to the SSH server
shell = ssh('passcode', 'pwnable.kr', 2222, password='guest')
# Start a process on the server
io = gdb.debug(['bash'],
ssh=shell,
gdbscript='''
break main
continue
''')
# Send a command to Bash
io.sendline("echo hello")
# Interact with the process
io.interactive()

pwntools/pwnlib/gdb.py

Lines 596 to 608 in 544ae93

shell = ssh('bandit0', 'bandit.labs.overthewire.org', password='bandit0', port=2220)
# Start a process on the server
cat = shell.process(['cat'])
# Attach a debugger to it
gdb.attach(cat, '''
break exit
continue
''')
# Cause `cat` to exit
cat.close()

pwntools/pwnlib/gdb.py

Lines 858 to 864 in 544ae93

>>> with context.local(log_level=9999): # doctest: +SKIP
... shell = ssh(host='bandit.labs.overthewire.org',user='bandit0',password='bandit0', port=2220)
... bash_libs = gdb.find_module_addresses('/bin/bash', shell)
>>> os.path.basename(bash_libs[0].path) # doctest: +SKIP
'libc.so.6'
>>> hex(bash_libs[0].symbols['system']) # doctest: +SKIP
'0x7ffff7634660'

can work (on my vps).

I think this pull request also solves #1314.

Copy link
Member

@Arusekk Arusekk left a comment

Choose a reason for hiding this comment

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

Well, this seems to be a nice change.

There are two minor drawbacks I addressed, but generally it is fine. Also it would be perfect if you added doctests for the new changes.

pwnlib/tubes/process.py Outdated Show resolved Hide resolved
pwnlib/tubes/ssh.py Show resolved Hide resolved
Co-Authored-By: Arusekk <arek_koz@o2.pl>
pwnlib/gdb.py Show resolved Hide resolved
@bet4it
Copy link
Contributor Author

bet4it commented Jul 16, 2019

@Arusekk It seems that there are no doctests in gdb.py, it may not suitable to add doctests there?

@Arusekk
Copy link
Member

Arusekk commented Jul 16, 2019

There is never anything wrong with adding tests to an untested piece of code 😉

The reason was so far that it was hard to automatically test debugging without having a display on the testing machine. (gdb.debug() results in starting another terminal by default)

@bet4it
Copy link
Contributor Author

bet4it commented Jul 16, 2019

How can we test remote debugging? The vps provided in the comment ( pwnable.kr ) can't be used, I think they may add some restrictions.
And I really don't know how to build doctest environment locally.

@Arusekk
Copy link
Member

Arusekk commented Jul 16, 2019

Fortunately, the CI setup creates a virtual host example.pwnme and allows to log in without password as user travis. Testing gdb seems way harder than ssh, though.

@bet4it
Copy link
Contributor Author

bet4it commented Jul 18, 2019

I'm trying to add doctests to gdb.py by setting context.terminal with custom script so we can interact with gdb. You can see what I have done in the latest commit.

I have some questions about it:

  • We may need to create a new script file for each doctest. Is this proper?
  • Is this proper to use pwntools in the script file?
  • Where should we put those scripts into?
  • Which name do you think the script files should have?
  • Should I use relative path when referring the script file? It seems that pwd is different when travis running in python2 and python3
  • Any other suggestions?

@Arusekk
Copy link
Member

Arusekk commented Jul 24, 2019

Yes, this looks perfectly fine, although a separate script for each file seems not very great.
I think the scripts could be placed under the docs/ directory, and named something indicating that they are fake terminals for gdb.

As for the path, you can use a path stored on the module (e.g. gdb.TEST_TERM_PATH), and resolve it relatively to pwnlib.__file__ or whatever.

Also, it could be useful to generate the fake terminal files on-the-fly, maybe using some utility function and tempfiles (the best option in my opinion).

You mentioned before something of deduplicating the code for validating argv and env. This is also a great idea, and the validation takes place (AFAICT) in gdb, in tubes.process, and in tubes.ssh.

@zachriggle what do you think of that?

@numin0us
Copy link

Hey @bet4it, I pulled your debug branch and noticed that this issue still crops up when gdb.attach() is invoked to attach to a local process.

root@kali:~/ctf/MBE/lab6A# ptpython
>>> from pwn import *
>>> context.log_level = 'DEBUG'
...
>>> p = process('./lab6A')
...
SION_BUS_ADDRESS': b'unix:path=/run/user/0/bus', b'SSH_TTY': b'/dev/pts/1', b'_': b'/usr/local/bin/ptpython', b'OLDPWD': b'/root/ctf/MBE'} : pid 12061
>>> gdb.attach(12061)
[DEBUG] Wrote gdb script to '/tmp/pwnk66rvllg.gdb'
    file "/root/ctf/MBE/lab6A/lab6A"

in GDB

pwndbg> disass main
Dump of assembler code for function main:
   0x00000c0f <+0>:     Cannot access memory at address 0xc0f

After hacking together a solution to remove the offending file line, debugging seemed to work fine.

@bet4it
Copy link
Contributor Author

bet4it commented Sep 21, 2019

I have added doctests for most functions in gdb.py.

Now I only need one fake terminal file for all the situations.
But I need to set it in doctests of every function.
Can I set it globally before doctests started?

To solve the bug mentioned by @skywardgaze, I remove all the 'file' statement in gdb script. I think we should always load the symbols of executable in the argument of gdb command.

Copy link
Member

@Arusekk Arusekk left a comment

Choose a reason for hiding this comment

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

Sorry for not responding, I have been quite busy lately. I have some questions about the code you added.

@@ -45,6 +48,7 @@ before_script:
- PWNLIB_NOTERM=1 python -c 'from pwn import *; print(pwnlib.term.term_mode)'
- PWNLIB_NOTERM=1 python -c 'from pwn import *; print(pwnlib.term.term_mode)'
- PWNLIB_NOTERM=1 python -c 'from pwn import *; print(pwnlib.term.term_mode)'
- sudo sh -c "echo 0 > /proc/sys/kernel/yama/ptrace_scope"
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary, as we do some prctl() in pwntools to mitigate this already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's necessary. prctl() only affects direct child process.
You can try this by yourself:

pwntools/pwnlib/gdb.py

Lines 587 to 597 in 0491972

# Start a forking server
server = process(['socat', 'tcp-listen:1234,fork,reuseaddr', 'exec:/bin/sh'])
# Connect to the server
io = remote('localhost', 1234)
# Connect the debugger to the server-spawned process
gdb.attach(io, '''
break exit
continue
''')

The attach should fail if ptrace_scope is not set to 0.

Copy link
Member

Choose a reason for hiding this comment

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

I see, the process is created by socat, not us. This is bad, because pwntools users won't be able to do this either without disabling Yama.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think most users won't use pwntools in this way, so it's okey to leave this problem here😃

pwnlib/gdb.py Show resolved Hide resolved
Conflicts:
	pwnlib/gdb.py
@@ -209,6 +209,7 @@ def cyclic_find(subseq, alphabet = None, n = None):

if isinstance(subseq, six.integer_types):
subseq = packing.pack(subseq, bytes=n)
subseq = context._encode(subseq)
Copy link
Member

Choose a reason for hiding this comment

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

This breaks python 3 build. What is your usecase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope cyclic_find can also accept a str likes cyclic_find('baaacaaa') besides of cyclic_find(b'baaacaaa') in Python 3.

Copy link
Member

Choose a reason for hiding this comment

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

This must be done somehow differently. _encode enforces bytes, while we need the type that is used in the alphabet. The internal library usage does not use str with binary alphabets, nor does it use bytes with text alphabets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can unify subseq and alphabet to bytes. Add a commit to fix the build.

@bet4it bet4it changed the base branch from dev3 to dev December 28, 2019 01:44
@Arusekk Arusekk merged commit f727b66 into Gallopsled:dev Dec 29, 2019
@bet4it bet4it deleted the debug branch December 29, 2019 12:41
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.

4 participants