1244116 - Telemetry for mixed content requests by plugins

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Franziskus Kiefer [:franziskus]]

We should get telemetry for mixed content requests done by plugins.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally

Review commit: https://reviewboard.mozilla.org/r/35121/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35121/

Comment 2

[Tanvi Vyas[:tanvi]]

Comment on attachment 8719796 [details]
MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally

https://reviewboard.mozilla.org/r/35121/#review31735

::: dom/base/nsDocument.cpp:1563
(Diff revision 1)
> +      if (mHasMixedContentObjectSubrequest) {

Change this a bit.
if (mHasMixedContentObjectSubrequest) {
Telemetry::Accumulate(Telemetry::MIXED_CONTENT_OBJECT, 1 /* mixed object subrequest loaded on page*/);
} else {
Telemetry::Accumulate(Telemetry::MIXED_CONTENT_OBJECT, 0 /* no mixed object subrequests loaded on page*/);
}

::: dom/base/nsIDocument.h:2926
(Diff revision 1)
> +  // True if a document has blocked Mixed Object Content (see nsMixedContentBlocker.cpp)

// True if a document loads a plugin object that attempts to load mixed content subresources through necko (see nsMixedContentBlocker.cpp)

We want this variable to be true regardless of whether the object subrequest was blocked or loaded.  The blocked or loaded will be determined by the users about:config mixed content prefs and whether they clicked "disable protection".  Overall, we can assume they are loaded since mixed passive content is allowed by default, and right now they are all passive.  I'm not sure about the "through necko" part, that is to be clear that object subrequest loads that use the plugins network stack aren't included here

We could make this finer grained to future proof, as a describe below, but I think this is overkill:
We could have mHasMixedObjectSubrequestBlocked and mHasMixedObjectSubrequestLoaded.  Then our telemetry would have 3 values:
0 = no attempts to load mixed object subrequests
1 = attempted to load mixed object subrequests and they were blocked
2 = attempted to load mixed object subrequests and they loaded.

We would start off with a histogram with lots of 0's and a few 2's.  Then when we switch these resources to mixed active isntead of mixed passive, we should see that column 1 has the value that column 2 used to have.  If we see a spike in column 2, that means users are being forced to disable protection in order for their pages to work.

We see very few disable events, which is why I think this extra work isn't necessary.  If we see a spike in disable events after we change from passive to active, that will give us a big enough clue that this is impacting users and websites.

::: dom/security/nsMixedContentBlocker.cpp:780
(Diff revision 1)
> +  // set hasMixedDisplayContentBlocked on this object if necessary

// set hasMixedContentObjectSubrequest on this subresource if necessary

::: toolkit/components/telemetry/Histograms.json:7683
(Diff revision 1)
> +    "description": "How often do objects load insecure content on secure pages (counting pages, not objects)? 0=allObjects"

0=pages with no mixed object subrequests, 1=pages with mixed object subrequests

Comment 3

[Franziskus Kiefer [:franziskus]]

https://reviewboard.mozilla.org/r/35121/#review31735

> // True if a document loads a plugin object that attempts to load mixed content subresources through necko (see nsMixedContentBlocker.cpp)
> 
> We want this variable to be true regardless of whether the object subrequest was blocked or loaded.  The blocked or loaded will be determined by the users about:config mixed content prefs and whether they clicked "disable protection".  Overall, we can assume they are loaded since mixed passive content is allowed by default, and right now they are all passive.  I'm not sure about the "through necko" part, that is to be clear that object subrequest loads that use the plugins network stack aren't included here
> 
> We could make this finer grained to future proof, as a describe below, but I think this is overkill:
> We could have mHasMixedObjectSubrequestBlocked and mHasMixedObjectSubrequestLoaded.  Then our telemetry would have 3 values:
> 0 = no attempts to load mixed object subrequests
> 1 = attempted to load mixed object subrequests and they were blocked
> 2 = attempted to load mixed object subrequests and they loaded.
> 
> We would start off with a histogram with lots of 0's and a few 2's.  Then when we switch these resources to mixed active isntead of mixed passive, we should see that column 1 has the value that column 2 used to have.  If we see a spike in column 2, that means users are being forced to disable protection in order for their pages to work.
> 
> We see very few disable events, which is why I think this extra work isn't necessary.  If we see a spike in disable events after we change from passive to active, that will give us a big enough clue that this is impacting users and websites.

yeah, the necko part makes sense, added that one.

I agree that we probably don't need that third value. We get the same information (we're negatively impacting the user) with the values we have already.

Comment 4

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8719796 [details]
MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35121/diff/1-2/

Comment 5

[Tanvi Vyas[:tanvi]]

Comment on attachment 8719796 [details]
MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally

https://reviewboard.mozilla.org/r/35121/#review33061

r+ with the changes below.  I believe you also need a telemtry data privacy review, but I can no longer find documentation about that.  I'll post back if/when I find it.

::: dom/base/nsDocument.cpp:1565
(Diff revision 2)
> +        Telemetry::Accumulate(Telemetry::MIXED_CONTENT_OBJECT, 1);

Do we need the Telemetry:: prefix before Accumulate?  The two Accumulate methods above don't have it.  I missed this in the previous review.

