1390547 - Strip newlines from MozCrashReason annotation

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[Kartikaya Gupta (email:[e-mail])]

The code at [1] writes the MozCrashReason annotation to crash reports. However this crash reason can be pretty much arbitrary data (specially now that we've wired up rust panic messages to it).

If that message contains newlines, it can mess up the crash report submission because the report syntax is newline-delimited key=value pairs. In fact, by calling something like this:
  MOZ_CRASH_UNSAFE_PRINTF("\nkey=value);
in the code it's possible to generate a crash report with the key=value pair showing up as a separate entry in the metadata table of the crash report [2]. It might be possible to overwrite other keys in the crash report or generate invalid data such that the report itself gets dropped.

While this isn't particularly harmful it might result in bad crashdata, and anway it's kind of unexpected and probably something worth guarding against.

I didn't check the other annotation keys to see if their values can also end up with newlines injected into them; MozCrashReason might not be the only one.

[1] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/toolkit/crashreporter/nsExceptionHandler.cpp#1146-1154
[2] https://crash-stats.mozilla.com/report/index/3a9f0ec9-143a-448e-9ea8-edb260170815#tab-metadata note the "secondline" key i added with this technique. changeset is at https://hg.mozilla.org/try/rev/a9ff37d8bf33a83dbf475882ebaa1daf763942ed

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

This is pretty bad actually, we already had other problems WRT this, see bug 1395507 for a particularly egregious one. This is probably worth fixing but as Ted already mentioned multiple times we should really drop the INI-style format for the .extra file and just write out a JSON blob.

Comment 2

[(not currently active) Ted Mielczarek]

We escape newlines and backslashes in annotations added using `AnnotateCrashReport`:
https://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/toolkit/crashreporter/nsExceptionHandler.cpp#2235

`MozCrashReason` gets special handling so it misses all that, unfortunately.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Bug 1390547 - Escape MozCrashReason when writing out a crash report

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

The original patch did not fix the issue with content crashes. I've uploaded a new version that fixes the issue in content processes and introduces unit-tests covering both main and content process crashes.

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

Review ping

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b791c9a47d33
Escape MozCrashReason when writing out a crash report r=ted

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b791c9a47d33