1241455 - Send TLS Error Reports for subresources

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mark Goodwin [:mgoodwin]]

The TLS error reporting functionality can currently only be invoked from error pages (abutCertError.xhtml and aboutNetError.xhtml). This means that we don't get any TLS error reports for subresource connections.

We're currently seeing high levels of validation failures that seem to fall into this category.

Comment 1

[Mark Goodwin [:mgoodwin]]

Attachment: MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

This patch makes use of the security reporter component (which hasn't landed yet
 - see blockers) to allow automatic TLS error reports to be sent directly from
nsHttpChannel.cpp rather than sending them from browser.js. This allows failed
connections for subresources to be reported.

Some of the report sending from browser.js was retained to allow reports to be
sent at the time a user enables reporting. This has been modified to also make
use of the component.

Since the patient is on the table, I've also taken the opportunity to
remove the retry and status bits from aboutCertError.xhtml and
aboutNetError.xhtml - which removes a bunch of code and simplifies things a bit.

The mochitests have been modified to cope with the fact that the UI does not
update with report sending / report failures - instead, success and failure are
determined by examining the response seen from the server from within the test.

Review commit: https://reviewboard.mozilla.org/r/32679/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32679/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8712855 [details]
MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

https://reviewboard.mozilla.org/r/32679/#review29559

::: browser/base/content/browser.js:2730
(Diff revision 1)
>      /*
>       * Requested info for the report:
>       * - 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)
>       *
>       * The request data should be added to the report by the receiving server.
>       */
>  
>      // Convert the nsIX509CertList into a format that can be parsed into
>      // JSON
>      let asciiCertChain = [];
>  
>      if (transportSecurityInfo.failedCertChain) {
>        let certs = transportSecurityInfo.failedCertChain.getEnumerator();
>        while (certs.hasMoreElements()) {
>          let cert = certs.getNext();
>          cert.QueryInterface(Ci.nsIX509Cert);
>          asciiCertChain.push(btoa(getDERString(cert)));
>        }
>      }
>  
>      let report = {
>        hostname: location.hostname,
>        port: location.port,
>        timestamp: Math.round(Date.now() / 1000),
>        errorCode: transportSecurityInfo.errorCode,
>        failedCertChain: asciiCertChain,
>        userAgent: window.navigator.userAgent,
>        version: 1,
>        build: gAppInfo.appBuildID,
>        product: gAppInfo.name,
>        channel: UpdateUtils.UpdateChannel
>      }
>  
>      let reportURL = Services.prefs.getCharPref("security.ssl.errorReporting.url");
>  

All of this is unused now, right? Please remove it. :-)

::: browser/base/content/content.js:253
(Diff revision 1)
>      let automatic = Services.prefs.getBoolPref("security.ssl.errorReporting.automatic");
>      content.dispatchEvent(new content.CustomEvent("AboutCertErrorOptions", {
>        detail: JSON.stringify({
>          enabled: Services.prefs.getBoolPref("security.ssl.errorReporting.enabled"),
>          automatic,
>        })
>      }));

Can you add a comment above this section of code that explains that nsHttpChannel itself takes care of sending the error report automatically if automatic sending is enabled, but that the UI sends it if automatic sending was disabled and is enabled by the user? Thanks!

Did we get a privacy review for these changes? Especially if we default to sending this information which is potentially sensitive (e.g. for corporate intranet domains chained to private roots).

::: browser/base/content/test/general/browser_ssl_error_reports.js:69
(Diff revision 1)
> -  yield promiseReport;
> +  //let status = yield promiseStatus;

Nit: please remove this bit of commented out code

::: browser/base/content/test/general/browser_ssl_error_reports.js:83
(Diff revision 1)
> +

Nit: don't add an empty line here.

::: browser/base/content/test/general/browser_ssl_error_reports.js:100
(Diff revision 1)
>    yield ContentTask.spawn(browser, null, function* () {
>      content.document.getElementById("automaticallyReportInFuture").click();
>    });
>  
>    // Wait for the error report submission.
> -  yield promiseReport;
> +  yield createReportResponseStatusPromise(URL_REPORTS + suffix);

This can race now - please create the promise before ContentTask.spawn, and yield for it afterwards.

