846489 - Create an SSL Error Reporting Mechanism

Status: RESOLVED FIXED

(Core :: Security, enhancement)

Description

[Kathleen Wilson]

Please create a certificate error reporting mechanism that will transmit and store the following information on a Mozilla server, allowing the data to be analyzed both automatically and manually.
- Domain of bad connection
- Error type (e.g. Pinning, domain mismatch, etc)
- Cert chain (at minimum, same data to distrust each cert in the chain)
- Request data (e.g. User Agent, IP, Timestamp)

Initially this reporting mechanism will be used to report, store, and analyze certificate pinning violations. In the future it could also be used for user-reported certificate errors, and other related concerns.

Certificate pinning is a mechanism by which site owners can specify a set of keys (actually fingerprints of the keys) such that in the next connection to the site, the set of keys in the certificate chain MUST intersect with the set of keys 'pinned' in the browser.
- https://bugzilla.mozilla.org/show_bug.cgi?id=744204
- https://wiki.mozilla.org/Security/Features/CA_pinning_functionality

When the set of keys in the certificate chain do not intersect with the set of keys 'pinned' in the browsers, then an alert will be generated and sent to Mozilla to be stored and analyzed. There may be some false alarms, but if a real issue (such as MITM) is identified, the security-group should be alerted for further action.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I'm assuming this bug is where the development for this feature is going, since the other bug (bug 846501) seems to be a project review tracking bug.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Jared - this adds a window that comes up when a certificate error is encountered. I was wondering if you could have a look. Thanks! (It's not entirely finished, but I thought I could get a head-start on an initial pass.)
(Here's the design, from Larissa: http://people.mozilla.org/~lco/ProjectSPF/Certificate_Messages/131209%20Wireframes.pdf )

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8375900 [details] [diff] [review]
patch

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

I really think we should re-consider using a popup window for this. We shouldn't be adding to the number of popup windows that users will see while using the browser.

I would prefer a checkbox within the current UI, unchecked by default, that when checked will send the information to Mozilla upon clicking on one of the buttons within the current UI. Larissa, what do you think about that?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

David, what do you think about implementing something that matches the interaction that I described in comment #3?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> David, what do you think about implementing something that matches the
> interaction that I described in comment #3?

I'm fine with that. I would still appreciate Larissa's input, though.

Comment 6

[Larissa Co [:lco]]

After talking with keeler, jaws, kathleen, and ibarlow, the solution we came up with was to use a doorhanger instead of the dialog box.

The doorhanger will appear to come from the site identity indicator (e.g. the globe/lock/etc). The user won't be able to access the notification again if he dismisses it; this is because the site identity panel is what usually shows up when you click on the indicator.

YES, this is somewhat of a hack. However, I think it's ok for the following reasons:
1. I would rather not create a new icon just for this special case, especially when there's no really value in allowing the user to revisit the doorhanger at a later point; this is a one-time prompt.
2. The number of users who will actually see this interface is quite low. Let's not go crazy optimizing for a corner case.
3. The doorhanger is a good compromise in drawing the user's attention without getting in their way.

I think that we should try this UI in Nightly and see how it goes in terms of getting feedback. We can adjust its visibility as necessary.

I don't have a great answer for where the doorhanger should be displayed (the Connection Untrusted page vs. in the redirect page). There are pros and cons to either case. However, my gut says that it should be displayed in the redirected page (i.e. either the Mozilla home page that the user sees when he clicks "get me out of here", or the page he's trying to visit when he adds an exception). It's a bit overwhelming for the user to encounter a page with two messages from the system: one warning him that the connection is untrusted, and another asking him to report the error. So I would rather that he see the option to report the error once he's redirected.

We'll have to tweak the message in the dialog a little though. How about this:

**Report the Untrusted Connection to Mozilla?**

