Closed Bug 1244116 Opened 4 years ago Closed 4 years ago

Telemetry for mixed content requests by plugins

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox47 --- affected

People

(Reporter: franziskus, Assigned: franziskus)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We should get telemetry for mixed content requests done by plugins.
Blocks: 1190623
Attachment #8719796 - Flags: review?(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
Attachment #8719796 - Flags: review?(tanvi)
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 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/
Attachment #8719796 - Attachment description: MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, f?tanvi → MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r?tanvi
Attachment #8719796 - Flags: review?(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
Attachment #8719796 - Flags: review?(tanvi) → review+
Oh, and we need a /dom review.  So please r? smaug on the next patch.
(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 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/
Attachment #8719796 - Attachment description: MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r?tanvi → MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r?smaug
Attachment #8719796 - Flags: review?(bugs)
(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 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.
Attachment #8719796 - Flags: review+
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/
> 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?)
Flags: needinfo?(a.m.naaktgeboren)
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?
Attachment #8719796 - Flags: review?(bugs) → review+
I'm still travelling and fb won't seem to load properly in fennec. I will do the review tomorrow.
Flags: needinfo?(a.m.naaktgeboren)
(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. :/
(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 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"
Attachment #8719796 - Flags: review+
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 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/
Attachment #8719796 - Attachment description: MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r?smaug → MozReview Request: Bug 1244116 - Telemetry for mixed content requests by plugins, r=smaug, p=ally
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Blocks: 1432162
Attachment #8944399 - Flags: review?(ckerschb)
Comment on attachment 8944399 [details]
Bug 1244116 - Extend telemetry for MIXED_CONTENT_OBJECT_SUBREQUEST

Ignore this, attached to wrong bug, sorry.
Attachment #8944399 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.