Certificate Transparency: Code for unpacking EV cert hashes whitelist

chrome/browser/net/packed_ct_ev_whitelist.cc

+// Copyright 2014 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 "chrome/browser/net/packed_ct_ev_whitelist.h"
+
+#include <string.h>
+
+#include <algorithm>
+
+#include "base/big_endian.h"
+#include "base/files/file_util.h"
+#include "base/lazy_instance.h"
+#include "base/logging.h"
+#include "chrome/browser/net/bit_stream_reader.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/ssl/ssl_config_service.h"
+
+namespace {
+const uint8_t kCertHashLengthBits = 64;  // 8 bytes
+const uint8_t kCertHashLength = kCertHashLengthBits / 8;
+const uint64_t kGolombMParameterBits = 47;  // 2^47
+
+void SetNewEVWhitelistInSSLConfigService(
+    scoped_refptr<net::ct::EVCertsWhitelist> new_whitelist) {

[Ryan Sleevi, 2014/10/21 22:50:25]

pass refcounted pointers as const-ref

[Eran Messeri, 2014/10/22 10:53:26]

Done.

+  net::SSLConfigService::SetEVCertsWhitelist(new_whitelist);
+}
+
+int hash_val_comp(const void* v1, const void* v2) {
+  return memcmp(v1, v2, kCertHashLength);
+}
+
+}  // namespace
+
+void SetEVWhitelistFromFile(const base::FilePath& compressed_whitelist_file) {
+  VLOG(1) << "Setting EV whitelist from file: "
+          << compressed_whitelist_file.value();
+  std::string compressed_list;
+  if (!base::ReadFileToString(compressed_whitelist_file, &compressed_list)) {
+    VLOG(1) << "Failed reading from " << compressed_whitelist_file.value();
+    return;
+  }
+
+  scoped_refptr<net::ct::EVCertsWhitelist> new_whitelist(
+      new PackedEVCertsWhitelist(compressed_list));
+  if (!new_whitelist->IsValid()) {
+    VLOG(1) << "Failed uncompressing EV certs whitelist.";
+    return;
+  }
+
+  base::Callback<void(void)> assign_cb =
+      base::Bind(SetNewEVWhitelistInSSLConfigService, new_whitelist);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE, assign_cb);
+}
+
+bool PackedEVCertsWhitelist::UncompressEVWhitelist(
+    const std::string& compressed_whitelist,
+    std::vector<std::string>* uncompressed_list) {
+  internal::BitStreamReader reader(base::StringPiece(
+      compressed_whitelist.data(), compressed_whitelist.size()));
+  std::vector<std::string> result;
+
+  VLOG(1) << "Uncompressing EV whitelist of size "
+          << compressed_whitelist.size();
+  uint64_t curr_hash(0);
+  if (!reader.ReadBits(kCertHashLengthBits, &curr_hash)) {
+    VLOG(1) << "Failed reading first hash.";
+    return false;
+  }
+  char hash_bytes[8];
+  base::WriteBigEndian(hash_bytes, curr_hash);
+  result.push_back(std::string(hash_bytes, 8));
+  static const uint64_t M = static_cast<uint64_t>(1) << kGolombMParameterBits;

[Ryan Sleevi, 2014/10/21 22:50:25]

needs a better name? Line 82 surprises me when reading it.

[Eran Messeri, 2014/10/22 10:53:26]

Can you suggest one? I've added a comment to clarify that it's the tunable
parameter used by the Golomb coding, but couldn't think of a better name.

[Ryan Sleevi, 2014/10/22 19:26:58]

even just something like kGolombParameterM or something to make it more explicit
than a single letter :)

[Eran Messeri, 2014/10/22 23:32:03]

Done, used your suggested name.

