Strip newlines from MozCrashReason annotation

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: kats, Assigned: gsvelto)

Tracking

Trunk
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

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
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.
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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
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.
Review ping
Flags: needinfo?(ted)
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b791c9a47d33
Escape MozCrashReason when writing out a crash report r=ted
https://hg.mozilla.org/mozilla-central/rev/b791c9a47d33
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: needinfo?(ted)
You need to log in before you can comment on or make changes to this bug.