Sharing the address and certificate information for the site you were trying to access (http://ulik2.accv.es) will help us identify and block malicious sites. Learn more...

Comment 7

[Tanvi Vyas[:tanvi]]

(In reply to Larissa Co [:lco] from comment #6)
> After talking with keeler, jaws, kathleen, and ibarlow, the solution we came
> up with was to use a doorhanger instead of the dialog box.
> 
> The doorhanger will appear to come from the site identity indicator (e.g.
> the globe/lock/etc). The user won't be able to access the notification again
> if he dismisses it; this is because the site identity panel is what usually
> shows up when you click on the indicator.
> 
> YES, this is somewhat of a hack. However, I think it's ok for the following
> reasons:
> 1. I would rather not create a new icon just for this special case,
> especially when there's no really value in allowing the user to revisit the
> doorhanger at a later point; this is a one-time prompt.
> 2. The number of users who will actually see this interface is quite low.
> Let's not go crazy optimizing for a corner case.
> 3. The doorhanger is a good compromise in drawing the user's attention
> without getting in their way.
> 
> I think that we should try this UI in Nightly and see how it goes in terms
> of getting feedback. We can adjust its visibility as necessary.
> 
> I don't have a great answer for where the doorhanger should be displayed
> (the Connection Untrusted page vs. in the redirect page). There are pros and
> cons to either case. However, my gut says that it should be displayed in the
> redirected page (i.e. either the Mozilla home page that the user sees when
> he clicks "get me out of here", or the page he's trying to visit when he
> adds an exception). It's a bit overwhelming for the user to encounter a page
> with two messages from the system: one warning him that the connection is
> untrusted, and another asking him to report the error. So I would rather
> that he see the option to report the error once he's redirected.
> 
> We'll have to tweak the message in the dialog a little though. How about
> this:
> 
> **Report the Untrusted Connection to Mozilla?**
> 
> Sharing the address and certificate information for the site you were trying
> to access (http://ulik2.accv.es) will help us identify and block malicious
> sites. Learn more...

Hi Larissa!  I took a look at your proposal and have some questions.  Perhaps a mock up would help answer some of them.

* Will the doorhanger automatically pop up or is it dismissed by default?  

* The code that pops up and shows doorhangers is separate from the code that shows the lock/globe icon.  From the above proposal, it sounds like we want to use the lock as a doorhanger.  Would this mean we don't use PopupNotifications.jsm?  If this is a onetime notification that is not encountered very often, it might be easier from a development stand point to just add another icon and use the doorhanger functionality.  It sounds like you have already discussed this with jaws.  If he is okay with adding doorhanger-like code to the site identity box, then that's fine.  The consequence is that everytime the look and appearance of doorhangers change, we will also have to make sure we change the code for this to match it (example bug 967349 / bug 864160).
* What would happen to the site identify information in the site identity box?  Would it be above or below the request to report the site?  Or will the site identify information be replaced by the reporting content the first time the user clicks on the lock?  And after dismissal, the user can see the site identity information again?

Thanks!

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tanvi Vyas [:tanvi] from comment #7)
>

(Please use needinfo to make sure that someone is flagged and questions don't get lost in bugmail)

> * Will the doorhanger automatically pop up or is it dismissed by default?  

Let's make it automatically pop up.

> * The code that pops up and shows doorhangers is separate from the code that
> shows the lock/globe icon.  From the above proposal, it sounds like we want
> to use the lock as a doorhanger.  Would this mean we don't use
> PopupNotifications.jsm?  If this is a onetime notification that is not
> encountered very often, it might be easier from a development stand point to
> just add another icon and use the doorhanger functionality.  It sounds like
> you have already discussed this with jaws.  If he is okay with adding
> doorhanger-like code to the site identity box, then that's fine.  The
> consequence is that everytime the look and appearance of doorhangers change,
> we will also have to make sure we change the code for this to match it
> (example bug 967349 / bug 864160).

Is that true? Can't we show a doorhanger and just set the anchor of the doorhanger to be the site identity icon?

> * What would happen to the site identify information in the site identity
> box?  Would it be above or below the request to report the site?  Or will
> the site identify information be replaced by the reporting content the first
> time the user clicks on the lock?  And after dismissal, the user can see the
> site identity information again?

The reporting mechanism popup would be opened automatically. If it gets dismissed and the user clicks on the site identity icon, then the site identity doorhanger will open.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I don't have time to work on this at the moment, so we should find a new owner.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Tanvi, can you take this?

Comment 11

[Tanvi Vyas[:tanvi]]

Not unless I postpone my current project.  Will talk to my team and get back to you this week.  If I can't take it, I will mark the UI parts of this bug for firefox-backlog review (perhaps using a separate bug to track the front-end work).

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Tanvi, can you take this?

Comment 12

[Tanvi Vyas[:tanvi]]

Oops, looks like I did that already.  Removing the backlog flag for now.

Comment 13

[Tanvi Vyas[:tanvi]]

A few more questions...

Does anyone have a copy of http://people.mozilla.org/~lco/ProjectSPF/Certificate_Messages/131209%20Wireframes.pdf ?  The link no longer works.

What buttons are available in the doorhanger?
- Is the "Learn More..." a link in the text like it is here - https://people.mozilla.org/~tvyas/FigureB.jpg
- What does the "report" button say?  Is it a drop down or a single 'report this page' button?

Does anyone have feedback on Larissa's proposed text -
**Report the Untrusted Connection to Mozilla?**

Sharing the address and certificate information for the site you were trying to access (http://ulik2.accv.es) will help us identify and block malicious sites. Learn more...

Comment 14

[Tanvi Vyas[:tanvi]]

Since lco is no longer at mozilla, maybe we should need-info someone else for the questions in comment 13.  Flagging Madhava to see if he can route this to someone.

Comment 15

[Garrett Robinson [:grobinson]]

Unfortunately, recent changes to the cert error flow for certain cases may invalidate the proposed user flow from Comment 6.

The proposed flow assumes that every cert error page has two options: a "Get me out of here", which redirects to about:home, and some bypass mechanism ("I Understand the Risks"). The "report error" doorhanger is meant to be shown *after* one of these two choices is made.

However, pinning violations and revoked certificates do not allow the user to bypass the warning and access the site anyway. At the moment, they are shown a different UI (see bug 1011638), where the only choice they have is to reload the page. There is no "I Understand the Risks", nor a "Get me out of here". In the case of pinning, this makes sense if the pinning error is due to a captive portal. However, it makes it unclear as to at which point in the flow the "Report an Error to Mozilla" message should be shown.

IMO, the best place might be on the about:certerror page itself. It already directs users to access a non-existent Help menu item to report errors... (bug 1014282)

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Rerouting to Philipp... Philipp, can you take a look at this bug and help out with Tanvi's questions?

Comment 17

[Tanvi Vyas[:tanvi]]

(In reply to Garrett Robinson [:grobinson] from comment #15)
> IMO, the best place might be on the about:certerror page itself.

Sounds like the proposal in comment 6 would work for non-pinning errors only.  Since this bug was intended to address pinning first, we need another flow.  What was the reason we tried to avoid adding a report button in the cert error page itself?

Comment 18

[Garrett Robinson [:grobinson]]

> Since this bug was intended to address pinning first, we need another flow.

Yup, exactly.

> What was the reason we tried to avoid adding a report button in the cert error page itself?

No idea, I wasn't involved with the first go-round on this bug. I think it's a reasonable thing to try.

Comment 19

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Reading this thread, my first intuition is also to add the option to the error page itself.
Is this only spawned from about:certerror or also from other error pages?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tanvi Vyas [:tanvi] from comment #17)
> What was the reason we tried to avoid adding a report button in the
> cert error page itself?

The goal was to increase responses, at the expense of making this an obtrusive alert. It started out as a proposal for a modal dialog, which I then negotiated to get moved to a doorhanger. I would be in favor of moving it to the page content, at the expense of lower responses but an improved user experience for people who don't want their task continuity broken.

Comment 21

[Garrett Robinson [:grobinson]]

Attachment: 846489-ssl-error-reporting.patch

I got started with an "on-page" (aboutCertError.xhtml) UI today. I got stuck on extracting the SSLStatus in browser.js::onAboutCertError in order to prepare the report. This is a long-standing problem with no easy solution - see comments in the patch for ideas on how to move forward.

Comment 22

[Tanvi Vyas[:tanvi]]

(In reply to Philipp Sackl [:phlsa] from comment #19)
> Reading this thread, my first intuition is also to add the option to the
> error page itself.
> Is this only spawned from about:certerror or also from other error pages?

Only the cert error page.

Comment 23

[Garrett Robinson [:grobinson]]

Attachment: 846489-ssl-error-reporting.patch

Uses nsIRecentBadCerts to get the certificate that caused the error. It's also used to get the cert when adding an exception, so it kind of makes sense in this context. We can revisit a "better" way to do this stuff (e.g. persist the certificate info on the docshell) later, in a different bug.

Comment 24

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8433729 [details] [diff] [review]
846489-ssl-error-reporting.patch

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

::: browser/base/content/browser.js
@@ +2323,5 @@
> +         *
> +         * We have 3 options here:
> +         * 1. use recentBadCertService, which "has its own problems" according
> +         *    to keeler
> +         * 2. xhr to the domain in question, and pull the cert off of that.

The certificate that nsIRecentBadCertService returns may not be the same certificate that caused the connection to fail. That is, it is inherently racy and shouldn't be used for anything other than its intended purpose. I appreciate the attempt to expedite the solution for this bug, but it isn't good to add race conditions here and it isn't good to make it harder to remove nsIRecentBadCertService than it already is. Please try to find an alternative solution.

Comment 25

[[:mmc] Monica Chew (no longer reading bugmail)]

How does the certificate viewer get the cert to show?

Comment 26

[Garrett Robinson [:grobinson]]

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #25)
> How does the certificate viewer get the cert to show?

It uses nsIRecentBadCertService, and falls back to sending an XHR and checking the channel if that doesn't work: http://dxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/exceptionDialog.js#119

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Garrett Robinson [:grobinson] from comment #26)
> (In reply to [:mmc] Monica Chew (please use needinfo) from comment #25)
> > How does the certificate viewer get the cert to show?
> 
> It uses nsIRecentBadCertService, and falls back to sending an XHR and
> checking the channel if that doesn't work:
> http://dxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/
> content/exceptionDialog.js#119

That's the exception dialog, not the cert viewer, which is also broken (many bugs filed against it). The cert exception dialog box hasn't been a high priority to fix because we (I) have decided that improvements to the cert error override mechanism should have very low priority. But, I think that this mechanism requires a higher level of correctness considering its intended purpose.

Perhaps describing some cases of this problem will lead to a solution. It is pretty obvious what to do if we're making a single connection to a load a page and that connection has a key pinning violation--we show the error page and we prompt in some manner to report the error to our reporting service using the entire certificate chain that was given in the TLS handshake. Right?

Even if nsIRecentBadCertService weren't racy and otherwise problematic, you *cannot* get the certificates other than the end-entity certificate from it. Thus, you cannot send back a complete report for even the simplest case.

Now, consider more complex cases: Let's say we try to load the same site in two tabs and we create TWO connections to do so; further, let's say one fails due to a key pinning violation and another one fails due to some other issue. IIUC, nsIRecentBadCertService will have trouble here because getRecentBadCert can only return *one* bad cert. But, what if the key pinning violation occurred before the other issue? Then the key pinning violation would be hidden and/or we'd send the wrong cert (chain) in the report.

Note also that we don't show the cert error page when the main page loads but a sub-resource fails to load due to a cert error--the cert error just causes the subresource to silently fail to load. And, note that especially with subresources we could have many connections happening simultaneously and/or in quick succession for a single hostname, some of which may have zero cert errors, some of which may have key pinning violations, and some of which may have other cert errors. nsIRecentBadCertService just doesn't handle this well at all, and it doesn't really need to, since its original intended purpose was solely to implement the Tools > Options > Advanged > Manage Certificates > Servers > Add Exception UI. Note that the addition of that "Add Exception" button is what made the certificate error exception dialog box so ugly and confusing. My opinion is that we should remove that button from Gecko along with all the associated badness, including in particular ns[I]RecentBadCert*.

Now, it seems to me that your reporting mechanism should try to report *all* the distinct cert chains that failed due to key pinning violation. I suggest that you change PublicKeyPinningService so that it saves all the violations in its own (in-memory) storage, and then implement a new GetAllPinningViolationsForHost(nsACString hostname) method that returns *all* the stored information, for all ports (since your UI prompt doesn't need to bother the user about ports), including in particular *all* of the relevant certificates instead of the last known bad end-entity certificate for the hostname. Note that you need to be aware of the potential for the accumulating key pinning violations to consume too much memory and deal with that by limiting the amount of info that you store. I believe this would be a better solution to the problem that you are trying to solve.

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #27)
> I believe this would be
> a better solution to the problem that you are trying to solve.

...while not being much harder to do than using nsIRecentBadCertService.

Comment 29

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Hey, sorry for letting this slip. You still need some UI work done here, right?

Comment 30

[Garrett Robinson [:grobinson]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #27)
> Even if nsIRecentBadCertService weren't racy and otherwise problematic, you
> *cannot* get the certificates other than the end-entity certificate from it.
> Thus, you cannot send back a complete report for even the simplest case.

I assumed I would be able to use nsIX509Cert.getChain [0]. Is there something wrong with that?

[0] http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/public/nsIX509Cert.idl?from=nsIX509Cert.idl#181

> Now, consider more complex cases: Let's say we try to load the same site in
> two tabs and we create TWO connections to do so; further, let's say one
> fails due to a key pinning violation and another one fails due to some other
> issue. IIUC, nsIRecentBadCertService will have trouble here because
> getRecentBadCert can only return *one* bad cert. But, what if the key
> pinning violation occurred before the other issue? Then the key pinning
> violation would be hidden and/or we'd send the wrong cert (chain) in the
> report.

Can we collect multiple failures for the same hostname:port, and report them all (let the analysts sort it out)?

> Note also that we don't show the cert error page when the main page loads
> but a sub-resource fails to load due to a cert error--the cert error just
> causes the subresource to silently fail to load. And, note that especially
> with subresources we could have many connections happening simultaneously
> and/or in quick succession for a single hostname, some of which may have
> zero cert errors, some of which may have key pinning violations, and some of
> which may have other cert errors. nsIRecentBadCertService just doesn't
> handle this well at all, and it doesn't really need to, since its original
> intended purpose was solely to implement the Tools > Options > Advanged >
> Manage Certificates > Servers > Add Exception UI. Note that the addition of
> that "Add Exception" button is what made the certificate error exception
> dialog box so ugly and confusing. My opinion is that we should remove that
> button from Gecko along with all the associated badness, including in
> particular ns[I]RecentBadCert*.

I interpreted the focus of this bug as reporting validation errors for resources (not resources) and extending the existing UI to do so. Reporting validation errors for sub-resources would require entirely new UI and add significant complexity. I'd prefer to focus on reporting errors for documents and, if people want it, develop reporting UI for sub-resources in a follow-up.

> Now, it seems to me that your reporting mechanism should try to report *all*
> the distinct cert chains that failed due to key pinning violation. I suggest
> that you change PublicKeyPinningService so that it saves all the violations
> in its own (in-memory) storage, and then implement a new
> GetAllPinningViolationsForHost(nsACString hostname) method that returns
> *all* the stored information, for all ports (since your UI prompt doesn't
> need to bother the user about ports), including in particular *all* of the
> relevant certificates instead of the last known bad end-entity certificate
> for the hostname. Note that you need to be aware of the potential for the
> accumulating key pinning violations to consume too much memory and deal with
> that by limiting the amount of info that you store. I believe this would be
> a better solution to the problem that you are trying to solve.

keeler points out that doing this would probably lead to false positives, since our path building algorithm may build multiple unsuccessful chains before giving up (especially in the case of cross-signing). I think we only want to report the chain that was presented in the certificate that failed to validate (let me know if I am misunderstanding you).

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Garrett Robinson [:grobinson] from comment #30)
> (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response)
> from comment #27)
> > Even if nsIRecentBadCertService weren't racy and otherwise problematic, you
> > *cannot* get the certificates other than the end-entity certificate from it.
> > Thus, you cannot send back a complete report for even the simplest case.
> 
> I assumed I would be able to use nsIX509Cert.getChain [0]. Is there
> something wrong with that?
> 
> [0]
> http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/public/
> nsIX509Cert.idl?from=nsIX509Cert.idl#181

It is bad to use nsIX509Cert.getChain. First, nsIX509Cert.getChain has the same kind of race conditions that nsIRecentBadCertService has in the face of different concurrent (or even serial) connections that have different certificate chains. Secondly, nsIX509Cert.getCert is only "guaranteed" to see the relevant CA certificates for *valid* (accepted), not *invalid* (rejected) certificates, because it requires the CA certificates to be a CERTCertificate object to be in memory and/or it requires the CA certificate to be written in the certificate database, and we explicitly avoid doing those things for invalid (rejected) certificates. Also, nsXI50Cert.getChain returns a *constructed* certificate chain, but for error reporting you should be reporting the *received* certificates. Finally, nsXI50Cert.getChain can return chains with certificates received on connections to other hosts, so your permission prompt to ask the user's permission to send the certificates received for a connection to host example.org would be misleading in its privacy implications.

tl;dr: Never use nsIX509Cert.getChain for anything. See bug 867473 about the planned removal of this horrific code.

> Can we collect multiple failures for the same hostname:port, and report them
> all (let the analysts sort it out)?

That's what I was recommending. (And, like I said, I suggest not keying things on the port, but just the hostname.)

> I interpreted the focus of this bug as reporting validation errors for
> resources (not resources) and extending the existing UI to do so. Reporting
> validation errors for sub-resources would require entirely new UI and add
> significant complexity. I'd prefer to focus on reporting errors for
> documents and, if people want it, develop reporting UI for sub-resources in
> a follow-up.

I don't have an opinion about that. Note that my points about race conditions and reporting the wrong certificates and/or reporting incomplete information still stand even without considering subresources.

> > Now, it seems to me that your reporting mechanism should try to report *all*
> > the distinct cert chains that failed due to key pinning violation.

> keeler points out that doing this would probably lead to false positives,
> since our path building algorithm may build multiple unsuccessful chains
> before giving up (especially in the case of cross-signing). I think we only
> want to report the chain that was presented in the certificate that failed
> to validate (let me know if I am misunderstanding you).

Sorry, I was unclear. When I said "report *all* the distinct cert chains that failed due to key pinning violation" I meant "report the contents of the TLS Certificate message for all TLS connections that failed due to key pinning violation." The emphasis should be on reporting the *received* certificate chains, not *constructed* certificate chains.

(Actually, to report the most complete information, you should report the union of the received and constructed chains, but that is complicated by the requirement to ask the user permission on a domain-by-domain basis for sending the information. Eventually we should make it so that a connection to hostname X never affects the connection to hostname y as far as cert chain construction is concerned. FWIW, I am working on something that should enable that, eventually.)

Comment 32

[Garrett Robinson [:grobinson]]

Attachment: 846489_ssl_error_reporting.patch

Now that we have the failed channel available on the docShell (thanks, keeler!), I'm returning to my original plan of getting the necessary info from the channel that caused the error page to appear.

Need to expose the peer certificate chain on nsISSLStatus (bug 1029155), as discussed with keeler and cviecco on IRC, to continue with this approach.

Comment 33

[Garrett Robinson [:grobinson]]

Now the URL is Philipp's sweet UI design, check it out!

Comment 34

[Garrett Robinson [:grobinson]]

Attachment: 846489_neterror_ui.patch

Implemented the UI (see URL) for certificate error reporting in netError.xhtml, so it can be shown for pinning errors. You can see how the "Report an error" link only appears for pinning errors by visiting https://pinningtest.appspot.com.

There are still a few minor tweaks needed.

* I wasn't able to get the "automatically report in the future" checkbox to autofocus, so it didn't make sense to add the blue glow shown in the mockup. Can revisit this later, but I think the glow is a UI hint that should only be present in the button is in some way special. If it were autofocused, you'd be able to toggle it with the spacebar.
* Showing the error reporting panel shifts the entire layout up. It should instead dropdown from the link without causing it to shift.

Comment 35

[Garrett Robinson [:grobinson]]

Attachment: 846489_expose_error_code_on_transport_security_info.patch

We need to expose the underlying error code from NSS on TransportSecurityInfo so it can be included in the error report.

Comment 36

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8472721 [details] [diff] [review]
846489_expose_error_code_on_transport_security_info.patch

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

LGTM.

::: netwerk/socket/nsITransportSecurityInfo.idl
@@ +12,4 @@
>  interface nsITransportSecurityInfo : nsISupports {
>      readonly attribute unsigned long    securityState;
>      readonly attribute wstring          errorMessage;
> +    readonly attribute long             errorCode;

Maybe add a comment that this is a PRErrorCode, just so we don't get confused later.

Comment 37

[Garrett Robinson [:grobinson]]

Attachment: 846489_expose_error_code_on_transport_security_info.patch

Comment on error code type in .idl. Add commit message for landing.

Comment 38

[[:mmc] Monica Chew (no longer reading bugmail)]

Hey Garrett,

Is there more WIP that's not reflected in the patches attached? mgoodwin was asking on irc.

Thanks,
Monica

Comment 39

[Mark Goodwin [:mgoodwin]]

Attachment: 846489_neterror_ui.patch

Unbitrotted version of Garrett's WIP.

Comment 40

[Mark Goodwin [:mgoodwin]]

Attachment: 846489_neterror_ui.patch

Gavin, I was wondering; could you take a look for feedback while I finish tests?

Changes for Garrett's WIP:
Moved the actual error reporting to content.js - there's no need for any of this to live in the parent process and moving things to content removes the need for much message passing (inc. a sync send to browser.js) and greatly simplifies things.

Added code to support the 'automatically send reports' feature in the UI.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8495919 [details] [diff] [review]
846489_neterror_ui.patch

redirecting to bsmedberg for feedback on the approach/data reporting aspects, felipe for the code.

Comment 42

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8495919 [details] [diff] [review]
846489_neterror_ui.patch

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

Code looks good. The strings in aboutNetERror.xhtml will need to be put in the .dtd accordingly.

It looks like this should properly work with e10s, but can you try it to make sure?

::: browser/base/content/content.js
@@ +121,5 @@
> +    chromeGlobal.addEventListener('AboutNetErrorSendReport', this, false, true);
> +  },
> +
> +  get isAboutNetError() {
> +    return content.document.documentURI.split('?')[0].toLowerCase() == "about:neterror";

you could just use documentURI.startsWith("about:neterror")

@@ +187,5 @@
> +    }
> +
> +    let docShell = contentDoc.defaultView.QueryInterface(Ci.nsIInterfaceRequestor)
> +        .getInterface(Ci.nsIWebNavigation)
> +        .QueryInterface(Ci.nsIDocShell);

there's a top level docShell variable available here in framescripts, you can use it directly.

Comment 43

[Mark Goodwin [:mgoodwin]]

Thanks for looking at this Filipe.

(In reply to :Felipe Gomes from comment #42)
> It looks like this should properly work with e10s, but can you try it to
> make sure?

My changes should work fine - problem is, aboutNetError was broken already with e10s. I'd like to make the rest work too but I think that fixing this here would complicate things. I already filed bug 1069317 for this - do you think it would be OK if I dealt with all of the e10s things in that bug instead?

Comment 44

[:Felipe Gomes (needinfo for replies!)]

Yeah sure. Although I'll note that not everything in about:neterror is broken.. There was already some work gone into it.. But I understand it's not completely working yet

Comment 45

[Mark Goodwin [:mgoodwin]]

Attachment: 03_846489_tests.patch

Initial attempt at tests

Comment 46

[Benjamin Smedberg]

Comment on attachment 8495919 [details] [diff] [review]
846489_neterror_ui.patch

Is this the data collection for pin failures that I discussed previously with dougt and mmc, or is this something else?

Please document the data that gets sent using in-tree docs similar to http://mxr.mozilla.org/mozilla-central/source/services/healthreport/docs/ I'd much prefer reviewing docs for data collection and then the code reviewers can make sure the code matches the docs.

What is data.mozilla.com? Is it pinned already? What is your data retention and reporting plan?

Comment 47

[Benjamin Smedberg]

And I'm sorry it took so long to reply, I missed this request when it first came through.

Comment 48

[Mark Goodwin [:mgoodwin]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #46)
> Comment on attachment 8495919 [details] [diff] [review]
> 846489_neterror_ui.patch
> 
> Is this the data collection for pin failures that I discussed previously
> with dougt and mmc <snip>?

Yes.

> Please document the data that gets sent using in-tree docs similar to
> http://mxr.mozilla.org/mozilla-central/source/services/healthreport/docs/
> I'd much prefer reviewing docs for data collection and then the code
> reviewers can make sure the code matches the docs.

I shall do.

> What is data.mozilla.com? Is it pinned already? 

See bug 963677

It's not pinned already. I can file a separate bug for this if you'd like.

> What is your data retention and reporting plan?

The data will, initially at least, only be retained for a short period of time (5 days). We don't have a concrete reporting plan yet; initially, we're interested in discovering what (and how much) we're breaking with pinning. Later on, we might like to use the data to alert us to other problems.

Comment 49

[Mark Goodwin [:mgoodwin]]

Attachment: 846489_neterror_ui.patch

Modified aboutNetError.xhtml and aboutNetError.css to have separate elements for all messages and buttons to allow for l10n to take place via netError.dtd.

Added strings to netError.dtd

Changed content.js to show and hide different elements rather than change textContent on a single element.

Changed content.js to address feedback in comment #42

Comment 50

[Mark Goodwin [:mgoodwin]]

Attachment: 03_846489_tests.patch

Added tests to ensure:
1) the report panel is hidden by default and appears when the user expands the section
2) selecting the 'automatically send' checkbox sets the relevant pref
3) reports are sent when the 'report' button is clicked
4) that the reports contain the correct certificate chain information
5) reports are not sent when the security.ssl.errorReporting.enabled pref is disabled
6) that reports are sent without the user clicking 'report' if security.ssl.errorReporting.automatic is set (i.e. the user has previously clicked the 'automatically send' checkbox)

Comment 51

[Mark Goodwin [:mgoodwin]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #46)
> What is data.mozilla.com? Is it pinned already?

So after thinking about this for a while and talking with David Keeler, I'm not sure it makes sense to pin this. The reason being that if an attacker is in a position to carry out a MITM attack on this, they'll already be in a position to observe the condition that resulted in an error report being sent.

Do you agree with this assessment?

Comment 52

[Benjamin Smedberg]

That depends on what data the report contains. If an attacker is MITM and could use this mechanism to extract more data from the user than simply the fact that they tried (and failed) to visit a particular SSL cert, that seems concerning to me.

Long-term, automatic retry of the ping sounds like the right answer (and will fit right into future telemetry architecture). I presume that you haven't built an auto-retry mechanism into the current code? Perhaps once the payload is documented it will be more clear whether there's an attack vector to worry about.

Comment 53

[Mark Goodwin [:mgoodwin]]

Attachment: 846489_neterror_ui.patch

1) Added in-tree docs for the feature, the prefs and the report data.
2) Added the URL of the SUMO document in the 'Learn more' link in about:netError

Do the supplied docs help at all for the pinning discussion?

Comment 54

[Benjamin Smedberg]

Comment on attachment 8506399 [details] [diff] [review]
846489_neterror_ui.patch

>diff --git a/browser/base/content/docs/sslerrorreport/dataformat.rst b/browser/base/content/docs/sslerrorreport/dataformat.rst

>+An example report::
>+
>+  {
>+    "timestamp":1413490449366,
>+    "errorCode":-16384,
>+    "errorMessage":"An error occurred during a connection to include-subdomains.pinning.example.com.\n\nThe server uses key pinning (HPKP) but no trusted certificate chain could be constructed that matches the pinset. Key pinning violations cannot be overridden.\n\n(Error code: mozilla_pkix_error_key_pinning_failure)\n",
>+    "failedCertChain":[
>+        "MIIDCjCCAfKgAwIBAgIENUiGYDANBgkqhkiG9w0BAQsFADAmMSQwIgYDVQQDExtBbHRlcm5hdGUgVHJ1c3RlZCBBdXRob3JpdHkwHhcNMTQxMDAxMjExNDE5WhcNMjQxMDAxMjExNDE5WjAxMS8wLQYDVQQDEyZpbmNsdWRlLXN1YmRvbWFpbnMucGlubmluZy5leGFtcGxlLmNvbTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALxYrge8C4eVfTb6/lJ4k/+/4J6wlnWpp5Szxy1MHhsLB+LJh/HRHqkO/tsigT204kTeU3dxuAfQHz0g+Td8dr6KICLLNVFUPw+XjhBV4AtxV8wcprs6EmdBhJgAjkFB4M76BL7/Ow0NfH012WNESn8TTbsp3isgkmrXjTZhWR33vIL1eDNimykp/Os/+JO+x9KVfdCtDCrPwO9Yusial5JiaW7qemRtVuUDL87NSJ7xokPEOSc9luv/fBamZ3rgqf3K6epqg+0o3nNCCcNFnfLW52G0t69+dIjr39WISHnqqZj3Sb7JPU6OmxTd13ByoLkoM3ZUQ2Lpas+RJvQyGXkCAwEAAaM1MDMwMQYDVR0RBCowKIImaW5jbHVkZS1zdWJkb21haW5zLnBpbm5pbmcuZXhhbXBsZS5jb20wDQYJKoZIhvcNAQELBQADggEBAAmzXfeoOS59FkNABRonFPRyFl7BoGpVJENUteFfTa2pdAhGYdo19Y4uILTTj+vtDAa5yryb5Uvd+YuJnExosbMMkzCrmZ9+VJCJdqUTb+idwk9/sgPl2gtGeRmefB0hXSUFHc/p1CDufSpYOmj9NCUZD2JEsybgJQNulkfAsVnS3lzDcxAwcO+RC/1uJDSiUtcBpWS4FW58liuDYE7PD67kLJHZPVUV2WCMuIl4VM2tKPtvShz1JkZ5UytOLs6jPfviNAk/ftXczaE2/RJgM2MnDX9nGzOxG6ONcVNCljL8avhFBCosutE6i5LYSZR6V14YY/xOn15WDSuWdnIsJCo=",
>+        "MIIC2jCCAcKgAwIBAgIBATANBgkqhkiG9w0BAQsFADAmMSQwIgYDVQQDExtBbHRlcm5hdGUgVHJ1c3RlZCBBdXRob3JpdHkwHhcNMTQwOTI1MjEyMTU0WhcNMjQwOTI1MjEyMTU0WjAmMSQwIgYDVQQDExtBbHRlcm5hdGUgVHJ1c3RlZCBBdXRob3JpdHkwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDBT+BwAhO52IWgSIdZZifU9LHOs3IR/+8DCC0WP5d/OuyKlZ6Rqd0tsd3i7durhQyjHSbLf2lJStcnFjcVEbEnNI76RuvlN8xLLn5eV+2Ayr4cZYKztudwRmw+DV/iYAiMSy0hs7m3ssfX7qpoi1aNRjUanwU0VTCPQhF1bEKAC2du+C5Z8e92zN5t87w7bYr7lt+m8197XliXEu+0s9RgnGwGaZ296BIRz6NOoJYTa43n06LU1I1+Z4d6lPdzUFrSR0GBaMhUSurUBtOin3yWiMhg1VHX/KwqGc4als5GyCVXy8HGrA/0zQPOhetxrlhEVAdK/xBt7CZvByj1Rcc7AgMBAAGjEzARMA8GA1UdEwQIMAYBAf8CAQAwDQYJKoZIhvcNAQELBQADggEBAJq/hogSRqzPWTwX4wTn/DVSNdWwFLv53qep9YrSMJ8ZsfbfK9Es4VP4dBLRQAVMJ0Z5mW1I6d/n0KayTanuUBvemYdxPi/qQNSs8UJcllqdhqWzmzAg6a0LxrMnEeKzPBPD6q8PwQ7tYP+B4sBN9tnnsnyPgti9ZiNZn5FwXZliHXseQ7FE9/SqHlLw5LXW3YtKjuti6RmuV6fq3j+D4oeC5vb1mKgIyoTqGN6ze57v8RHi+pQ8Q+kmoUn/L3Z2YmFe4SKN/4WoyXr8TdejpThGOCGCAd3565s5gOx5QfSQX11P8NZKO8hcN0tme3VzmGpHK0Z/6MTmdpNaTwQ6odk="
>+      ],
>+    "userAgent":"Mozilla/5.0 (X11; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0"
>+  }
>+
>+Where the data represents the following:
>+
>+"timestamp"
>+  The (local) time at which the report was generated

In what format? UTC seconds since 1970?

>+"errorCode"
>+  The error code

The error code from what? Is there an enumeration which lists the possible codes?

>+"errorMessage"
>+  The error displayed to the user

Why is this important? I recommend removing this and hard-coding the domain name.

>+"failedCertChain"
>+  The certificate chain which caused the pinning violation

base64-encoded, presumably. I don't think you need to include full cert chains in the example data.

>+"user agent"
>+  The user agent string of the browser sending the report

I'm not sure you need the UA, but it wouldn't hurt. You should definitely also include the following data points:

* the product sending the report (currently always Firefox, but we have webapprt and FxAndroid and FxOS which might want to use the same service in the future).
* The channel
* The buildid
* A versioning key, so this will be version 1 and we can bump it in the future if necessary.


>+++ b/browser/base/content/docs/sslerrorreport/preferences.rst

>+"security.ssl.errorReporting.automatic"
>+  Should error reports be sent without user interaction. Default value:
>+  ``false``. Note: this pref is overridden by the value of
>+  ``security.ssl.errorReporting.enabled``

In what cases would automatic reporting be true?

IIRC in the original request we also wanted to record the IP address of the target server, in order to separate out things like DNS poisoning, captive portals, etc. Is that no longer useful information?

Please also record the count of user reports in telemetry? will would help us measure the effectiveness of this UI and cross-reference the number of incoming pin reports on the server versus the number reported by telemetry.

Comment 55

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8504383 [details] [diff] [review]
03_846489_tests.patch

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

::: browser/base/content/test/general/browser.ini
@@ +149,5 @@
>  [browser_bug419612.js]
>  skip-if = e10s # Bug 691614 - no e10s zoom support yet
>  [browser_bug422590.js]
> +[browser_bug846489.js]
> +skip-if = e10s # about:netError is broken with e10s

https://bugzilla.mozilla.org/show_bug.cgi?id=1069317 is fixed.

Comment 56

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #54)
> >+"errorCode"
> >+  The error code
> 
> The error code from what? Is there an enumeration which lists the possible
> codes?

This is the error code from certificate verification. Here's a small list of the most commonly-encountered errors:

https://wiki.mozilla.org/SecurityEngineering/x509Certs#Error_Codes_in_Firefox

In theory many of the errors from sslerr.h, secerr.h, and pkixnss.h could be encountered. We're starting with just MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE, which means that key pinning failed (i.e. there wasn't an intersection between the keys in any computed trusted certificate chain and the expected list of keys for the domain the user is attempting to connect to).

Comment 57

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8504383 [details] [diff] [review]
03_846489_tests.patch

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

::: browser/base/content/test/general/browser_bug846489.js
@@ +21,5 @@
> +}
> +
> +function testNetErrorUIWhenEnabled() {
> +  Services.prefs.setBoolPref("security.ssl.errorReporting.enabled", true);
> +  Services.prefs.setCharPref("security.ssl.errorReporting.url", "https://example.com/browser/browser/base/content/test/general/pinning_reports.sjs?succeed");

A better idea is to set this pref through prefs_general to make sure no tests will end up hitting the network if they test this service but forget to set this pref.
E.g. http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#236

Comment 58

[Mark Goodwin [:mgoodwin]]

Attachment: 846489_neterror_ui.patch

Modified to work with e10s:
1) moved actual report send into browser.js so that the failed cert chain can be seen on report generation (the content process stub does not allow access to this in e10s).
2) moved pref setting to browser.js (content process apparently cannot set prefs)
3a) browser.js adds message listeners to receive Browser:SendSSLErrorReport and Browser:SetSSLErrorReportAuto messages sent by content.js
3b) content.js adds listener to receive Browser:SSLErrorReportStatus from browser.js

