[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