788704 - Add telemetry for the enablePrivilege pref

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Andrew McCreight [:mccr8]]

I don't know if this is a good idea or not securitywise (quantifying how many people are exposed...) but froydnj says it would be easy to do if we think it is a good idea.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

Here's the patch to add telemetry on whether we've called enablePrivilege at all.
I don't know whether it would be worth trying to find out how many times it had been called; it sounds like we only care about whether enablePrivilege has been called.

This would be fairly trivial to uplift if we wanted some idea of how big of an effect enablePrivilege changes would have.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 660043 [details] [diff] [review]
patch

This looks right, though we need someone from security to make the call on whether we want to quantify this information. Flagging dveditz as a catch-all, but feel free to re-assign as appropriate.

Is there any way to make telemetry data private?

For this to be useful, we'll want to backport it to 17.

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 660043 [details] [diff] [review]
patch

Review of attachment 660043 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure the patch matches the original request.

mccr8 seems to be wanting to know "How many users have made themselves Pwnable"

the patch is measuring "How often are users running content that required them to turn on the pref", which is a subset of the question mccr8 was asking. This subset corresponds roughly to "How many will get made when we remove this entirely" so it is a useful number, but it's not quite the original "how unsafe are our users after bug 757046?"

There's a third question that would be interesting to know: "How much web content /attempts/ to use enablePrivilege whether or not users are vulnerable?".

Aside from questions of correctness--and that I find all 3 questions interesting to know, but especially the two you aren't answering ;-) -- I have no security or privacy concerns with this patch. Bug 757046 on the other hand...

Comment 4

[Daniel Veditz [:dveditz]]

> corresponds roughly to "How many will get made ..."

"How many will get MAD ..."

Comment 5

[Daniel Veditz [:dveditz]]

I think you wanted more of a sec-review+ than a r+ from me based on comment 2 so I'll set that instead and clear the patch r?

Comment 6

[Daniel Veditz [:dveditz]]

If something like the current patch landed on beta/aurora prior to the CAPS-ectomy then the accumulate in netscape_security_enablePrivilege would be measuring question 3: how many users encounter web content that calls enablePrivilege.

The only way to answer that question after bholley's changes would be to always hook up the netscape.security.enablePrivilege method and then check the pref each time it's called before doing something with it. That's probably not worth it, especially if we can get a rough answer to 3 by uplifting the probe to Aurora and maybe Beta ASAP.

For the uplift probe it'd be great to get a second flag accumulator after the  securityManager->EnableCapability(cap.ptr()); call if it succeeded. That way we'll know how much web content calls it and how many users agree to it. The first number is how much trouble users will be in if they flip the dire pref, and the second number might hint at how many users are likely to do so.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> If something like the current patch landed on beta/aurora prior to the
> CAPS-ectomy then the accumulate in netscape_security_enablePrivilege would
> be measuring question 3: how many users encounter web content that calls
> enablePrivilege.
> 
> The only way to answer that question after bholley's changes would be to
> always hook up the netscape.security.enablePrivilege method and then check
> the pref each time it's called before doing something with it. That's
> probably not worth it, especially if we can get a rough answer to 3 by
> uplifting the probe to Aurora and maybe Beta ASAP.

It'd need to be beta, because the CAPS-ectomy is already on aurora.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> For the uplift probe it'd be great to get a second flag accumulator after
> the  securityManager->EnableCapability(cap.ptr()); call if it succeeded.
> That way we'll know how much web content calls it and how many users agree
> to it. The first number is how much trouble users will be in if they flip
> the dire pref, and the second number might hint at how many users are likely
> to do so.

Given comment 7, is this still applicable?

Comment 9

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/703fb179c834

Comment 10

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/703fb179c834

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 660043 [details] [diff] [review]
patch

We'd like to uplift this to Aurora to get more information via Telemetry on how users are affected by the enablePrivilege removal.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None
Testing completed (on m-c, etc.): On m-c for a few days.
Risk to taking this patch (and alternatives if risky): Minimal risk.
String or UUID changes made by this patch: None.

Comment 12

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c51952868cb5

Comment 13

[Nathan Froyd [:froydnj]]

I'm not doing a version of this patch for beta unless/until I hear something addressing comment 6 and/or comment 7.