Comment 59

[Mark Goodwin [:mgoodwin]]

Attachment: 846489_neterror_ui.patch

I'm hoping we're pretty much done here.

Changes:
1) Removal of user error message from the SSL Error report data
2) Updated in-tree docs to explain the error code
3) Moved sendErrorReport functionality into browser.js to make things work with e10s

Comment 60

[Mark Goodwin [:mgoodwin]]

Attachment: 03_846489_tests.patch

Tests completely rewritten thanks to e10s awkwardness. In particular:

1) moved from DOM mutation to message listening to determine if and when things are happening
2) added a pref in prefs_general to ensure error reporting does not cause (unrelated) tests to exit on error report

Comment 61

[Mark Goodwin [:mgoodwin]]

Attachment: 846489_neterror_ui.patch

Oops, forgot to add items to the report:
1) Added version, build, product and channel to the report

Comment 62

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8509657 [details] [diff] [review]
846489_neterror_ui.patch

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

::: browser/base/content/browser.js
@@ +4,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  let Ci = Components.interfaces;
>  let Cu = Components.utils;
> +let Cc = Components.classes;

strange that this isn't defined but is used everywhere in the code

@@ +2441,5 @@
> +      case "Browser:SendSSLErrorReport":
> +        this.onSSLErrorReport(msg.target, msg.json.elementId,
> +                              msg.json.documentURI,
> +                              msg.json.location,
> +                              msg.json.securityInfo);