+
+  while (reader.BitsLeft() > kGolombMParameterBits) {
+    uint64_t read_prefix = 0;
+    if (!reader.ReadUnaryEncoding(&read_prefix)) {
+      VLOG(1) << "Failed reading unary-encoded prefix.";
+      return false;
+    }
+    if (read_prefix > (UINT64_MAX / M)) {
+      VLOG(1) << "Received value that would cause overflow: " << read_prefix;
+      return false;
+    }
+
+    uint64_t r = 0;
+    if (!reader.ReadBits(kGolombMParameterBits, &r)) {
+      VLOG(1) << "Failed reading " << kGolombMParameterBits << " bits.";
+      return false;
+    }
+    DCHECK_LT(r, M);
+
+    uint64_t curr_diff = read_prefix * M + r;
+    curr_hash += curr_diff;
+
+    base::WriteBigEndian(hash_bytes, curr_hash);
+    result.push_back(std::string(hash_bytes, 8));
+  }
+
+  uncompressed_list->swap(result);
+  return true;
+}
+
+PackedEVCertsWhitelist::PackedEVCertsWhitelist(
+    const std::string& compressed_whitelist)
+    : is_whitelist_valid_(false), whitelist_(NULL), whitelist_size_(0) {
+  std::vector<std::string> new_whitelist;
+  if (!UncompressEVWhitelist(compressed_whitelist, &new_whitelist))
+    return;
+
+  whitelist_ = new char[new_whitelist.size() * kCertHashLength];

[Ryan Sleevi, 2014/10/21 22:50:25]

Is there a reason that you're manually managing the memory like this?

Note that this is a code-smell for overflow issues - new_whitelist.size() *
kCertHashLength causing an overflow (if new_whitelist.size() > (max /
kCertHashLength), bad things)

Our normal pattern is to use something like std::vector<char[8]>, and then
allocate the vector to be new_whitelist.size(). vector will ensure that
new_whitelist.size() < (size / sizeof(elem)). 

[Eran Messeri, 2014/10/22 10:53:26]

Switched to using vector and std::binary_search.

It wasn't clear to me if your comment is about the manual memory management or
the usage of a raw type (char*).
I've used raw memory access here since bsearch, which was used later on,
requires a buffer. But binary_search can do the job equally well.

[Eran Messeri, 2014/10/22 12:42:14]

FWIW, using std::binary_search is twice is slow as using bsearch :/
std::map vs bsearch: std::map takes 140% more time than bseacrh.
std::map vs std::binary_search: std::map takes only 20% more than binary_search
over a vector.

I don't think that performance here matters that much, just FYI.

[Ryan Sleevi, 2014/10/22 19:26:58]

Well, both :) More aptly the raw memory management AND the lack of explicit
sizes.
I think this misses where I was originally going. You know the contents of the
vector are fixed (it's a vector of 8 byte hashes), so vector<char[8]> is a
totally legal definition. You'll get the same memory layout as char[8*size], but
with the added consistency checks for overflow as part of vector allocation.

Put differently, explicitly:

std::vector<char[8]> whitelist_;

whitelist_.resize(new_whitelist.size());
for (size_t i = 0; i < new_whitelist.size(); ++i) {
  memcpy(&whitelist_[i], new_whitelist[i].data(), kCertHashLength);
}
is_whitelist_valid_ = true;

Then you can still use bsearch, as &whitelist_[0] will be fully contiguous.

[Eran Messeri, 2014/10/22 23:32:03]

Done, although in a slightly different way:
std::vector<char[8]> whitelist_;
may be a legal definition with some compilers, but since arrays are not copyable
or assignable, they have to be stored in a struct and that struct used as the
template parameter for the vector.
So that's what I did (see struct TruncatedHash in the header file).

It's not pretty, but it works.

+  for (size_t i = 0; i < new_whitelist.size(); ++i) {
+    memcpy(&(whitelist_[i * kCertHashLength]),
+           new_whitelist[i].c_str(),
+           kCertHashLength);
+  }
+  whitelist_size_ = new_whitelist.size();
+  is_whitelist_valid_ = true;
+}
+
+PackedEVCertsWhitelist::~PackedEVCertsWhitelist() {
+  delete[] whitelist_;
+}
+
+bool PackedEVCertsWhitelist::ContainsCertificateHash(
+    const std::string& certificate_hash) const {
+  DCHECK(whitelist_);
+  return bsearch(certificate_hash.c_str(),
+                 whitelist_,
+                 whitelist_size_,
+                 kCertHashLength,
+                 hash_val_comp) != NULL;
+}
+
+bool PackedEVCertsWhitelist::IsValid() const {
+  return is_whitelist_valid_;
+}