Measure prevalence of SSL MITM errors for Updates

RESOLVED FIXED in Firefox 17

Status

()

Toolkit
Application Update
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: devd, Assigned: devd)

Tracking

unspecified
mozilla18
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox17 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
It would be nice to know how many times updates fail because some middle box is trying to MITM the SSL traffic.
(Assignee)

Updated

5 years ago
Blocks: 715942
It would be a "good thing" to report whether it was a real MITM vs. a product causing a MITM such as bug 770594. It would be good to report all of the failure cases individually from:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/shared/CertUtils.jsm

per irc convo with devd, it is my understanding that security would like the focus to be on getting data for the case in bug 770594.
(Assignee)

Comment 2

5 years ago
Created attachment 658712 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates
(Assignee)

Updated

5 years ago
Attachment #658712 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

5 years ago
Assignee: nobody → dev.akhawe+mozilla
(Assignee)

Comment 3

5 years ago
@dveditz: The proposed patch will only measure cases where the MAXERROR count is exceeded and the warning is shown. Do you think this patch is sufficient, or should we measure every time an update connection fails*? I am not sure the latter is useful: captive portals might introduce a lot of noise in the data.
Attachment #658712 - Flags: review?(robert.bugzilla) → review?(netzen)
Comment on attachment 658712 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates

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

Thanks for the patch, r=me with the changes.

::: security/manager/boot/public/nsISecurityUITelemetry.idl
@@ +99,5 @@
> + * Firefox Update Errors: UI is only thrown after 
> + * repeated errors. We only measure when the UI is shown.
> + */
> +const uint32_t WARNING_INSECURE_UPDATE = 52;
> +const uint32_t WARNING_NO_SECURE_UPDATE = 53;

I think this file got a bit stale, please ensure values are still ok.

::: toolkit/mozapps/update/content/updates.js
@@ +1669,5 @@
>     */
>    onPageShow: function() {
>      gUpdates.setButtons(null, null, "okButton", true);
>      gUpdates.wiz.getButton("finish").focus();
> +    let secHistogram = Components.classes["@mozilla.org/base/telemetry;1"].

CoC["@mozilla.org/base/telemetry;1"]

@@ +1670,5 @@
>    onPageShow: function() {
>      gUpdates.setButtons(null, null, "okButton", true);
>      gUpdates.wiz.getButton("finish").focus();
> +    let secHistogram = Components.classes["@mozilla.org/base/telemetry;1"].
> +                                  getService(Ci.nsITelemetry).

Use CoI here and elsewhere, Ci isn't defined
Attachment #658712 - Flags: review?(netzen) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 659414 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates
(Assignee)

Updated

5 years ago
Attachment #658712 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Thanks! The extra values are from the patch at Bug 787738. I am hoping to land both these bugs together. Does that explain why the constants were not looking right to you? Or are you talking about something else?

Other issues fixed.
(Assignee)

Comment 7

5 years ago
Comment on attachment 659414 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates

carrying over r+ from netzen
Attachment #659414 - Flags: review+
(Assignee)

Comment 8

5 years ago
Created attachment 659429 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates
(Assignee)

Updated

5 years ago
Attachment #659414 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 659433 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates
(Assignee)

Updated

5 years ago
Attachment #659429 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Comment on attachment 659433 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates

carrying over r=netzen
Attachment #659433 - Flags: review+
(In reply to Devdatta Akhawe from comment #6)
> Thanks! The extra values are from the patch at Bug 787738. I am hoping to
> land both these bugs together. Does that explain why the constants were not
> looking right to you? Or are you talking about something else?
> 
> Other issues fixed.

Yes that explains it, thanks!
(Assignee)

Comment 12

5 years ago
Created attachment 659492 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates
(Assignee)

Updated

5 years ago
Attachment #659433 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Comment on attachment 659492 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates

new uuid for the idl, carrying over the R+
Attachment #659492 - Flags: review+

Comment 14

5 years ago
Dev pushed this to try : https://tbpl.mozilla.org/?tree=Try&rev=bc2f144e83b3 - looks good.

Comment 15

5 years ago
Pushed to inbound : https://hg.mozilla.org/integration/mozilla-inbound/rev/4147ad8c7ab0
https://hg.mozilla.org/mozilla-central/rev/4147ad8c7ab0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 17

5 years ago
Comment on attachment 659492 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: n/a
Testing completed (on m-c, etc.): has been on m-c since Monday
Risk to taking this patch (and alternatives if risky): should be very low - adding a new telemetry probe
String or UUID changes made by this patch: nsISecurityUITelemetry UUID change (interface was introduced in ff17, i believe)

we would like to uplift this probe to Aurora/17 as the rest of the security UI telemetry probes landed in 17 and we can then start collecting a comprehensive set of security UI data in 17.
Attachment #659492 - Flags: approval-mozilla-aurora?
Attachment #659492 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 18

5 years ago
 https://hg.mozilla.org/releases/mozilla-aurora/rev/bd535573e384

Updated

5 years ago
status-firefox17: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.