use msg.data instead of msg.json. It means the same thing but .json is deprecated

@@ +2527,5 @@
> +    }
> +
> +    let reportURL = Services.prefs.getCharPref("security.ssl.errorReporting.url");
> +
> +    var xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]

s/var/let

Comment 63

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8509586 [details] [diff] [review]
03_846489_tests.patch

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

::: testing/profiles/prefs_general.js
@@ +233,5 @@
>  // Enable logging of APZ test data (see bug 961289).
>  user_pref('apz.test.logging_enabled', true);
>  
> +// Make sure SSL Error reports don't hit the network
> +user_pref("security.ssl.errorReporting.url","https://example.com/browser/browser/base/content/test/general/pinning_reports.sjs?succeed");

nit: space after comma

Comment 64

[:Felipe Gomes (needinfo for replies!)]

I reviewed the code, and I'm assuming that discussions about hosting this service and the privacy considerations for this feature were discussed elsewhere, right?

Comment 65

[Mark Goodwin [:mgoodwin]]

(In reply to :Felipe Gomes from comment #64)
> I reviewed the code, and I'm assuming that discussions about hosting this
> service and the privacy considerations for this feature were discussed
> elsewhere, right?

bsmedberg is looking at the content of the reports (this discussion is continuing in this bug).

The privacy review work was happening in bug 846506; I'm still awaiting info on the state of this from Alina.

I'll get the issues above addressed and, hopefully, we'll have answers and agreement on the rest soon.

Comment 66

[Mark Goodwin [:mgoodwin]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #54)
> In what format? UTC seconds since 1970?

UTC Millis since 1970.

> >+"errorCode"
> >+  The error code

See comment #56.
 
> >+"errorMessage"
> >+  The error displayed to the user
> 
> Why is this important?

It isn't. I have removed this.

> I'm not sure you need the UA, but it wouldn't hurt. You should definitely
> also include the following data points:
> 
> * the product sending the report (currently always Firefox, but we have
> webapprt and FxAndroid and FxOS which might want to use the same service in
> the future).
> * The channel
> * The buildid
> * A versioning key, so this will be version 1 and we can bump it in the
> future if necessary.

Done.

> >+"security.ssl.errorReporting.automatic"
> >+  Should error reports be sent without user interaction. Default value:
> >+  ``false``. Note: this pref is overridden by the value of
> >+  ``security.ssl.errorReporting.enabled``
> 
> In what cases would automatic reporting be true?

This is only set when specifically requested by the user. The user can set
this value (or unset it) by checking the "Automatically report errors in the
future" checkbox when about:neterror is displayed for SSL Errors.

I've added the above para to the in-tree docs - they'll land with me addressing the nits above.

> IIRC in the original request we also wanted to record the IP address of the
> target server, in order to separate out things like DNS poisoning, captive
> portals, etc. Is that no longer useful information?

We can add IP address at a later date (see bug 1087552) - most of these can be determined by examining the cert chain (subject name does not match hostname, chain is invalid, chain is otherwise valid but violates a PKP) - and those that can't can be faked by an attacker in any case in an active network attack anyway.

> Please also record the count of user reports in telemetry? will would help
> us measure the effectiveness of this UI and cross-reference the number of
> incoming pin reports on the server versus the number reported by telemetry.

jcj recently added telemetry for pin violations. Assuming the privacy review concludes positively, may we proceed?

Comment 67

[Mark Goodwin [:mgoodwin]]

A verdant try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c8a3dfda1b17

Comment 68

[Mark Goodwin [:mgoodwin]]

Attachment: 846489_neterror_ui.patch

Nits addressed:
1) replaced usage of msg.json with msg.data
2) replaced "var xhr = " with "let xhr ="

