Last Comment Bug 774395 - Measure prevalence of SSL MITM errors for Updates
: Measure prevalence of SSL MITM errors for Updates
Status: RESOLVED FIXED
[qa-]
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla18
Assigned To: Devdatta Akhawe [:devd]
:
Mentors:
Depends on:
Blocks: 715942
  Show dependency treegraph
 
Reported: 2012-07-16 11:54 PDT by Devdatta Akhawe [:devd]
Modified: 2012-10-16 16:26 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Measure prevalence of SSL MITM errors for updates (3.02 KB, patch)
2012-09-05 18:30 PDT, Devdatta Akhawe [:devd]
netzen: feedback+
Details | Diff | Splinter Review
Measure prevalence of SSL MITM errors for updates (3.01 KB, patch)
2012-09-07 17:49 PDT, Devdatta Akhawe [:devd]
dev.akhawe+mozilla: review+
Details | Diff | Splinter Review
Measure prevalence of SSL MITM errors for updates (2.98 KB, patch)
2012-09-07 18:47 PDT, Devdatta Akhawe [:devd]
no flags Details | Diff | Splinter Review
Measure prevalence of SSL MITM errors for updates (2.98 KB, patch)
2012-09-07 18:49 PDT, Devdatta Akhawe [:devd]
dev.akhawe+mozilla: review+
Details | Diff | Splinter Review
Measure prevalence of SSL MITM errors for updates (3.58 KB, patch)
2012-09-08 09:26 PDT, Devdatta Akhawe [:devd]
dev.akhawe+mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Devdatta Akhawe [:devd] 2012-07-16 11:54:05 PDT
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 [:rstrong] (use needinfo to contact me) 2012-07-16 12:02:39 PDT
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] 2012-09-05 18:30:10 PDT
Created attachment 658712 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates
Comment 3 Devdatta Akhawe [:devd] 2012-09-05 18:49:49 PDT
@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] 2012-09-07 16:25:07 PDT
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] 2012-09-07 17:49:39 PDT
Created attachment 659414 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates
Comment 6 Devdatta Akhawe [:devd] 2012-09-07 17:50:14 PDT
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] 2012-09-07 17:50:47 PDT
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] 2012-09-07 18:47:02 PDT
Created attachment 659429 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates
Comment 9 Devdatta Akhawe [:devd] 2012-09-07 18:49:40 PDT
Created attachment 659433 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates
Comment 10 Devdatta Akhawe [:devd] 2012-09-07 18:56:51 PDT
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] 2012-09-07 20:15:43 PDT
(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] 2012-09-08 09:26:56 PDT
Created attachment 659492 [details] [diff] [review]
Measure prevalence of SSL MITM errors for updates
Comment 13 Devdatta Akhawe [:devd] 2012-09-08 13:08:17 PDT
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 2012-09-10 14:16:33 PDT
Dev pushed this to try : https://tbpl.mozilla.org/?tree=Try&rev=bc2f144e83b3 - looks good.
Comment 15 Ian Melven :imelven 2012-09-10 14:20:26 PDT
Pushed to inbound : https://hg.mozilla.org/integration/mozilla-inbound/rev/4147ad8c7ab0
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-09-10 18:43:20 PDT
https://hg.mozilla.org/mozilla-central/rev/4147ad8c7ab0
Comment 17 Ian Melven :imelven 2012-09-12 13:55:10 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.