[RFC ABRT PATCH v2] Add gdb python plugin which analyzes vulnerability

Martin Milata mmilata at redhat.com
Thu Jun 13 14:55:14 UTC 2013


Hi,
I don't have much to say regarding what the script does -- I think it's
a good start and you probably have better knowledge of low-level details
than I do anyway. Perhaps someone from security or tools department
might give us some feedback or hint how to go about this? There's also
similar script for the ubuntu error reporting tool, you might want to
check it out for some ideas:
http://bazaar.launchpad.net/~apport-hackers/apport/trunk/view/head:/data/general-hooks/parse_segv.py

As the script only uses the instruction pointer and signal number from
the coredump, we could run it on the server side in the future, as both
values should be available in a uReport. The major advantages of this
approach would be that we don't have to depend on GDB (apparently a
problem on RHEL) and that we can run improved versions of the script on
crashes received when this version was not available. The plugin doesn't
seem to be integrated with GDB very tightly, so this should be easy if
we keep this in mind.

Some remarks regarding the code below:

On Thu, Jun 13, 2013 at 14:01:11 +0200, Denys Vlasenko wrote:
> +_writing_instr = {
> +    # insn:N, where N:
> +    # -1: this insn always writes to memory
> +    # -2: writes to memory if any operand is a memory operand
> +    # 2:  writes to memory if 2nd (or later) operand is a memory operand
> +    #
> +    # Two-operand insns
> +    "add":2,
I suggest using something more descriptive, like assigning the numbers
to a variable, e.g. WRITES_ALWAYS = -1, and then using those.

> +def _fetch_insn_from_table(ins, table):
> +    if not ins:
> +        return None
> +    if ins in table.keys():
> +        return table[ins]
You can use just "ins in table".

> +    if self.operands.startswith("0x") or self.operands.startswith("-0x") or self.operands.startswith("("):
> +        mem_op_pos = 0
> +    else:
> +        mem_op_pos = self.operands.find(",0x")
> +        if mem_op_pos < 0:
> +            mem_op_pos = self.operands.find(",-0x")
> +        if mem_op_pos < 0:
> +            mem_op_pos = self.operands.find(":0x")
> +        if mem_op_pos < 0:
> +            mem_op_pos = self.operands.find(":-0x")
> +        if mem_op_pos < 0:
> +            mem_op_pos = self.operands.find(",(")
> +        if mem_op_pos < 0:
> +            mem_op_pos = self.operands.find(":(")
> +        if mem_op_pos < 0:
> +            return False # no memory operands
> +        mem_op_pos += 1
Can you use a regular expression for this, possibly with splitting the
operands into list first? http://docs.python.org/2.7/library/re.html

> +class _Signal_and_Insn:
> +    pass
As Jiri said, I'd make most of the functions method of this class. At
least you'll get rid of the underscores before their names;)

> +def _get_signal_and_instruction():
This one should probably be the constructor. As mentioned, I'd probably
move calls to the gdb module outside the class (but that can be done
when we actually need it).

> +        # HACK_ALERT: kernels before 3.?.? do not record siginfo in coredumps,
> +        # so $_siginfo isn't present.
By the way, do you have any reference for this? The prstatus structure
which contains a signal number seems to be present in coredumps at least
on 2.6.32. Or am I misunderstanding the problem?

> +    for line in raw_instructions.split("\n"):
> +        # line can be:
> +        # "Dump of assembler code from 0xAAAA to 0xBBBB:"
> +        # "[=>] 0x00000000004004dc[ <+0>]:  push   %rbp"
> +        #   (" <+0>" part is present when we run on a live process,
> +        #   on coredump it is absent)
> +        # "End of assembler dump."
> +        # "" (empty line)
What about using regular expressions here?

> +    elif self.signo == signal.SIGQUIT:
> +        self.exploitable_rating = 0
> +        self.exploitable_desc = "QUIT signal (Ctrl-\ pressed?)"
The backslash should probably be doubled or the whole string literal
preceded by the letter r. This was found by pylint, I can recommend
running it yourself, it can be useful sometimes.

> +    elif self.signo == signal.SIGBUS:
> +        self.exploitable_rating = 5
> +        self.exploitable_desc = "Access past the end of mapped file, invalid address, unaligned access, etc"
> +    #elif self.signo = signal.SIGfoo:
The giant if-else block looks ugly, but I can't think of a less ugly way
to replace it ...

> +        f = sys.stdout
The sys module is not imported (says pylint).


Cheers,
Martin


More information about the Crash-catcher mailing list