A default CTPolicyEnforcer is initialized without populating the CTLogVerifier

[sleevi]

Bug is in https://github.com/electron/brightray/blob/2e407b66777d29f03e92c0a625a02d633002643b/browser/url_request_context_getter.cc

A default CTPolicyEnforcer, beginning in Chrome 53, will return the default CT status of "not compliant" for any cert that doesn't match the CT policy.

However, no cert can match the CT policy without CTVerifier populating SCTs. This is done via MultiLogCTVerifier::AddLogs, with the result from net::ct::CreateLogVerifiersForKnownLogs(), which Brightray doesn't do in the URLRequestContextGetter.

[Note: We're working on cleaning up this API in Chromium, now that CT support has matured, such that a default-constructed MultiLogCTVerifier will do the right thing]

The result of this is that all certs fail to match the CT policy. However, some certs - whether for Expect-CT or because of CA misissuance (e.g. Symantec) - are expected to comply with CT policies.

Options:

1. If you don't care about CT (which you should care about, for security), supply a dummy CTPolicyEnforcer that says everything meets your policy.
2. If you want to disable per-CA security features (e.g. trust all Symantec certs), supply a TransportSecurityState::RequireCTDelegate that says CT isn't required for any host
3. If you want to support CT with Chromium's policy, use net::ct::CreateLogVerifiersForKnownLogs(). However, you should only do so with a reasonable update schedule in place; the set of known logs will grow and change over time, and an outdated Electron/Brightray may result in certificates accepted by Chrome being rejected.

Longer term, you can find yourself more insulated from these changes if you use net's URLRequestContextBuilder and file bugs upstream if you can't, since that's the Recommended Way and would have protected you from this

[deepak1556]

Thanks for the explanation! Option 3 sounds best, we would definitely want to support CT.

an outdated Electron/Brightray may result in certificates accepted by Chrome being rejected.

Brightray will get updated after every chrome stable release, this should make it consistent with upstream ?

Also moving to URLRequestContextBuilder sounds good, I don't think there is anything blocking us from doing it. Would like opinion from @zcbenz

/cc @electron/maintainers

[sleevi]

On Friday, September 16, 2016, Robo [email protected] wrote:

Brightray will get updated after every chrome stable release, this should
make it consistent with upstream ?

Yeah, I moreso meant as something for downstream consumers to be aware of,
and to make sure they're tracking Electron releases.

The effect of new CT logs coming online (1-2 every 2-4 releases) and
becoming widespread within a Chrome release or two means that if a consumer
gets further back than 3-5 releases, they may find sites stop working when
they replace certs. The odds are pretty low of that happening even then,
because the ecosystem takes a while to propagate changes, but it's just
something to be aware of.

[deepak1556]

Tried updating to use URLRequestContextBuilder but there are a few setters missing for brightray to make the move.

• Setter for ssl_config_service. Currently Electron overrides the SSLConfigService class to provide support for --ssl-version-fallback-min and --cipher-suite-blacklist flags.
• Setter for http_network_session or add it as a param to HttpCacheParams. We enable network throttling borrowing chromes' DevToolsNetworkTransactionFactory class and there is a need to set the cache session from it.
• Setter for url_request_job_factory. We allow creation of custom protocol schemes and handlers as a part of Electron at runtime and AtomURLRequestJobFactory class handles the mappings and job creation.

Edit: can these setters be considered for upstream or are there exisitng options i am overlooking ?

[bridiver]

Chrome provides a command line switch for a file/url. Electron could do something similar, but @sleevi it isn't clear to me what the TreeStateTracker is used for

  ct_tree_tracker_.reset(
      new certificate_transparency::TreeStateTracker(globals_->ct_logs));
  // Register the ct_tree_tracker_ as observer for new STHs.
  RegisterSTHObserver(ct_tree_tracker_.get());

  // Add logs from command line
  if (command_line.HasSwitch(switches::kCertificateTransparencyLog)) {
    std::string switch_value = command_line.GetSwitchValueASCII(
        switches::kCertificateTransparencyLog);
    for (const base::StringPiece& curr_log : base::SplitStringPiece(
             switch_value, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL)) {
      std::vector<std::string> log_metadata = base::SplitString(
          curr_log, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
      CHECK_GE(log_metadata.size(), 3u)
          << "CT log metadata missing: Switch format is "
          << "'description:base64_key:url_without_schema'.";
      std::string log_description(log_metadata[0]);
      std::string log_url(std::string("https://") + log_metadata[2]);
      std::string ct_public_key_data;
      CHECK(base::Base64Decode(log_metadata[1], &ct_public_key_data))
          << "Unable to decode CT public key.";
      scoped_refptr<const net::CTLogVerifier> external_log_verifier(
          net::CTLogVerifier::Create(ct_public_key_data, log_description,
                                     log_url));
      CHECK(external_log_verifier) << "Unable to parse CT public key.";
      VLOG(1) << "Adding log with description " << log_description;
      ct_logs.push_back(external_log_verifier);
    }
  }

[sleevi]

We're removing (and in trunk I think it already landed) the command-line flag. You shouldn't need to initialize a TreeStateTracker yet (that's more of an advanced CT thing we're working on in Chrome, and is only relevant if you're using Chrome's component updater)

[sleevi]

@deepak1556 Adding SSLConfig and URLRequestJobFactory are pretty reasonable, but HttpNetworkSession is probably something we wouldn't, since the whole point of the Builder is to ensure that the URLRequestContext is internally consistent with all the objects, and the object lifetimes are all safe. Taking the CacheParams may be reasonable. If you file an upstream bug with the use case, I'm sure we can find something that works :)