Certificate Transparency: Fetching of Signed Tree Heads (DRAFT)

components/certificate_transparency/log_proofs_fetcher.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/certificate_transparency/log_proofs_fetcher.h"
+
+#include "base/base64.h"
+#include "base/json/json_reader.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/strings/string_piece.h"
+#include "base/time/time.h"
+#include "base/values.h"
+#include "net/base/load_flags.h"
+#include "net/base/request_priority.h"
+#include "net/cert/ct_log_verifier.h"
+#include "net/cert/ct_serialization.h"
+#include "net/cert/signed_tree_head.h"
+#include "net/url_request/url_request_context.h"
+
+const int kMaxSTHSize = 600;

[Ryan Sleevi, 2015/04/24 10:42:07]

1) Document
2) SizeIn.... Bytes? Kb?

kMaxSTHSizeInBytes ?

[Eran Messeri, 2015/06/18 15:18:40]

Done.

+
+namespace certificate_transparency {
+
+LogProofsFetcher::LogProofsFetcher(net::URLRequestContext* request_context)
+    : request_context_(request_context) {
+}
+
+LogProofsFetcher::~LogProofsFetcher() {
+  STLDeleteContainerPairPointers(inflight_requests_.begin(),
+                                 inflight_requests_.end());
+}

[Ryan Sleevi, 2015/04/24 10:42:06]

SPECIAL NOTE: Because you have a URLRequestContext , and you're making requests,
you have to follow the rule that all of this requests are cancelled before the
URLRequest is destroyed.

[I haven't checked that, not sure how best to ensure it, but just an early
design note we can go over]

[Eran Messeri, 2015/06/18 15:18:41]

Done - cancelled all requests.

+
+scoped_ptr<net::URLRequest> LogProofsFetcher::CreateURLRequest(
+    GURL fetch_sth_url) {
+  scoped_ptr<net::URLRequest> request = request_context_->CreateRequest(
+      fetch_sth_url, net::DEFAULT_PRIORITY, this);
+  request->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                        net::LOAD_DO_NOT_SAVE_COOKIES);
+  return request.Pass();
+}
+
+void LogProofsFetcher::FetchSTH(net::CTLogVerifier* verifier) {
+  std::string fetch_sth_url = verifier->url() + "/get-sth";
+  GURL fetch_url(fetch_sth_url);

[Ryan Sleevi, 2015/04/24 10:42:06]

SOMETHING: I suspect we have something for appending paths to GURLs here w/o
having to do the to-ascii-and-back conversion in line 44. I'll have to check.

[Eran Messeri, 2015/06/18 15:18:41]

Found it - GURL has a Resolve method that does that. Note changes to
ct_known_logs_static to preperd '/' as it needed for Resolve() to correctly
append to the path rather than replace.

+  scoped_ptr<net::URLRequest> request = CreateURLRequest(fetch_url);
+  request->set_method("GET");
+
+  net::URLRequest* raw_request = request.get();
+  FetchParams* params = new FetchParams(verifier);
+  inflight_requests_.insert(std::make_pair(request.release(), params));
+  raw_request->Start();
+}
+
+void LogProofsFetcher::OnResponseStarted(net::URLRequest* request) {
+  const net::URLRequestStatus& status(request->status());
+  VLOG(0) << "Response started for fetch-sth from " << request->original_url();

[Ryan Sleevi, 2015/04/24 10:42:06]

VLOG(0) is wrong; VLOG(1) at best.

That said, this seems unnecessary - the net-internals log will have this state
transition.

[Eran Messeri, 2015/06/18 15:18:40]

I've changed all VLOGs in this file to VLOG(1), however I intend to remove them
all; they were added to aid debugging.
Please ignore all LOG/VLOG statements - I'll tidy them up in a follow-up
patchset.

+  if (!status.is_success()) {
+    LOG(WARNING) << "Fetching STH from " << request->original_url()
+                 << " failed. status:" << status.status()
+                 << " error:" << status.error();

[Ryan Sleevi, 2015/04/24 10:42:06]

ditto; net-internals will have this (since it'll have logged the request)

[Eran Messeri, 2015/06/18 15:18:41]

Acknowledged.

+    RequestCleanup(request);
+  } else if (request->GetResponseCode() != 200) {
+    LOG(WARNING) << "Fetch STH HTTP status: " << request->GetResponseCode();

[Ryan Sleevi, 2015/04/24 10:42:07]

ditto; net-internals will have this.

[Eran Messeri, 2015/06/18 15:18:41]

Acknowledged.

+    RequestCleanup(request);
+    return;
+  }
+
+  RequestsMap::iterator it(inflight_requests_.find(request));

[Ryan Sleevi, 2015/04/24 10:42:06]

auto it = inflight_requests_.find(request);

[Eran Messeri, 2015/06/18 15:18:40]

Done.

+  DCHECK(it != inflight_requests_.end());
+  scoped_refptr<net::IOBufferWithSize> buffer(it->second->sth_buffer);
+  VLOG(0) << "Kicking off read.";

[Ryan Sleevi, 2015/04/24 10:42:07]

all these vlogs are in net-internals; I'll stop whinging :)

[Eran Messeri, 2015/06/18 15:18:40]

Acknowledged.

+  request->Read(buffer.get(), buffer->size(), &(it->second->read_bytes));
+  if (request->status().is_io_pending()) {
+    VLOG(0) << "IO Pending: Will wait.";
+    return;
+  } else {
+    VLOG(0) << "Data available: Invoking OnReadComplete.";
+    OnReadCompleted(request, it->second->read_bytes);
+  }
+}
+
+void LogProofsFetcher::OnReadCompleted(net::URLRequest* request,
+                                       int bytes_read) {
+  VLOG(0) << "Read complete, got " << bytes_read << " bytes.";
+  RequestComplete(request, bytes_read);
+}
+
+void LogProofsFetcher::RequestComplete(net::URLRequest* request,
+                                       int bytes_read) {
+  RequestsMap::iterator it(inflight_requests_.find(request));

[Ryan Sleevi, 2015/04/24 10:42:06]

auto it = inflight_requests_.find(request)

[Eran Messeri, 2015/06/18 15:18:41]

Done.

+  DCHECK(it != inflight_requests_.end());
+  if (it->first != request) {

[Ryan Sleevi, 2015/04/24 10:42:06]

seems like a fatal error here, right?

if (it->first != request) {
  NOTREACHED();
  return;
}

[Eran Messeri, 2015/06/18 15:18:40]

Done.

+    LOG(WARNING) << "Different requests?!";
+  }
+
+  FetchParams* params(it->second);
+  VLOG(0) << "Got " << bytes_read << " bytes.";
+
+  // Extract STH json an parse it
+  base::StringPiece json_sth(params->sth_buffer->data(), bytes_read);
+  VLOG(0) << "STH data: " << json_sth;
+  scoped_ptr<net::ct::SignedTreeHead> sth;
+  if (CTLogResponseParser::FillSignedTreeHead(json_sth, sth.get())) {

[Ryan Sleevi, 2015/04/24 10:42:06]

DESIGN: Crap, I forgot the important thing to note - we don't process JSON in
the browser.

I know, I know, you're probably thinking, "WHAT!??", but if it's remotely
supplied JSON, from non-Google servers, then we don't process it in the browser.
We'd also prefer not to process Google-supplied JSON in the browser, but we tend
to be lazy in this respect, since we generally consider ourselves trustworthy
(since we are contributing the code...)

Let's chat about the design implications of this.

[Ryan Sleevi, 2015/04/24 10:42:06]

STYLE: We tend to put error control *before* the rest of processing, so that we
can clearly reason about the current state of execution at any given time.

if (!CTLogResponseParser::....) {
  LOG(ERROR) << "Invalid STH";  // Probably too chatty
  RequestCleanup(request);
  return;
}

....

[Ryan Sleevi, 2015/04/24 10:42:06]

SECURITY BUG: You forgot to allocate |sth|, but FillSignedTreeHead happily
starts scribbling into it.

I believe you meant to write

net::ct::SignedTreeHead sth;
if (CTLog....(json_sth, &sth)) {

}

[Eran Messeri, 2015/06/18 15:18:40]

Done.

[Eran Messeri, 2015/06/18 15:18:40]

Done.

[Eran Messeri, 2015/06/18 15:18:41]

Done.

+    VLOG(0) << "Parsed STH successfully.";
+    if (params->verifier->VerifySignedTreeHead(sth.get())) {

[Ryan Sleevi, 2015/04/24 10:42:06]

ditto here, whenever this "becomes something important".

(It's unclear whether/if/how we want to surface information about STH
verifications; it may be that we introduce some delegate about OnSTHVerified() /
OnSTHFailed() or something more nuanced, and then let the higher layers glue up
appropriate policy)

[Eran Messeri, 2015/06/18 15:18:41]

Ack,Done.

+      VLOG(0) << "Received valid STH.";
+    } else {
+      VLOG(0) << "Received invalid STH.";
+    }
+  } else {
+    LOG(ERROR) << "Invalid STH.";
+  }
+  RequestCleanup(request);
+}
+
+void LogProofsFetcher::RequestCleanup(net::URLRequest* request) {
+  VLOG(0) << "Cleaning up request to " << request->original_url();
+  RequestsMap::iterator it(inflight_requests_.find(request));

[Ryan Sleevi, 2015/04/24 10:42:07]

auto

[Eran Messeri, 2015/06/18 15:18:41]

Done.

+  DCHECK(it != inflight_requests_.end());
+
+  scoped_ptr<FetchParams> params(it->second);
+  // Delete the request, remove from map.
+  scoped_ptr<net::URLRequest> url_request(it->first);
+  // Both pointers will be deleted when exiting the method.
+  VLOG(0) << "Erasing iterator.";

[Ryan Sleevi, 2015/04/24 10:42:06]

too chatty (as with all the VLOGs), so hushing on that.

[Eran Messeri, 2015/06/18 15:18:40]

Acknowledged.

+  inflight_requests_.erase(it);
+}
+
+LogProofsFetcher::FetchParams::FetchParams(net::CTLogVerifier* verifier)
+    : verifier(verifier), sth_buffer(new net::IOBufferWithSize(kMaxSTHSize)) {
+}
+
+LogProofsFetcher::FetchParams::~FetchParams() {
+}
+
+bool CTLogResponseParser::FillSignedTreeHead(const base::StringPiece& json_sth,
+                                             net::ct::SignedTreeHead* sth) {
+  base::JSONReader json_reader;
+  base::Value* json = json_reader.Read(json_sth);
+  if (json == NULL) {

[Ryan Sleevi, 2015/04/24 10:42:06]

s/NULL/nullptr

[Eran Messeri, 2015/06/18 15:18:41]

Done.

+    LOG(WARNING) << "Empty JSON.";
+    return false;
+  }
+
+  const base::DictionaryValue* json_dict;
+  if (!json->GetAsDictionary(&json_dict)) {
+    LOG(WARNING) << "Json value not a dictionary.";
+    return false;
+  }
+
+  int tree_size;
+  if (!json_dict->GetInteger("tree_size", &tree_size)) {
+    LOG(WARNING) << "Json dictionary does not contain tree_size";
+    return false;
+  }
+
+  double timestamp;
+  if (!json_dict->GetDouble("timestamp", &timestamp)) {
+    LOG(WARNING) << "Json dictionary does not contain timestamp";
+    return false;
+  }
+
+  std::string sha256_root_hash;
+  if (!json_dict->GetString("sha256_root_hash", &sha256_root_hash)) {
+    LOG(WARNING) << "Json dictionary does not contain sha256_root_hash";
+    return false;
+  }
+
+  std::string tree_head_signature;
+  if (!json_dict->GetString("tree_head_signature", &tree_head_signature)) {
+    LOG(WARNING) << "Json dictionary does not contain tree_head_signature";
+    return false;
+  }
+
+  std::string decoded_root_hash;
+  if (!base::Base64Decode(sha256_root_hash, &decoded_root_hash)) {
+    LOG(WARNING) << "Failed decoding sha256_root_hash";
+    return false;
+  }
+
+  if (decoded_root_hash.length() != net::ct::kSthRootHashLength) {
+    LOG(WARNING) << "sha256_root_hash must be exactly 32 bit.";
+    return false;
+  }
+
+  std::string decoded_signature;
+  if (!base::Base64Decode(tree_head_signature, &decoded_signature)) {
+    LOG(WARNING) << "Failed decoding tree_head_signature";
+    return false;
+  }
+
+  base::StringPiece sp(decoded_signature);
+  if (!DecodeDigitallySigned(&sp, &(sth->signature))) {
+    LOG(WARNING) << "Failed decoding signature to DigitallySigned";
+    return false;
+  }
+
+  sth->version = net::ct::SignedTreeHead::V1;
+  sth->tree_size = tree_size;
+  sth->timestamp =
+      base::Time::UnixEpoch() + base::TimeDelta::FromMilliseconds(timestamp);
+  memcpy(sth->sha256_root_hash, decoded_root_hash.c_str(),
+         net::ct::kSthRootHashLength);
+  return true;
+}
+
+}  // namespace certificate_transparency