Closed Bug 1390547 Opened 3 years ago Closed 2 years ago
Strip newlines from Moz
Crash Reason annotation
46 bytes, text/x-phabricator-request
|Details | Review|
The code at  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 . 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.  http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/toolkit/crashreporter/nsExceptionHandler.cpp#1146-1154  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.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/b791c9a47d33 Escape MozCrashReason when writing out a crash report r=ted
You need to log in before you can comment on or make changes to this bug.