::: browser/base/content/test/general/browser_ssl_error_reports.js:150
(Diff revision 1)
> -  return ContentTask.spawn(browser, null, function* () {
> -    let type = "Browser:SSLErrorReportStatus";
> -    let active = false;
> -
> +  if (status > 199 && status < 300) {
> +    return false;
> +  }
> +  return true;

Nit: return status < 200 || status >= 300;

::: browser/base/content/test/general/browser_ssl_error_reports.js:162
(Diff revision 1)
> -            if (active) {
> -              resolve(message.data.reportStatus);
> -            } else {
> -              reject("activity should be seen before success");
> -            }
> -            break;
> -          case "error":
> -            removeMessageListener(type, onReportStatus);
> -            reject("sending the report failed");
> -            break;
> +    var observer = {
> +      QueryInterface: function (aIID) {
> +        if (aIID.equals(Ci.nsISupports) ||
> +          aIID.equals(Ci.nsIObserver))
> +          return this;
> +        throw Cr.NS_ERROR_NO_INTERFACE;
> +      },
> +
> +      observe: function(subject, topic, data) {
> +        subject.QueryInterface(Ci.nsIHttpChannel);
> +        let requestURI = subject.URI.spec;
> +        if (requestURI === URI) {
> +          // we have a matching request
> +          resolve(subject.responseStatus);
> +          obs.removeObserver(this, "http-on-examine-response");
>          }
> +      }
> +    };

Nit: In JS you can just use a function instead of an object, and then you don't need the QI implementation. We also have "Services" for easy access to things like the observer service.

So this function can be reduced to:

```
function createReportResponseStatusPromise(expectedURI) {
  return new Promise(resolve => {
    let observer = (subject, topic, data) => {
      subject.QueryInterface(Ci.nsIHttpChannel);
      let requestURI = subject.URI.spec;
      if (requestURI == expectedURI) {
        Services.obs.removeObserver(observer, "http-on-examine-response");
        resolve(subject.responseStatus);
      }
    };
    Services.obs.addObserver(observer, "http-on-examine-response", false);
  });
}
```

::: netwerk/protocol/http/nsHttpChannel.cpp:5793
(Diff revision 1)
> +            errorReporter->ReportTLSError(secInfo, hostStr, port);

I have no idea about how this plays out, but have we at least considered the bandwidth impact of what happens if I load example.com which uses loads of content from cdn.example.net, which has an invalid cert? If there are 50 subresources, are we going to send 50 separate reports for cdn.example.net, milliseconds apart? Because that doesn't sound great. In a sense, doing this only for toplevel loads provides a limit to how much of this reporting we're doing, and we're taking that limit away now. :-)

Comment 3

[:Gijs (he/him)]

Comment on attachment 8712855 [details]
MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

Gah, forgot to check ship it - with everything in my earlier comments addressed, r=me on the browser/ bits! :-)

Comment 4

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8712855 [details]
MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32679/diff/1-2/

Comment 5

[Mark Goodwin [:mgoodwin]]

https://reviewboard.mozilla.org/r/32679/#review29559

> I have no idea about how this plays out, but have we at least considered the bandwidth impact of what happens if I load example.com which uses loads of content from cdn.example.net, which has an invalid cert? If there are 50 subresources, are we going to send 50 separate reports for cdn.example.net, milliseconds apart? Because that doesn't sound great. In a sense, doing this only for toplevel loads provides a limit to how much of this reporting we're doing, and we're taking that limit away now. :-)

We have considered this. I'm going to file a followup to do some rudimentary de-duplication.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8712855 [details]
MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

https://reviewboard.mozilla.org/r/32679/#review29645

LGTM!

I'm still curious about the privacy side of the automatic submission and if we'll disable that by default on ESR or do anything else to it. Also note that if we plan to uplift this, you need this patch without the string removals I'm suggesting below (can't change strings on aurora/beta, not even removing them).

::: browser/base/content/aboutNetError.xhtml
(Diff revision 2)
> -            <button id="reportCertificateErrorRetry">&errorReporting.tryAgain;</button>
> -            <span id="reportSendingMessage">&errorReporting.sending;</span>
> -            <span id="reportSentMessage">&errorReporting.sent;</span>

Sorry for missing this first time around: You should remove these now-unused strings from the relevant .dtd files (they're duplicated in netError.dtd and aboutCertError.dtd, it seems)

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8712855 [details]
MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

https://reviewboard.mozilla.org/r/32679/#review29673

just so you are sure - this will report the origin information for the failure.. and you'll get ALL kinds of failures, not just security ones. (tcp, dns, http parsing, cancels, etc..)

because we're at the channel layer here you won't get the IP of the server actually used which could certainly be interesting.. and necko actually covers up some tls "intolerance" errors by retrying behind the scenes with different preferences.. the channels don't see that code, so neither will this.

::: netwerk/protocol/http/nsHttpChannel.cpp:5784
(Diff revision 2)
> +        if (NS_SUCCEEDED(rv)) {

since you don't use rv (as a return value), often the one argument version of do_QueryInterface is used and the result is just nullchecked.. I don't mind if you want to do this tho

::: netwerk/protocol/http/nsHttpChannel.cpp:5788
(Diff revision 2)
> +            NS_ENSURE_SUCCESS(rv, rv);

this would return from onstop() on fail.. you can't do that without calling the listener onstop().. I think you just want to skip the error-reporter bits

::: netwerk/protocol/http/nsHttpChannel.cpp:5790
(Diff revision 2)
> +            NS_ENSURE_SUCCESS(rv, rv);

same

Comment 8

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8712855 [details]
MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32679/diff/2-3/

Comment 9

[Mark Goodwin [:mgoodwin]]

https://reviewboard.mozilla.org/r/32679/#review29673

That wasn't what I intended. I've added a check for this now.

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8712855 [details]
MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

https://reviewboard.mozilla.org/r/32679/#review29703

::: netwerk/protocol/http/nsHttpChannel.cpp:5771
(Diff revisions 2 - 3)
> +    uint32_t errorClass;

all this code should probably go in its own function and just called if NS_FAILED(status).. because hopefully that's not the common case :)

ProcessSecurityReport() or something

Comment 11

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8712855 [details]
MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32679/diff/3-4/

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8712855 [details]
MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

https://reviewboard.mozilla.org/r/32679/#review29787

Comment 13

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41ffd1417da1

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8712855 [details]
MozReview Request: Bug 1241455 Send TLS Error Reports for subresources r?past, Gijs, mcmanus

https://reviewboard.mozilla.org/r/32679/#review29811

Comment 15

[Mark Goodwin [:mgoodwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c79d6954e4b1dc2779744cdec700eb0f209342cf
Bug 1241455 Send TLS Error Reports for subresources r=past, Gijs, mcmanus

Comment 16

[Carsten Book [:Tomcat]]

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