774395 - Measure prevalence of SSL MITM errors for Updates

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Devdatta Akhawe [:devd]]

It would be nice to know how many times updates fail because some middle box is trying to MITM the SSL traffic.

Comment 1

[Robert Strong (they/them - no direct email)]

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.

Comment 2

[Devdatta Akhawe [:devd]]

Attachment: Measure prevalence of SSL MITM errors for updates

Comment 3

[Devdatta Akhawe [:devd]]

@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.

Comment 4

[Brian R. Bondy [:bbondy]]

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

Comment 5

[Devdatta Akhawe [:devd]]

Attachment: Measure prevalence of SSL MITM errors for updates

Comment 6

[Devdatta Akhawe [:devd]]

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.

Comment 7

[Devdatta Akhawe [:devd]]

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

carrying over r+ from netzen

Comment 8

[Devdatta Akhawe [:devd]]

Attachment: Measure prevalence of SSL MITM errors for updates

Comment 9

[Devdatta Akhawe [:devd]]

Attachment: Measure prevalence of SSL MITM errors for updates

Comment 10

[Devdatta Akhawe [:devd]]

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

carrying over r=netzen

Comment 11

[Brian R. Bondy [:bbondy]]

(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!

Comment 12

[Devdatta Akhawe [:devd]]

Attachment: Measure prevalence of SSL MITM errors for updates

Comment 13

[Devdatta Akhawe [:devd]]

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

new uuid for the idl, carrying over the R+

Comment 14

[Ian Melven :imelven]

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

Comment 15

[Ian Melven :imelven]

Pushed to inbound : https://hg.mozilla.org/integration/mozilla-inbound/rev/4147ad8c7ab0

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4147ad8c7ab0

Comment 17

[Ian Melven :imelven]

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.

Comment 18

[Ian Melven :imelven]

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