Encrypt certificate logger requests for extended reporting

Encrypt certificate logger requests for extended reporting

This change encrypts requests by using ECDH to compute a shared key and
then encrypting with AES-CTR-128 and HMAC-SHA256.

The public key in this code is not the production public key.

BUG=461590

[estark, 2015-03-03 22:51:34]

estark@chromium.org changed reviewers:
+ agl@chromium.org

[estark, 2015-03-03 22:51:34]

agl@: This isn't ready for a full review yet, just a rough draft, but I wanted
to see if you could take a quick look at |EncryptSerializedReport| in
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc to see if the
general direction looks okay.

One specific question I have is if it's okay to get the AES and HMAC keys by
just taking the two halves of the SHA256 output.

[agl, 2015-03-05 19:22:26]

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/cert_...
File chrome/browser/net/cert_logger.proto (right):

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/cert_...
chrome/browser/net/cert_logger.proto:66: required bytes nonce = 4;
Since the key is random per-message, the nonce can be fixed and thus omitted
from the protobuf if you like.

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/cert_...
chrome/browser/net/cert_logger.proto:67: required bytes mac = 5;
this shouldn't be split out, it should be appended to the ciphertext. AEADs work
on opaque ciphertext blocks and don't generally expose their internals like
this.

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/cert_...
chrome/browser/net/cert_logger.proto:71: ECDH_AES_CTR_128_HMAC_SHA256 = 1;
the server side that I reviewed was using AES_256.

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/cert_...
chrome/browser/net/cert_logger.proto:74: optional Algorithm algorithm = 6
[default = UNKNOWN_ALGORITHM];
the default is UNKNOWN_ALGORITHM? Don't you want the default to be
ECDH_AES_CTR_128_HMAC_SHA256?

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:70: hmac_key =
std::string((char*)(symmetric_key + sizeof(symmetric_key) / 2),
An HMAC-SHA256 key is generally 32 bytes. Also, if you need to match the server
side and use AES-256 then you need 32 bytes there.

It's a real pain that SHA-512 isn't provided in crypto/ but you can hash
|shared_secret| twice, once with a zero byte prepended and once with a 1.

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:93: char
counter[kAesNonceSize];
s/char/uint8/

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:95:
crypto::RandBytes(counter, kAesNonceSize);
as noted, the nonce can be all zeros because the key is random per-message.

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:103: bool
init_result = hmac.Init(hmac_key);
why does |init_result| and |hmac_result| exist? You can just put the expression
in the if condition.

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:109: unsigned char
digest[hmac.DigestLength()];
Aren't variable-length arrays a compiler extension in C++? Maybe ask Jeffery,
but I think it would be better to fix this length to 32 and CHECK_EQ(32u,
hmac.DigestLength());

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right):

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.h:61: void
CalculateSymmetricKeys(const uint8* client_private_key,
this can be written:
  const uint8 client_private_key[32]

Which is the same thing, but documents the expected length in the header if you
like. (Same for several cases below.)

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.h:63: std::string&
aes_key,
These are outputs? If so, it should be a std::string*.

https://codereview.chromium.org/975623002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.h:68:
EncryptedCertLoggerRequest& encrypted_report);
If encrypted_report is an output, it should be a pointer.