::: dom/base/nsIDocument.h:612
(Diff revision 2)
> +   * Get mixed object content flag for this document.

Nit:
Get mixed content object subrequest flag for this document.

And

Set the mixed content object subrequest flag for this document.

::: dom/security/nsMixedContentBlocker.cpp:781
(Diff revision 2)
> +  if (objectSubrequest) {

Why not just check if aContentType == TYPE_OBJECT_SUBREQUEST, since we don't use the boolean for anything else.

::: toolkit/components/telemetry/Histograms.json:7680
(Diff revision 2)
> +  "MIXED_CONTENT_OBJECT": {

MIXED_CONTENT_OBJECT_SUBREQUEST

Comment 6

[Tanvi Vyas[:tanvi]]

Oh, and we need a /dom review.  So please r? smaug on the next patch.

Comment 7

[Tanvi Vyas[:tanvi]]

Here we go: https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval

Comment 8

[Franziskus Kiefer [:franziskus]]

(In reply to Tanvi Vyas [:tanvi] from comment #7)
> Here we go:
> https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval

that says I need feedback from a module owner or peer, but if I need review from one of them anyway that should be enough, right?

Comment 9

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8719796 [details]
MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35121/diff/2-3/

Comment 10

[Tanvi Vyas[:tanvi]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #8)
> (In reply to Tanvi Vyas [:tanvi] from comment #7)
> > Here we go:
> > https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval
> 
> that says I need feedback from a module owner or peer, but if I need review
> from one of them anyway that should be enough, right?

Yeah, its a little confusing.  But I think this means the module owner or peer of Data Collection.  They are listed at the top of the wiki page.  So feedback? to ally or benjamin.

Comment 11

[Tanvi Vyas[:tanvi]]

Comment on attachment 8719796 [details]
MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally

https://reviewboard.mozilla.org/r/35121/#review33197

::: dom/base/nsIDocument.h:566
(Diff revision 3)
> -   * Get mixed active content blocked flag for this document.
> +   * Get mixed content object subrequest flag for this document.

Looks like you changed the comment for the wrong getter/setter pair.

Comment 12

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8719796 [details]
MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35121/diff/3-4/

Comment 13

[Franziskus Kiefer [:franziskus]]

> Looks like you changed the comment for the wrong getter/setter pair.

oops, not sure what I was thinking there...

> Yeah, its a little confusing.  But I think this means the module owner or peer of Data Collection.  They are listed at the top of the wiki page.  So feedback? to ally or benjamin.

Ally, are you ok with this Telemetry patch? (no feedback? on reviewboard, so needinfo?)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8719796 [details]
MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally

https://reviewboard.mozilla.org/r/35121/#review33503

::: dom/base/nsIDocument.h:616
(Diff revision 4)
> +  bool GetHasMixedContentObjectSubrequest()

So we don't ever call GetHasMixedContentObjectSubrequest so why add it?

Comment 15

[Allison Naaktgeboren :ally]

I'm still travelling and fb won't seem to load properly in fennec. I will do the review tomorrow.

Comment 16

[Allison Naaktgeboren :ally]

(In reply to Allison Naaktgeboren :ally from comment #15)
> I'm still travelling and fb won't seem to load properly in fennec. I will do
> the review tomorrow.

s/fb/rb.  Thanks autocorrect. :/

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Allison Naaktgeboren :ally from comment #16)
> (In reply to Allison Naaktgeboren :ally from comment #15)
> > I'm still travelling and fb won't seem to load properly in fennec. I will do
> > the review tomorrow.
> 
> s/fb/rb.  Thanks autocorrect. :/

Please file a mozreview bug about that issue.

Comment 18

[Allison Naaktgeboren :ally]

Comment on attachment 8719796 [details]
MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally

https://reviewboard.mozilla.org/r/35121/#review33621

r+ with changes.

::: toolkit/components/telemetry/Histograms.json:7793
(Diff revision 4)
> +  "MIXED_CONTENT_OBJECT_SUBREQUEST": {

Please include the bug_numbers field. See https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#100  for an example.

::: toolkit/components/telemetry/Histograms.json:7795
(Diff revision 4)
> +    "expires_in_version": "never",

"never" is no longer an acceptable value for new probes. How about 55?  If at 55 the probe is still provably in use, it can be extended.

::: toolkit/components/telemetry/Histograms.json:7798
(Diff revision 4)
> +    "description": "How often do objects load insecure content on secure pages (counting pages, not objects)? 0=pages with no mixed object subrequests, 1=pages with mixed object subrequests"

uber nit: "How often do objects load" --> "How often objects load"

Comment 19

[Allison Naaktgeboren :ally]

The end of the commit comment should read r=smaug, p=ally. It's the new fangled convention from the creation of the data collection module.

Comment 20

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8719796 [details]
MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35121/diff/4-5/

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f09fc69822a5

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1244116 - Extend telemetry for MIXED_CONTENT_OBJECT_SUBREQUEST

Review commit: https://reviewboard.mozilla.org/r/214598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214598/

Comment 24

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8944399 [details]
Bug 1244116 - Extend telemetry for MIXED_CONTENT_OBJECT_SUBREQUEST

Ignore this, attached to wrong bug, sorry.