Carrying r+

Comment 69

[Mark Goodwin [:mgoodwin]]

Attachment: 03_846489_tests.patch

Nit addressed:
1) Missing space was added in user_pref args

Carrying r+

Comment 70

[Benjamin Smedberg]

> UTC Millis since 1970.

Unless there is a critical need for ms, I think 

> > Please also record the count of user reports in telemetry? will would help
> > us measure the effectiveness of this UI and cross-reference the number of
> > incoming pin reports on the server versus the number reported by telemetry.
> 
> jcj recently added telemetry for pin violations. Assuming the privacy review
> concludes positively, may we proceed?

This is the privacy review. We will definitely need to add a section to the Firefox privacy notice about this collection, but I don't anticipate that being a problem and I'll sync up with Geoff about that on Monday.

That doesn't quite answer the question, though. The question is measuring whether this UI is working correctly. You are currently measuring the total number of pin violations, but we should also measure the number of pin-violation *submissions*.

I'll still need to mark feedback+ on whatever patch has the final docs.

Comment 71

[Kathleen Wilson]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #70)
> We will definitely need to add a section to the
> Firefox privacy notice about this collection, but I don't anticipate that
> being a problem and I'll sync up with Geoff about that on Monday.
> 

Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=846506#c16
"After several meetings with Alina and Jishnu there was agreement on updates to the Firefox Privacy policy..."

