Closed
Bug 1244116
Opened 9 years ago
Closed 9 years ago
Telemetry for mixed content requests by plugins
Categories
(Core :: Security, defect)
Core
Security
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.
Updated•9 years ago
|
Blocks: MixedContentBlocker
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/35121/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35121/
Assignee | ||
Updated•9 years ago
|
Attachment #8719796 -
Flags: review?(tanvi)
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Comment 6•9 years ago
|
||
Oh, and we need a /dom review. So please r? smaug on the next patch.
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
(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?
Assignee | ||
Comment 9•9 years ago
|
||
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)
Comment 10•9 years ago
|
||
(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•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
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/
Assignee | ||
Comment 13•9 years ago
|
||
> 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 14•9 years ago
|
||
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+
Comment 15•9 years ago
|
||
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)
Comment 16•9 years ago
|
||
(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•9 years ago
|
||
(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•9 years ago
|
||
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+
Comment 19•9 years ago
|
||
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.
Assignee | ||
Comment 20•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 21•9 years ago
|
||
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 22•9 years ago
|
||
bugherder |
Comment 23•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/214598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214598/
Attachment #8944399 -
Flags: review?(ckerschb)
Updated•7 years ago
|
Attachment #8944399 -
Flags: review?(ckerschb)
Comment 24•7 years ago
|
||
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.
Description
•