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.

Sign up for GitHub

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

New CLI arguments and experimental code coverage #508

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

New CLI arguments and experimental code coverage #508

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
0d110de
Add --qemu for QEMU mode with coverage collection...
jtpereyda Mar 19, 2021
845b039
coverage feedback working!
jtpereyda Mar 20, 2021
54772b1
fix coverage bugs; cleaner exit on unexpected Exception
jtpereyda Mar 20, 2021
164f7d8
add QEMU path check (and remove reundant stop process)
jtpereyda Mar 20, 2021
6011f9e
fix QEMU path error
jtpereyda Mar 20, 2021
f0465d4
fix: QEMU server restart working now
jtpereyda Mar 24, 2021
982a12a
keep-web-open now works on error exits
jtpereyda Mar 26, 2021
33a7800
add --stdout, --web-ui, --restart-interval, --target-start-wait
jtpereyda Mar 26, 2021
579cb75
handle error in simple debugger
jtpereyda Mar 26, 2021
4503e31
TCP graceful shutdown; connection shutdown exception
jtpereyda Mar 30, 2021
75b6ab2
fix --web-port parsing
jtpereyda Mar 30, 2021
81f660c
add Session.register_post_start_target_callback() and fix false warning
jtpereyda Mar 30, 2021
f1825f3
print out signal name on crash
jtpereyda Apr 23, 2021
9426465
Merge branch 'master' into coverage
jtpereyda Apr 30, 2021
de3ed7d
documentation updates and cleanup
jtpereyda Apr 30, 2021
68ad063
Fixing style errors.
stickler-ci Apr 30, 2021
44baaa3
code review fixes -- back to normal socket-like recv behavior
jtpereyda May 14, 2021
33fe390
re-fix node id for Request
jtpereyda May 14, 2021
0f77469
conditional install for sysv_ipc; check for OS
jtpereyda May 17, 2021
366326b
Fixing style errors.
stickler-ci May 17, 2021
dceeac7
import guard on Qemu debugger in cli.py
jtpereyda May 17, 2021
5239677
Fixing style errors.
stickler-ci May 17, 2021
b04709f
fix OS check for Qemu import
jtpereyda May 17, 2021
be17dd3
fix non-persistent test case context
jtpereyda May 17, 2021
44c2f40
Merge branch 'master' into coverage
jtpereyda May 17, 2021
5a3d362
fix SessionInfo class queue properties (for boo open command)
jtpereyda May 17, 2021
0da9d85
raise timeout exception on recv timeout for TCP
jtpereyda May 25, 2021
68de3c5
Merge branch 'master' into coverage
jtpereyda May 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions boofuzz/blocks/request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ def __init__(self, name=None, children=None):
FuzzableBlock.__init__(self, name=name, request=self)
Node.__init__(self)
self.label = name # node label for graph rendering.
self._name = name
SR4ven marked this conversation as resolved.
Show resolved Hide resolved
self.stack = [] # the request stack.
self.block_stack = [] # list of open blocks, -1 is last open block.
self.callbacks = collections.defaultdict(list)
Expand Down Expand Up @@ -68,6 +69,8 @@ def name(self):
def name(self, name):
self._name = name

id = name # alias id to support Node superclass

@property
def fuzzable(self):
return True
Expand Down
72 changes: 54 additions & 18 deletions boofuzz/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import logging
import shlex
import sys
import time

import click
Expand All @@ -18,18 +19,20 @@
from .monitors import ProcessMonitor
from .utils.process_monitor_local import ProcessMonitorLocal
from .utils.debugger_thread_simple import DebuggerThreadSimple
from .utils.debugger_thread_qemu import DebuggerThreadQemu
from .utils import debugger_thread_qemu

temp_static_session = None
temp_static_procmon = None
temp_static_fuzz_only_one_case = None


@click.group(help="boofuzz experimental CLI; usage may change over time")
@click.group(help="boofuzz experimental CLI; usage may change over time", context_settings=dict(max_content_width=120))
def cli():
pass