The meetings we had led to the changes here:
https://www.mozilla.org/en-US/privacy/firefox/
In the "Security" section: " If you encounter an untrusted connection, you can also choose to send Mozilla the associated certificates."
But I thought the word "certificates" was supposed to link to 
https://support.mozilla.org/en-US/kb/secure-website-certificate
which lists the information that is included in a certificate.

Comment 72

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #70)
> > > Please also record the count of user reports in telemetry? will would help
> > > us measure the effectiveness of this UI and cross-reference the number of
> > > incoming pin reports on the server versus the number reported by telemetry.
> > 
> > jcj recently added telemetry for pin violations. Assuming the privacy review
> > concludes positively, may we proceed?
> 
> That doesn't quite answer the question, though. The question is measuring
> whether this UI is working correctly. You are currently measuring the total
> number of pin violations, but we should also measure the number of
> pin-violation *submissions*.

This is done by http://people.mozilla.org/~mchew/pinning_dashboard and documented in http://monica-at-mozilla.blogspot.com/2014/09/making-decisions-with-limited-data.html, and expanded on by Bug 1054498 as mgoodwin mentioned.

Comment 73

[Mark Goodwin [:mgoodwin]]

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #72)

> This is done by http://people.mozilla.org/~mchew/pinning_dashboard and
> documented in
> http://monica-at-mozilla.blogspot.com/2014/09/making-decisions-with-limited-
> data.html, and expanded on by Bug 1054498 as mgoodwin mentioned.

