Closed Bug 1468222 Opened 2 years ago Closed 2 years ago

Consolidate nsISSLStatus into nsITransportSecurityInfo

Categories

(Core :: Security: PSM, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: jcj, Assigned: dipen)

References

Details

(Whiteboard: [psm-assigned] [psm-contractor])

Attachments

(1 file, 1 obsolete file)

These two IDLs are both used to communicate transport security status. However, some of the information is redundant and in any case there’s no reason to spread it across two interfaces and implementations. nsISSLStatus should be rolled into nsITransportSecurityInfo and the implementations (nsSSLStatus and TransportSecurityInfo) should be merged.

All tests should be updated as necessary to continue functioning.
Status: NEW → ASSIGNED
Comment on attachment 8989666 [details]
Bug 1468222 -  Initial updates to consolidate nsISSLStatus into nsITransportSecurityInfo.

https://reviewboard.mozilla.org/r/254674/#review261532


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/security.py:45
(Diff revision 1)
>  
>          :returns: Certificate details as JSON object.
>          """
>          cert = self.marionette.execute_script("""
>            var securityUI = arguments[0].linkedBrowser.securityUI;
> -          var status = securityUI.QueryInterface(Components.interfaces.nsISSLStatusProvider)
> +          var status = securityUI.QueryInterface(Components.interfaces.nsITransportSecurityInfoProvider).secInfo;

Error: Line too long (113 > 99 characters) [flake8: E501]
Comment on attachment 8989666 [details]
Bug 1468222 -  Initial updates to consolidate nsISSLStatus into nsITransportSecurityInfo.

https://reviewboard.mozilla.org/r/254674/#review262280

Awesome - I think this is generally how we want things to look after doing this. This patch is quite large, though, so we probably should break up the work a bit if we can. One thing that might make this a bit more manageable would be to first remove `nsISSLStatusProvider` entirely (in a separate bug). Only 3 or so classes (spread across C++ and JS) implement it, so we should just be able to modify them to have a property `nsISSLStatus sslStatus` that can be accessed directly (rather than first QI-ing to `nsISSLStatusProvider`).
Also, when you're ready for a more final review, changes in non-PSM components will need review from an owner/peer of those modules: https://wiki.mozilla.org/Modules/All

::: commit-message-987ea:2
(Diff revision 1)
> +Bug 1468222 -  Initial updates to consolidate nsISSLStatus into nsITransportSecurityInfo. r?keeler,jcj
> +

Lately we've been a bit more verbose in commit messages, so maybe have a sentence or two about the motivation or strategy here.

::: browser/base/content/browser.js:3098
(Diff revision 1)
>          if (isTopFrame) {
>            secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_CLICK_ADD_EXCEPTION);
>          }
>  
>          securityInfo = getSecurityInfo(securityInfoAsString);
> -        sslStatus = securityInfo.QueryInterface(Ci.nsISSLStatusProvider)
> +        sslStatus = securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);

It looks like `getSecurityInfo` returns something that's already been QI'd to `nsITransportSecurityInfo`, so this extra QI shouldn't be necessary.

::: browser/base/content/browser.js:3135
(Diff revision 1)
>          if (isTopFrame) {
>            secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_UNDERSTAND_RISKS);
>          }
>  
>          securityInfo = getSecurityInfo(securityInfoAsString);
> -        sslStatus = securityInfo.QueryInterface(Ci.nsISSLStatusProvider)
> +        sslStatus = securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);

Same here.

