1138669 - (CVE-2015-2742) Some mac crash reports include private information

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[David Parks [:handyman]]

I'm working on bug 1124408 and the crash reports I'm viewing contain private information.  The bug is about a crash when using some password fields (like the password manager).  In the crash report, actual keys that the user pressed when they got the crash are plainly visible.  For example:

https://crash-stats.mozilla.com/report/index/5f846d1b-cff1-455d-86dc-6dd4e2150301

From the AppNotes section, I can see that this user has a 'k' in their password.  This information is valuable to the bug but also clearly private.  I don't know the policy here but I suspect this info should be removed from the report.

The message is logged from the Cocoa widget object.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I'm inclined to think this isn't really a problem.  We don't know the account name, and we only know one letter of the password.

Dan, what do you think?

Comment 2

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

I think we're OK here.

Comment 3

[Benjamin Smedberg]

What code is adding this to the crash report? Why do we need to record this information in the crash report?

I can certainly think of ways that sites could try to abuse this, given enough time. I think that if we need this data, we should move it into a private metadata field and not the public app notes.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

The code is here:

https://hg.mozilla.org/mozilla-central/annotate/993eb76a8bd6/widget/cocoa/nsChildView.mm#l5375

We're just recording the native key event that triggers a crash.

I really do think this is harmless.  But it may no longer be needed -- especially as we seem to be closing in on fixes for this class of crashes.

What do you think, David?

Comment 5

[David Parks [:handyman]]

Attachment: Remove theEvent from nsChildView crash log message

(In reply to Steven Michaud from comment #4)
>
> I really do think this is harmless.  But it may no longer be needed --
> especially as we seem to be closing in on fixes for this class of crashes.

I'd say 'closing in' is accurate, but the bugs I've been handling related to this are still popping up.  I don't know if its still got 'top crasher' status but I have been using the incidental reports of "I'm still seeing this crash" to determine if the core issue is resolved.  I'd really like to keep the crash.  That said, we can afford to drop theEvent from the log message.  Its utility is marginal.  Alternatively, we could just write the parts of the event that don't include key info.

This patch removes the entire event from the message, in case we decide to go that way.

Comment 6

[Benjamin Smedberg]

If you still need it, please put it in a separate annotation instead of the app notes.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Steven Michaud from comment #1)
> I'm inclined to think this isn't really a problem.  We don't know the
> account name, and we only know one letter of the password.

+1

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> If you still need it, please put it in a separate annotation instead of the
> app notes.

At least for bug 893973, we have a lot of reports but we still don't fine the hint from the inputting character. So, I think that removing input character from the log is okay but I'm not sure for the other developers.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8573539 [details] [diff] [review]
Remove theEvent from nsChildView crash log message

So none of the developers who've worked on this problem think the native key event is critically important to resolving this bug.  Then let's get rid of it, at least temporarily.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/133524b43f9c

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/133524b43f9c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Safe to call this wontfix for the ESRs?

Comment 12

[David Parks [:handyman]]

I'm not clear on how these calls are made.  Steven?

Comment 13

[Steven Michaud [:smichaud] (Retired)]

> Safe to call this wontfix for the ESRs?

Yes.  The code that was patched only runs in non-release builds.