I think bsmedberg is asking for separate metrics on violation, attempted violation reports and actual violation reports.

Is that correct?

Comment 74

[Benjamin Smedberg]

Yes. In particular, the telemetry is not there to measure pin violations but to measure the opt-in rate of this new UI.

Comment 75

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #74)
> Yes. In particular, the telemetry is not there to measure pin violations but
> to measure the opt-in rate of this new UI.

Looks like mgoodwin filed https://bugzilla.mozilla.org/show_bug.cgi?id=1088141 for this.

Comment 76

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8510222 [details] [diff] [review]
846489_neterror_ui.patch

As per comment #75 I'd like to move telemetry to a separate bug. I agree we need the telemetry but I'd like to land it separately. Is there anything else that needs addressing?

Comment 77

[Benjamin Smedberg]

Comment on attachment 8510222 [details] [diff] [review]
846489_neterror_ui.patch

I still prefer seconds to ms, but I won't insist on it.

Comment 78

[Mark Goodwin [:mgoodwin]]

Attachment: 846489_neterror_ui.patch

Bitrot (and bsmedberg's millis / seconds issue) addressed.

Changes:
1) Date.now() replaced with Math.round(Date.now()/1000) in report timestamp
2) Documentation modified to reflect this

Comment 79

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8514316 [details] [diff] [review]
846489_neterror_ui.patch

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