@cli.group(help="Must be run via a fuzz script")
@cli.group(help="Must be run via a fuzz script", context_settings=dict(show_default=True))
@click.option("--target", metavar="HOST:PORT", help="Target network address", required=True)
@click.option("--test-case-index", help="Test case index", type=str)
@click.option("--test-case-name", help="Name of node or specific test case")
Expand All @@ -39,10 +42,15 @@ def cli():
)
@click.option("--procmon-host", help="Process monitor port host or IP")
@click.option("--procmon-port", type=int, default=DEFAULT_PROCMON_PORT, help="Process monitor port")
@click.option("--procmon-start", help="Process monitor start command")
@click.option("--procmon-capture", is_flag=True, help="Capture stdout/stderr from target process upon failure")
@click.option("--tui/--no-tui", help="Enable/disable TUI")
@click.option("--text-dump/--no-text-dump", help="Enable/disable full text dump of logs", default=False)
@click.option(
"--stdout",
type=click.Choice(["HIDE", "CAPTURE", "MIRROR"], case_sensitive=False),
default="MIRROR",
help="How to handle stdout (and stderr) of target. CAPTURE saves output for crash reporting but can "
"slow down fuzzing.",
)
@click.option("--tui/--no-tui", help="Enable TUI")
@click.option("--text-dump/--no-text-dump", help="Enable full text dump of logs", default=False)
@click.option("--feature-check", is_flag=True, help="Run a feature check instead of a fuzz test", default=False)
@click.option("--target-cmd", help="Target command and arguments")
@click.option(
Expand All @@ -60,6 +68,18 @@ def cli():
type=int,
help="Record this many cases before each failure. Set to 0 to record all test cases (high disk space usage!).",
)
@click.option(
"--qemu/--no-qemu",
is_flag=True,
default=False,
help="Experimental: Enable QEMU mode with code coverage feedback; requires afl-qemu-trace",
)
@click.option("--qemu-path", help="afl-qemu-trace path; looks in PATH by default")
@click.option("--web-port", type=int, default=constants.DEFAULT_WEB_UI_PORT, help="port for web GUI")
@click.option("--restart-interval", type=int, help="restart every n test cases")
@click.option(
"--target-start-wait", type=float, default=0, help="wait n seconds for target to settle in before fuzzing"
)
@click.pass_context
def fuzz(
ctx,
Expand All @@ -70,23 +90,36 @@ def fuzz(
sleep_between_cases,
procmon_host,
procmon_port,
procmon_start,
procmon_capture,
stdout,
tui,
text_dump,
feature_check,
target_cmd,
keep_web,
combinatorial,
record_passes,
qemu,
qemu_path,
web_port,
restart_interval,
target_start_wait,
):
if qemu:
if qemu_path is not None:
debugger_thread_qemu.QEMU_PATH = qemu_path
if not debugger_thread_qemu.QEMU_PATH:
print("afl-qemu-trace not found. Is it available in PATH?", file=sys.stderr)
sys.exit(1)
debugger = DebuggerThreadQemu
else:
debugger = DebuggerThreadSimple
local_procmon = None
if target_cmd is not None and procmon_host is None:
local_procmon = ProcessMonitorLocal(
crash_filename="boofuzz-crash-bin",
proc_name=None,
pid_to_ignore=None,
debugger_class=DebuggerThreadSimple,
debugger_class=debugger,
level=1,
)

Expand All @@ -100,12 +133,16 @@ def fuzz(
fuzz_loggers.append(FuzzLoggerCsv(file_handle=f))

procmon_options = {}
if procmon_start is not None:
procmon_options["start_commands"] = [procmon_start]
if target_cmd is not None:
procmon_options["start_commands"] = shlex.split(target_cmd)
if procmon_capture:
procmon_options["start_commands"] = [shlex.split(target_cmd)]
if target_start_wait:
procmon_options["startup_wait"] = target_start_wait
if stdout == "CAPTURE":
procmon_options["capture_output"] = True
elif stdout == "HIDE":
procmon_options["hide_output"] = True
elif stdout == "MIRROR":
pass

if local_procmon is not None or procmon_host is not None:
if procmon_host is not None:
Expand Down Expand Up @@ -152,6 +189,8 @@ def fuzz(
index_end=end,
keep_web_open=keep_web,
fuzz_db_keep_only_n_pass_cases=record_passes,
web_port=web_port,
restart_interval=restart_interval,
)

ctx.obj = CliContext(session=session)
Expand All @@ -162,13 +201,10 @@ def fuzzcallback(result, *args, **kwargs):
if feature_check:
session.feature_check()
else:
session.fuzz(name=test_case_name, max_depth=max_depth)

if procmon is not None:
procmon.stop_target()
session.fuzz(name=test_case_name, max_depth=max_depth, qemu=qemu)


@cli.command(name="open")
@cli.command(name="open", context_settings=dict(show_default=True))
@click.option("--debug", help="Print debug info to console", is_flag=True)
@click.option(
"--ui-port",
Expand Down
27 changes: 24 additions & 3 deletions boofuzz/connections/tcp_socket_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,32 @@ class TCPSocketConnection(base_socket_connection.BaseSocketConnection):
send_timeout (float): Seconds to wait for send before timing out. Default 5.0.
recv_timeout (float): Seconds to wait for recv before timing out. Default 5.0.
server (bool): Set to True to enable server side fuzzing.
graceful_shutdown (bool): close() method attempts a graceful shutdown. Default: True.

"""

def __init__(self, host, port, send_timeout=5.0, recv_timeout=5.0, server=False):
def __init__(self, host, port, send_timeout=5.0, recv_timeout=5.0, server=False, graceful_shutdown=True):
super(TCPSocketConnection, self).__init__(send_timeout, recv_timeout)

self.host = host
self.port = port
self.server = server
self._serverSock = None
self.graceful_shutdown = graceful_shutdown

def close(self):
if self.graceful_shutdown:
try:
self._sock.shutdown(socket.SHUT_RDWR)
while len(self._sock.recv(1024)) > 0:
pass
except ConnectionError:
SR4ven marked this conversation as resolved.
Show resolved Hide resolved
pass
except OSError as e:
if e.errno == errno.ENOTCONN:
pass
else:
raise
super(TCPSocketConnection, self).close()

if self.server:
Expand Down Expand Up @@ -84,18 +98,25 @@ def _connect_socket(self):

def recv(self, max_bytes):
"""
Receive up to max_bytes data from the target.
Receive up to max_bytes data from the target. Timeout results in 0 bytes returned.

Args:
max_bytes (int): Maximum number of bytes to receive.

Returns:
Received data.
Received data (empty bytes array if timed out).

Raises:
BoofuzzTargetConnectionShutdown: Target shutdown connection (e.g. socket recv returns 0 bytes)
BoofuzzTargetConnectionAborted: ECONNABORTED
BoofuzzTargetConnectionReset: ECONNRESET, ENETRESET, ETIMEDOUT
"""
data = b""

try:
data = self._sock.recv(max_bytes)
if len(data) == 0:
raise exception.BoofuzzTargetConnectionShutdown()
Copy link
Owner Author

@jtpereyda jtpereyda Apr 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

While working with boofuzz, I realized that the past design choice to return b"" on timeout, combined with the fact that the underlying socket API has recv return 0 to indicated a closed connection, leads to an ambiguity: a return value of b"" can mean a closed connection, or a timeout.

My solution was to throw an exception to indicate the socket has shut down, which is more intuitive to me personally. However, this actually flips the semantics of recv, which throws socket.timeout on timeout and returns b"" on socket shutdown. Perhaps it would be wiser to follow this approach to map better to peoples' existing knowledge.

Either way, we have some choice to make:

  1. Keep these changes (throw exception on socket shutdown), which removes the ambiguity but can break existing fuzzers that depend on socket shutdowns yielding an empty bytes array.
  2. Flip the approach, that is, allow socket.timeout to be bubbled up as an exception. This removes the ambiguity, but breaks existing fuzzers that depend on on timeouts yielding an empty bytes array.
  3. Keep the existing behavior, and add a new way to detect whether the socket was shutdown or whether a timeout happened. This is pretty lame but backwards compatible.

Edit: This change seems to be where the test failures are coming from.

Copy link
Owner Author

Choose a reason for hiding this comment

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

On the other hand, I don't know how many scripts out there would actually hit this compatibility problem.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Perhaps one tool to compare with is pwntools, which also has a recv method for which timeout returns an empty bytes, and a closed connection yields an exception: https://docs.pwntools.com/en/stable/tubes.html#pwnlib.tubes.tube.tube.recv

Copy link
Collaborator

@SR4ven SR4ven May 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm difficult choice.
I don't like the existing behaviour because we can't differentiate a close from a timeout. So I'd say either stick to the default socket/posix behaviour or raise an exception for everything.

The default posix way would be choice 2 if I understand correctly. Raise socket.timeout and return b"" on close.

However, as we are on application level I could also imagine raising an exception for both cases. Not sure what others think about this but it might be the most user friendly. You simply tell boofuzz to receive after a test case and if anything goes wrong (close/timeout/abort/reset) you get an exception.
On the other hand, I know some of my targets close the connection if the received data was invalid. In that case the target didn't crash but boofuzz would raise an exception. That might be a bit inconvenient.

I also just found the session option check_data_received_each_request. It seems this option is the reason for both close and timeout returning b"". If we switch to exceptions, we'd have to catch them here and depending on the setup maybe re-raise?

Right now I favour choice 2 as anyone with basic knowledge about sockets will understand what's going on. Also it looks like that approach could easily be adapted to the current check_data_received_each_request behaviour.

BTW how can we get EWOULDBLOCK if we use a blocking socket and catch socket.timeout before?

boofuzz/boofuzz/connections/tcp_socket_connection.py

Line 131 in 68ad063

elif e.errno == errno.EWOULDBLOCK: # timeout condition if using SO_RCVTIMEO or SO_SNDTIMEO

And why do we interpret ETIMEDOUT as BoofuzzTargetConnectionReset?

boofuzz/boofuzz/connections/tcp_socket_connection.py

Line 129 in 68ad063

elif (e.errno == errno.ECONNRESET) or (e.errno == errno.ENETRESET) or (e.errno == errno.ETIMEDOUT):

Edit: I wouldn't worry about breaking backwards compatibility. As you already said I doubt that many scripts use the boofuzz socket interface directly and rely on this specific behaviour.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I also just found the session option check_data_received_each_request. It seems this option is the reason for both close and timeout returning b"". If we switch to exceptions, we'd have to catch them here and depending on the setup maybe re-raise?

Yes, that Session option is a layer on top of the socket behavior. The only place in my scripts this would matter is in callbacks, where I sometimes have code set up to receive the next (typically valid) message. Whatever choice we make here, we can modify Session to still act the same way with check_data_received_each_request.

Copy link
Owner Author

@jtpereyda jtpereyda May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Trying to get all these options clear in my head:

 ----------+--------------+-----------------------+--------------+------------+------------
|          | OS socket    | OS socket nonblocking | boo previous | PR initial | PR propsed |
+----------+--------------+-----------------------+--------------+------------+------------+
| timeout  | wait forever | exception             | return b""   | return b"" | exception  |
| shutdown | return b""   | return b""            | return b""   | exception  | return b"" |
 ----------+--------------+-----------------------+--------------+------------+------------

Part of me wants to yield an exception in both cases. I'm leaning toward matching OS socket behavior, but that behavior is a bit counterintuitive.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The table seems to be correct.

Agreed, I feel exactly the same way. I think if you have worked with sockets before, the OS socket behaviour is more intuitive.
It's the other way around if you haven't I guess.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which timeout/shutdown behaviour are we going to use now? The one initially proposed in this PR or the OS like?

Copy link
Owner Author

Choose a reason for hiding this comment

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

@SR4ven The OS-like behavior. Just realized I didn't add the timeout exception though...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright. We don't need BoofuzzTargetConnectionShutdown anymore do we.

Also, do we need to adapt other connection classes to the new behaviour? UDP maybe?
We could do that in another PR if needed.

except socket.timeout:
data = b""
except socket.error as e:
Expand Down
1 change: 1 addition & 0 deletions boofuzz/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
)

ERR_CONN_RESET = "Target connection reset."
ERR_CONN_SHUTDOWN = "Connection shutdown by target."

ERR_CONN_RESET_FAIL = "Target connection reset -- considered a failure case when triggered from post_send"

Expand Down
4 changes: 4 additions & 0 deletions boofuzz/exception.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ class BoofuzzTargetConnectionReset(BoofuzzError):
pass


class BoofuzzTargetConnectionShutdown(BoofuzzError):
pass


@attr.s
class BoofuzzTargetConnectionAborted(BoofuzzError):
"""
Expand Down
26 changes: 26 additions & 0 deletions boofuzz/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@
COLOR_PAIR_MAGENTA = 6
COLOR_PAIR_BLACK = 7

sigmap = dict(
(k, v) for v, k in reversed(sorted(signal.__dict__.items())) if v.startswith("SIG") and not v.startswith("SIG_")
)

test_step_info = {
"test_case": {
"indent": 0,
Expand Down Expand Up @@ -476,3 +480,25 @@ def parse_test_case_name(test_case):
mutations = match.group(1)
mutations = re.split(r",\s*", mutations)
return path, mutations


def _reset_shm_map(shm_map):
"""Reset shared memory map (used with AFL fork server)."""
for i in range(0, len(shm_map)):
shm_map[i] = 0


def crash_reason(exit_status):
"""Get human readable crash reason from exit status."""
reason = "Process died for unknown reason"
if exit_status is not None:
if os.WCOREDUMP(exit_status):
reason = "Segmentation fault"
elif os.WIFSTOPPED(exit_status):
reason = "Stopped with signal " + str(os.WTERMSIG(exit_status))
elif os.WIFSIGNALED(exit_status):
sig = os.WTERMSIG(exit_status)
reason = "Terminated with signal {0} {1}".format(str(sig), sigmap[sig])
elif os.WIFEXITED(exit_status):
reason = "Exit with code - " + str(os.WEXITSTATUS(exit_status))
return reason
2 changes: 1 addition & 1 deletion boofuzz/pgraph/edge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def __init__(self, src, dst):

# the unique id for any edge (provided that duplicates are not allowed) is the combination of the source and
# the destination stored as a long long.
self.id = (src << 32) + dst
self.id = str(src) + "->" + str(dst)
self.src = src
self.dst = dst

Expand Down
Loading