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

Nikola Pajkovsky npajkovs at redhat.com
Thu Jun 13 15:08:44 UTC 2013


CC'ing Jan

Martin Milata <mmilata at redhat.com> writes:

> 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

-- 
Nikola


More information about the Crash-catcher mailing list