::: browser/base/content/browser.js
@@ +2547,5 @@
> +
> +    let report = {
> +      hostname: location.hostname,
> +      port: location.port,
> +      timestamp: Math.round(Date.now()/1000),

nit: spacing around /

Comment 80

[Mark Goodwin [:mgoodwin]]

Attachment: 846489_neterror_ui.patch

Nit addressed

Comment 81

[Mark Goodwin [:mgoodwin]]

requesting checkin as per bug 846506

Comment 82

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1a2d9c2eeef2

Comment 83

[Mark Goodwin [:mgoodwin]]

Attachment: 01_bug846489_expose_error_code_on_transport_security_info.patch

Added bug#, description and reviewer to patch

Comment 84

[Mark Goodwin [:mgoodwin]]

Attachment: 02_bug846489_neterror_ui.patch

Added bug#, description and reviewer to patch

Comment 85

[Mark Goodwin [:mgoodwin]]

Attachment: 03_bug846489_tests.patch

Added bug#, description and reviewer to patch

Comment 86

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74b8493b5610
https://hg.mozilla.org/integration/mozilla-inbound/rev/de8069c6745e
https://hg.mozilla.org/integration/mozilla-inbound/rev/287ddaf9b9df

Comment 87

[Mark Goodwin [:mgoodwin]]

Attachment: 04_bug846489_fix_report_URL.patch

We should probably be pointing to prod. not stage.

Comment 88

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8515098 [details] [diff] [review]
04_bug846489_fix_report_URL.patch

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

Good catch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e23ba6e29b84

Comment 89

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74b8493b5610
https://hg.mozilla.org/mozilla-central/rev/de8069c6745e
https://hg.mozilla.org/mozilla-central/rev/287ddaf9b9df

Comment 90

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e23ba6e29b84