::: browser/base/content/pageinfo/security.js:30
(Diff revision 1)
>      var cert = security._cert;
>      viewCertHelper(window, cert);
>    },
>  
>    _getSecurityInfo() {
> -    const nsISSLStatusProvider = Ci.nsISSLStatusProvider;
> +    const nsITransportSecurityInfoProvider = Ci.nsITransportSecurityInfoProvider;

Personally I don't see the point in having these consts, but browser isn't a module I'm a peer on, so my stylistic preference doesn't carry too much weight here.

::: devtools/shared/webconsole/network-helper.js:550
(Diff revision 1)
>     *                    * "secure": the connection was properly secured.
>     *          If state == broken:
>     *            - errorMessage: error code string.
>     *          If state == secure:
>     *            - protocolVersion: one of TLSv1, TLSv1.1, TLSv1.2, TLSv1.3.
> -   *            - cipherSuite: the cipher suite used in this connection.
> +   *            - cipherName: the cipher suite used in this connection.

Why change the name here?

::: netwerk/socket/nsITransportSecurityInfo.idl:59
(Diff revision 1)
> +
> +    [must_use]
> +    readonly attribute boolean isDomainMismatch;
> +    [must_use]
> +    readonly attribute boolean isNotValidAtThisTime;
> +    /* Note: To distinguish between

Ehhhh this comment isn't relevant any longer.

::: security/manager/ssl/SSLServerCertVerification.cpp:619
(Diff revision 1)
>        nsCOMPtr<nsIBadCertListener2> bcl = do_GetInterface(cb);
>        if (bcl) {
>          nsIInterfaceRequestor* csi
>            = static_cast<nsIInterfaceRequestor*>(mInfoObject);
>          bool suppressMessage = false; // obsolete, ignored
> -        Unused << bcl->NotifyCertProblem(csi, mInfoObject->SSLStatus(),
> +        Unused << bcl->NotifyCertProblem(csi, mInfoObject,

For some of these lines, you might be able to re-break them (80 chars generally) so they don't end up looking odd. Not a big deal though.

::: security/manager/ssl/SSLServerCertVerification.cpp:830
(Diff revision 1)
>    // Get the existing cert. If there isn't one, then there is
>    // no cert change to worry about.
>    nsCOMPtr<nsIX509Cert> cert;
>  
> -  RefPtr<nsSSLStatus> status(infoObject->SSLStatus());
> -  if (!status) {
> +  // DIPEN: This needs scrutiny.  Used to look at SSLStatus being set.
> +  if (!infoObject->IsPreliminaryHandshakeDone()) {

It seems like this is correct, barring concurrency issues (e.g. `IsPreliminaryHandshakeDone` can be true before the server certificate is set, but only on separate threads). Honestly, I'm a bit concerned about how cavalier we are with the threading model here - we've got multiple threads involved (the socket transport thread, a thread pool for certificate verification, and the main thread), but locking/assertions that we're on the expected thread is minimal. Maybe a good first step would be to add some assertions to verify that everything is set up the way we think it is?

In any case, another option might be to gate on whether or not a server certificate is set here - in the original code, having an nsSSLStatus seems to be fairly tightly coupled with having a server certificate.

::: security/manager/ssl/TransportSecurityInfo.cpp:234
(Diff revision 1)
> -  // For successful connections and for connections with overridable errors,
> -  // mSSLStatus will be non-null. However, for connections with non-overridable
> -  // errors, it will be null.
> -  nsCOMPtr<nsISerializable> serializable(mSSLStatus);
> +  // BEGIN SSLStatus
> +
> +  // The current version of the binary stream format.
> +  const uint8_t STREAM_FORMAT_VERSION = 3;
> +
> +  // DIPEN:  Need to review making cert optional. Previously entire SSLStatus

I think we need to handle cases where the TLS handshake failed and we never got the auth certificate callback, so there wouldn't be a certificate.

::: security/manager/ssl/TransportSecurityInfo.cpp:235
(Diff revision 1)
> -  // mSSLStatus will be non-null. However, for connections with non-overridable
> -  // errors, it will be null.
> -  nsCOMPtr<nsISerializable> serializable(mSSLStatus);
> +
> +  // The current version of the binary stream format.
> +  const uint8_t STREAM_FORMAT_VERSION = 3;
> +
> +  // DIPEN:  Need to review making cert optional. Previously entire SSLStatus
> +  // was optional. Also is streamFormatVersion needed?

Unfortunately, it seems we do need to version the format for now - see bug 1248628 for some background information. It may be that the situation has changed since then, but in any case we need to be aware that changes we make here may affect service workers in a way that isn't handled well (to be clear, my position is that it should be up to the service workers implementation to figure out the right thing to do when cached information can't be deserialized, but I wasn't able to convince those working on that implementation to do that).

::: security/manager/ssl/TransportSecurityInfo.cpp:529
(Diff revision 1)
>      return;
>  
>    if (certVerificationResult != SECSuccess) {
> -    MOZ_ASSERT(status, "Must have nsSSLStatus object when remembering flags");
>  
> -    if (!status)
> +    if (!infoObject->mHaveCertErrorBits)

Feel free to fix up style issues while you're here - braces around conditional bodies in this case.

::: security/manager/ssl/TransportSecurityInfo.cpp:540
(Diff revision 1)
> -    bits.mIsUntrusted = status->mIsUntrusted;
> +    bits.mIsUntrusted = infoObject->mIsUntrusted;
>  
>      MutexAutoLock lock(mMutex);
>      mErrorHosts.Put(hostPortKey, bits);
>    }
>    else {

Another style issue - this should just be `} else {` all on one line.

::: security/manager/ssl/nsIBadCertListener2.idl:9
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
>  
> -interface nsISSLStatus;
> +interface nsITransportSecurityInfo;

nit: let's sort these alphabetically

::: security/manager/ssl/tests/mochitest/browser/browser_bug627234_perwindowpb.js:6
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
>  var FakeSSLStatus = function() {

We might consider updating the names of these kinds of things to `FakeTransportSecurityInfo` or similar.

::: security/manager/ssl/tests/unit/head_psm.js:765
(Diff revision 1)
>  // add_cert_override except it may not be the case that the connection has an
>  // SSLStatus set on it. In this case, the error was not overridable anyway, so
>  // we consider it a success.
>  function attempt_adding_cert_override(aHost, aExpectedBits, aSecurityInfo) {
> -  let sslstatus = aSecurityInfo.QueryInterface(Ci.nsISSLStatusProvider)
> -                               .SSLStatus;
> +  let sslstatus = aSecurityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
> +  // sslstatus used to come back null.  Now make sure serverCert is set

Instead of this comment, let's update the one above the function declaration (and s/SSLStatus/server certificate/)

::: security/manager/ssl/tests/unit/test_session_resumption.js:44
(Diff revision 1)
>    add_connection_test("expired.example.com", PRErrorCodeSuccess, null,
>      (transportSecurityInfo) => {
>        ok(transportSecurityInfo.securityState &
>           Ci.nsIWebProgressListener.STATE_CERT_USER_OVERRIDDEN,
>           "expired.example.com should have STATE_CERT_USER_OVERRIDDEN flag");
>        let sslStatus = transportSecurityInfo

Let's just s/sslStatus/transportSecurityInfo/ everywhere in this file?

::: security/manager/ssl/tests/unit/test_ssl_status.js:24
(Diff revision 1)
>    // Test successful connection (failedCertChain should be null,
>    // succeededCertChain should be set as expected)
>    add_connection_test(
>      "good.include-subdomains.pinning.example.com", PRErrorCodeSuccess, null,
> -    function withSecurityInfo(aSSLStatus) {
> -      let sslstatus = aSSLStatus.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus;
> +    function withSecurityInfo(aTransportSecurityInfo) {
> +      let sslstatus = aTransportSecurityInfo.QueryInterface(Ci.nsITransportSecurityInfo);

We should be able to just have `aTransportSecurityInfo.QueeryInterface(Ci.nsITransportSecurityInfo);` by itself and then refer to `aTransportSecurityInfo` everywhere (instead of `sslstatus`).

::: toolkit/components/extensions/webrequest/ChannelWrapper.cpp:884
(Diff revision 1)
>  {
>    if (nsCOMPtr<nsIChannel> chan = MaybeChannel()) {
>      nsCOMPtr<nsISupports> securityInfo;
>      Unused << chan->GetSecurityInfo(getter_AddRefs(securityInfo));
>      if (nsCOMPtr<nsITransportSecurityInfo> tsi = do_QueryInterface(securityInfo)) {
> -      auto errorCode = tsi->GetErrorCode();
> +      int32_t errorCode=0;

nit: spaces around `=`

::: toolkit/mozapps/update/nsUpdateService.js:3117
(Diff revision 1)
>      var status = this._getChannelStatus(request);
>      LOG("Checker:onError - request.status: " + status);
>  
>      // Set MitM pref.
>      try {
>        var sslStatus = request.channel.QueryInterface(Ci.nsIRequest)

The QI to `nsIRequest` shouldn't be necessary (or even helpful) here...
Attachment #8989666 - Flags: review?(dkeeler) → review-
Move all fields of nsISSLStatus to nsITransportSecurityProvider
Remove nsISSLStatus interface and definition
Update all code and test references to nsISSLStatus
Maintain ability to read in older version of serialized nsISSLStatus.  This
is verified with psm_DeserializeCert gtest.
Comment on attachment 9002293 [details]
Bug 1468222 Consolidate nsISSLStatus info nsITransportSecurityInfo r=keeler,gijs,baku,sebastian,mcmanus,ato,dustin

Andreas Tolfsen ﹝:ato﹞ has approved the revision.
Attachment #9002293 - Flags: review+
Comment on attachment 9002293 [details]
Bug 1468222 Consolidate nsISSLStatus info nsITransportSecurityInfo r=keeler,gijs,baku,sebastian,mcmanus,ato,dustin

:Gijs (he/him) has approved the revision.
Attachment #9002293 - Flags: review+
Comment on attachment 9002293 [details]
Bug 1468222 Consolidate nsISSLStatus info nsITransportSecurityInfo r=keeler,gijs,baku,sebastian,mcmanus,ato,dustin

Patrick McManus [:mcmanus] has approved the revision.
Attachment #9002293 - Flags: review+
Comment on attachment 9002293 [details]
Bug 1468222 Consolidate nsISSLStatus info nsITransportSecurityInfo r=keeler,gijs,baku,sebastian,mcmanus,ato,dustin

Simon Fraser [:sfraser] ⌚️GMT has approved the revision.
Attachment #9002293 - Flags: review+
Comment on attachment 9002293 [details]
Bug 1468222 Consolidate nsISSLStatus info nsITransportSecurityInfo r=keeler,gijs,baku,sebastian,mcmanus,ato,dustin

[:keeler] (use needinfo) has approved the revision.
Attachment #9002293 - Flags: review+
Comment on attachment 9002293 [details]
Bug 1468222 Consolidate nsISSLStatus info nsITransportSecurityInfo r=keeler,gijs,baku,sebastian,mcmanus,ato,dustin

Andrea Marchesini [:baku] has approved the revision.
Attachment #9002293 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 9002293 [details]
Bug 1468222 Consolidate nsISSLStatus info nsITransportSecurityInfo r=keeler,gijs,baku,sebastian,mcmanus,ato,dustin

James Willcox (:snorp) (jwillcox@mozilla.com) has approved the revision.
Attachment #9002293 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd8baf88f373
Consolidate nsISSLStatus info nsITransportSecurityInfo r=snorp,ato,sfraser,keeler,baku,mcmanus,Gijs
Keywords: checkin-needed
Backed out for test_security-info-parser.js failures

backout:  https://hg.mozilla.org/integration/autoland/rev/ef90f6788e2ca3e984fb52be09974c1853f4e58f

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bd8baf88f3739a6b2211faaafea974c84a87d0b2

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198174147&repo=autoland&lineNumber=2083

[task 2018-09-07T23:45:09.442Z] 23:45:09     INFO -  TEST-START | devtools/shared/webconsole/test/unit/test_security-info-parser.js
[task 2018-09-07T23:45:09.883Z] 23:45:09  WARNING -  TEST-UNEXPECTED-FAIL | devtools/shared/webconsole/test/unit/test_security-info-parser.js | xpcshell return code: 0
[task 2018-09-07T23:45:09.884Z] 23:45:09     INFO -  TEST-INFO took 439ms
[task 2018-09-07T23:45:09.885Z] 23:45:09     INFO -  >>>>>>>
[task 2018-09-07T23:45:09.886Z] 23:45:09     INFO -  PID 9967 | JavaScript strict warning: resource://devtools/shared/Loader.jsm, line 222: ReferenceError: reference to undefined property "name"
[task 2018-09-07T23:45:09.886Z] 23:45:09     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-09-07T23:45:09.887Z] 23:45:09     INFO -  PID 9967 | NetworkHelper.parseSecurityInfo threw an exception: Could not get HSTS/HPKP status as hostname is not available.
[task 2018-09-07T23:45:09.888Z] 23:45:09     INFO -  PID 9967 | console.error: "NetworkHelper.parseSecurityInfo threw an exception: Could not get HSTS/HPKP status as hostname is not available."
[task 2018-09-07T23:45:09.888Z] 23:45:09     INFO -  TEST-PASS | devtools/shared/webconsole/test/unit/test_security-info-parser.js | run_test - [run_test : 48] State is correct. - "secure" == "secure"
[task 2018-09-07T23:45:09.889Z] 23:45:09  WARNING -  TEST-UNEXPECTED-FAIL | devtools/shared/webconsole/test/unit/test_security-info-parser.js | run_test - [run_test : 50] Cipher suite is correct. - "undefined" == "TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256"
[task 2018-09-07T23:45:09.890Z] 23:45:09     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/devtools/shared/webconsole/test/unit/test_security-info-parser.js:run_test:50
[task 2018-09-07T23:45:09.891Z] 23:45:09     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:527
[task 2018-09-07T23:45:09.891Z] 23:45:09     INFO -  -e:null:1
[task 2018-09-07T23:45:09.892Z] 23:45:09     INFO -  exiting test
[task 2018-09-07T23:45:09.893Z] 23:45:09     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "name"" {file: "resource://devtools/shared/Loader.jsm" line: 222}]"
[task 2018-09-07T23:45:09.893Z] 23:45:09     INFO -  <<<<<<<
[task 2018-09-07T23:45:09.897Z] 23:45:09     INFO -  INFO | Result summary:
[task 2018-09-07T23:45:09.900Z] 23:45:09     INFO -  INFO | Passed: 386
[task 2018-09-07T23:45:09.900Z] 23:45:09  WARNING -  INFO | Failed: 1
[task 2018-09-07T23:45:09.900Z] 23:45:09  WARNING -  One or more unittests failed.
[task 2018-09-07T23:45:09.900Z] 23:45:09     INFO -  INFO | Todo: 0
[task 2018-09-07T23:45:09.900Z] 23:45:09     INFO -  INFO | Retried: 1
[task 2018-09-07T23:45:09.900Z] 23:45:09     INFO -  SUITE-END | took 381s
[task 2018-09-07T23:45:09.902Z] 23:45:09     INFO -  Node moz-http2 server shutting down ...
[task 2018-09-07T23:45:09.954Z] 23:45:09    ERROR - Return code: 1
[task 2018-09-07T23:45:09.957Z] 23:45:09     INFO - TinderboxPrint: xpcshell-xpcshell<br/>386/<em class="testfail">1</em>/0
Flags: needinfo?(bugzilla)
Will correct and re-submit.

There is an error in the test that is causing the failure. The test has been passing until now because of two compounding errors.

The MockSecurityInfo definition (and the previous SSLStatus definition within it) defined the attribute cipherSuite.  However, the name of the attribute should have been cipherName.  This is what is defined in the IDL and expected in NetworkHelper.parseSecurityInfo() (devtools/shared/webconsole/network-helper.js).

The equality test in devtools/shared/webconsole/test/unit/test_security-info-parser.js allowed the test to pass because both parameters evaluated to null.
Comment on attachment 9002293 [details]
Bug 1468222 Consolidate nsISSLStatus info nsITransportSecurityInfo r=keeler,gijs,baku,sebastian,mcmanus,ato,dustin

J.C. Jones [:jcj] (he/him) has approved the revision.
Attachment #9002293 - Flags: review+
Pushed by jjones@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5cfda4227c6a
Consolidate nsISSLStatus info nsITransportSecurityInfo r=Gijs,snorp,jcj,mcmanus,sfraser,keeler,baku,ato
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5cfda4227c6a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla64
Depends on: CVE-2018-12385
Depends on: 1490653
You need to log in before you can comment on or make changes to this bug.