CT: Adding SCT verification functionality: A CTLogVerifier instance can verify SCTs signed by a sin…

CT: Adding SCT verification functionality: A CTLogVerifier instance can verify SCTs signed by a single log.

BUG=309578
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234672

[Ryan Sleevi, 2013-11-08 00:30:42]

Mostly good, a few style nits here. I'll let Wan-Teh do the actual review.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h
File net/cert/ct_log_verifier.h (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:30: */
nit: Comment style: alternates between // and /* */ here. Would be good to be
consistent.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:39: const base::StringPiece& description = "");
style: Default arguments are forbidden -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss.cc
File net/cert/ct_log_verifier_nss.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:101: switch (public_key_->keyType) {
Use SECKEY_GetPublicKeyType()

[wtc, 2013-11-08 21:03:59]

Patch set 1 LGTM. I will review the _unittest.cc file after lunch.

You should be able to use crypto/signature_verifier.h to verify the signature.
The only hassle I can see is that you need to represent the signature algorithm
in DER-encoded format. If you find that inconvenient, we can consider changing
crypto/signature_verifier.h to not require that.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h
File net/cert/ct_log_verifier.h (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:13: #include "base/time/time.h"

This header doesn't seem necessary.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:27: /**

Chromium doesn't use this /** comment delimiter for automatic generation of
documentation. Just use //.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:49: bool VerifySCT(const ct::LogEntry& entry,

If this class only verifies SignedCertificateTimestamp, you can also just name
this method "Verify".

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:70: ct::DigitallySigned::SignatureAlgorithm
sig_algorithm_;

Nit: just wanted to make sure you intended to abbreviate "signature" to "sig"
because in general our Style Guide in unfriendly to abbreviations.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss.cc
File net/cert/ct_log_verifier_nss.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:23: SECOidTag
GetNssSigAlg(ct::DigitallySigned::SignatureAlgorithm alg) {

Nit: Nss => NSS

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:32: // FALLTHROUGH

This FALLTHROUGH comment and the FALLTHROUGH comment on line 54 are not
necessary because there is no code under this case.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:68: CTLogVerifier::CTLogVerifier() :
public_key_(NULL) {}

hash_algorithm_ and sig_algorithm_ should also be initialized.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:83: DVLOG(1) << "Failed decoding public key: "
<< public_key;

public_key contains binary data, so it is not suitable for direct use in error
messages. See also line 91.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:100: // supported.

Nit: copy this comment to the _openssl.cc file.

Nit: delete the blank line 98.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:115: // Extra sanity check: Require RSA
signatures of at least 2048 bits.

Nit: signatures => keys

Nit: copy this comment to the _openssl.cc file.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
File net/cert/ct_log_verifier_openssl.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
net/cert/ct_log_verifier_openssl.cc:32: case
ct::DigitallySigned::HASH_ALGO_NONE:

Nit: add a default case, unless you know it is impossible.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
net/cert/ct_log_verifier_openssl.cc:47: CTLogVerifier::CTLogVerifier() :
public_key_(NULL) {}

Should also initialize hash_algorithm_ and sig_algorithm_.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
net/cert/ct_log_verifier_openssl.cc:101: (1 == EVP_VerifyInit(&ctx, hash_alg) &&

I am not familiar with OpenSSL. Would EVP_DigestVerify* be a better interface
than EVP_Verify*? Chromium's crypto/signature_verifier_openssl.cc uses the
EVP_DigestVerify* interface.

https://codereview.chromium.org/55953002/diff/1/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/55953002/diff/1/net/net.gyp#newcode1261
net/net.gyp:1261: 'cert/cert_verify_proc_nss.h',

Should we exclude the source file 'cert/ct_log_verifier_nss.cc' here?

[wtc, 2013-11-08 21:51:14]

I have reviewed the _unittest.cc file.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_unit...
File net/cert/ct_log_verifier_unittest.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_unit...
net/cert/ct_log_verifier_unittest.cc:72: // ever attempting signature
validation.

Nit: is "ever" a typo of "even"?

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_unit...
net/cert/ct_log_verifier_unittest.cc:73: cert_sct.log_id =
std::string(cert_sct.log_id.size(), '\0');

Nit: you can use the assign method to avoid the constructor.

[Eran M. (Google), 2013-11-12 12:01:28]

Addressed all comments. I'm still checking if using a different OpenSSL api to
verify the signature is more suitable.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h
File net/cert/ct_log_verifier.h (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:13: #include "base/time/time.h"
On 2013/11/08 21:04:00, wtc wrote:
> 
> This header doesn't seem necessary.

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:27: /**
On 2013/11/08 21:04:00, wtc wrote:
> 
> Chromium doesn't use this /** comment delimiter for automatic generation of
> documentation. Just use //.

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:30: */
On 2013/11/08 00:30:43, Ryan Sleevi (OoO until 11-18) wrote:
> nit: Comment style: alternates between // and /* */ here. Would be good to be
> consistent.

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:39: const base::StringPiece& description = "");
On 2013/11/08 00:30:43, Ryan Sleevi (OoO until 11-18) wrote:
> style: Default arguments are forbidden -
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

Done - made required.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:49: bool VerifySCT(const ct::LogEntry& entry,
On 2013/11/08 21:04:00, wtc wrote:
> 
> If this class only verifies SignedCertificateTimestamp, you can also just name
> this method "Verify".

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:70: ct::DigitallySigned::SignatureAlgorithm
sig_algorithm_;
On 2013/11/08 21:04:00, wtc wrote:
> 
> Nit: just wanted to make sure you intended to abbreviate "signature" to "sig"
> because in general our Style Guide in unfriendly to abbreviations.

Done, replaced sig with signature.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss.cc
File net/cert/ct_log_verifier_nss.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:23: SECOidTag
GetNssSigAlg(ct::DigitallySigned::SignatureAlgorithm alg) {
On 2013/11/08 21:04:00, wtc wrote:
> 
> Nit: Nss => NSS

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:32: // FALLTHROUGH
On 2013/11/08 21:04:00, wtc wrote:
> 
> This FALLTHROUGH comment and the FALLTHROUGH comment on line 54 are not
> necessary because there is no code under this case.

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:68: CTLogVerifier::CTLogVerifier() :
public_key_(NULL) {}
On 2013/11/08 21:04:00, wtc wrote:
> 
> hash_algorithm_ and sig_algorithm_ should also be initialized.

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:83: DVLOG(1) << "Failed decoding public key: "
<< public_key;
On 2013/11/08 21:04:00, wtc wrote:
> 
> public_key contains binary data, so it is not suitable for direct use in error
> messages. See also line 91.

Done - removed.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:100: // supported.
On 2013/11/08 21:04:00, wtc wrote:
> 
> Nit: copy this comment to the _openssl.cc file.
> 
> Nit: delete the blank line 98.

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:101: switch (public_key_->keyType) {
On 2013/11/08 00:30:43, Ryan Sleevi (OoO until 11-18) wrote:
> Use SECKEY_GetPublicKeyType()

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_nss....
net/cert/ct_log_verifier_nss.cc:115: // Extra sanity check: Require RSA
signatures of at least 2048 bits.
On 2013/11/08 21:04:00, wtc wrote:
> 
> Nit: signatures => keys
> 
> Nit: copy this comment to the _openssl.cc file.

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
File net/cert/ct_log_verifier_openssl.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
net/cert/ct_log_verifier_openssl.cc:32: case
ct::DigitallySigned::HASH_ALGO_NONE:
On 2013/11/08 21:04:00, wtc wrote:
> 
> Nit: add a default case, unless you know it is impossible.

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
net/cert/ct_log_verifier_openssl.cc:47: CTLogVerifier::CTLogVerifier() :
public_key_(NULL) {}
On 2013/11/08 21:04:00, wtc wrote:
> 
> Should also initialize hash_algorithm_ and sig_algorithm_.

Done.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
net/cert/ct_log_verifier_openssl.cc:101: (1 == EVP_VerifyInit(&ctx, hash_alg) &&
On 2013/11/08 21:04:00, wtc wrote:
> 
> I am not familiar with OpenSSL. Would EVP_DigestVerify* be a better interface
> than EVP_Verify*? Chromium's crypto/signature_verifier_openssl.cc uses the
> EVP_DigestVerify* interface.

Converted the code to using EVP_DigestVerify although I can't judge if it's
better - it's the same number of calls and same arguments.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_unit...
File net/cert/ct_log_verifier_unittest.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_unit...
net/cert/ct_log_verifier_unittest.cc:72: // ever attempting signature
validation.
On 2013/11/08 21:51:14, wtc wrote:
> 
> Nit: is "ever" a typo of "even"?

It was just confusing - removed the word entirely.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_unit...
net/cert/ct_log_verifier_unittest.cc:73: cert_sct.log_id =
std::string(cert_sct.log_id.size(), '\0');
On 2013/11/08 21:51:14, wtc wrote:
> 
> Nit: you can use the assign method to avoid the constructor.

Done.

https://codereview.chromium.org/55953002/diff/1/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/55953002/diff/1/net/net.gyp#newcode1261
net/net.gyp:1261: 'cert/cert_verify_proc_nss.h',
On 2013/11/08 21:04:00, wtc wrote:
> 
> Should we exclude the source file 'cert/ct_log_verifier_nss.cc' here?

Probably so - added.

[commit-bot: I haz the power, 2013-11-12 17:45:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eranm@google.com/55953002/330001

[commit-bot: I haz the power, 2013-11-12 23:31:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eranm@google.com/55953002/330001

[commit-bot: I haz the power, 2013-11-13 00:18:14]

Change committed as 234672

[wtc, 2013-11-14 18:46:29]

Patch set 6 LGTM. Thanks!

Please write a new CL to fix the coding style nits.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
File net/cert/ct_log_verifier_openssl.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
net/cert/ct_log_verifier_openssl.cc:101: (1 == EVP_VerifyInit(&ctx, hash_alg) &&

On 2013/11/12 12:01:28, eranm wrote:
>
> Converted the code to using EVP_DigestVerify although I can't judge if it's
> better - it's the same number of calls and same arguments.

I was hoping you could ask Ben Laurie :-)

The only external difference I can tell is that EVP_DigestVerifyInit has two
more arguments, but we are not using them. I am curious if there is an internal
difference.

https://codereview.chromium.org/55953002/diff/1/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/test/ct_test_util.cc#newc...
net/test/ct_test_util.cc:96: const uint64_t kTestSCTTimestamp =
GG_UINT64_C(1365181456089);

Just curious: why did you decide to delete this constant? Is it because it is
only used once?

I am also curious what time this timestamp represents. Does that matter to the
unit test?

https://codereview.chromium.org/55953002/diff/330001/net/cert/ct_log_verifier...
File net/cert/ct_log_verifier_nss.cc (right):

https://codereview.chromium.org/55953002/diff/330001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier_nss.cc:69: public_key_(NULL) {}

Please see the Style Guide's recommendation on how to format a constructor
initializer list:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

https://codereview.chromium.org/55953002/diff/330001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier_nss.cc:112: SECKEY_GetPublicKeyType(public_key_);

Nit: this should be formatted as follows:

  DVLOG(1) << "Unsupported key type: "
           << SECKEY_GetPublicKeyType(public_key_);

You can find an example of this formatting style in the Extended Discussion at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Streams

and at

http://www.chromium.org/developers/coding-style#Code_Formatting

https://codereview.chromium.org/55953002/diff/330001/net/cert/ct_log_verifier...
File net/cert/ct_log_verifier_openssl.cc (right):

https://codereview.chromium.org/55953002/diff/330001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier_openssl.cc:51: public_key_(NULL) {}

Please see the Style Guide's recommendation on how to format a constructor
initializer list:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

https://codereview.chromium.org/55953002/diff/330001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier_openssl.cc:113: reinterpret_cast<unsigned
char*>(const_cast<char*>(signature.data())),

It is unfortunate that EVP_DigestVerifyFinal requires us to use a const_cast.
This seems to be a bug in the function prototype of EVP_DigestVerifyFinal.

[Eran M. (Google), 2013-11-16 22:59:16]

See replies below. I will send out another CL with formatting fixes shortly.

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
File net/cert/ct_log_verifier_openssl.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/cert/ct_log_verifier_open...
net/cert/ct_log_verifier_openssl.cc:101: (1 == EVP_VerifyInit(&ctx, hash_alg) &&
On 2013/11/14 18:46:30, wtc wrote:
> 
> On 2013/11/12 12:01:28, eranm wrote:
> >
> > Converted the code to using EVP_DigestVerify although I can't judge if it's
> > better - it's the same number of calls and same arguments.
> 
> I was hoping you could ask Ben Laurie :-)
> 
> The only external difference I can tell is that EVP_DigestVerifyInit has two
> more arguments, but we are not using them. I am curious if there is an
internal
> difference.

As mentioned in an offline email, I ended up asking the author of the API,
Steve. He explained the difference (it's not relevant here) and pointed out 
that EVP_DigestVerify is the newer API of the two. So I just migrated the code
to use the newer one.

https://codereview.chromium.org/55953002/diff/1/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/test/ct_test_util.cc#newc...
net/test/ct_test_util.cc:96: const uint64_t kTestSCTTimestamp =
GG_UINT64_C(1365181456089);
On 2013/11/14 18:46:30, wtc wrote:
> 
> Just curious: why did you decide to delete this constant? Is it because it is
> only used once?
> 
> I am also curious what time this timestamp represents. Does that matter to the
> unit test?

A rather lame reason: The Windows trybot did not like this variable declaration
(compile failed) and since I only use the value in one place, I inlined it.

It does matter - this value is used during the creation of a
SignedCertificateTimestamp instance that verifies over the test certificate
defined above.

https://codereview.chromium.org/55953002/diff/330001/net/cert/ct_log_verifier...
File net/cert/ct_log_verifier_openssl.cc (right):

https://codereview.chromium.org/55953002/diff/330001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier_openssl.cc:113: reinterpret_cast<unsigned
char*>(const_cast<char*>(signature.data())),
On 2013/11/14 18:46:30, wtc wrote:
> 
> It is unfortunate that EVP_DigestVerifyFinal requires us to use a const_cast.
> This seems to be a bug in the function prototype of EVP_DigestVerifyFinal.

Yes, I've noticed a few of the OpenSSL's functions not accepting a const pointer
where they should.

[wtc, 2013-11-19 21:58:09]

https://codereview.chromium.org/55953002/diff/1/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/55953002/diff/1/net/test/ct_test_util.cc#newc...
net/test/ct_test_util.cc:96: const uint64_t kTestSCTTimestamp =
GG_UINT64_C(1365181456089);

On 2013/11/16 22:59:16, eranm wrote:
>
> A rather lame reason: The Windows trybot did not like this variable
declaration
> (compile failed) ...

That's strange. I wonder if the problem is as simple as uint64 vs. uint64_t.

> It does matter - this value is used during the creation of a
> SignedCertificateTimestamp instance that verifies over the test certificate
> defined above.

Thanks. What date/time does this timestamp represent? Would be nice to have a
comment because I was curious about it.

https://codereview.chromium.org/55953002/diff/1/net/test/ct_test_util.cc#newc...
net/test/ct_test_util.cc:150:
base::TimeDelta::FromMilliseconds(GG_UINT64_C(1365181456275));

Nit: since FromMilliseconds takes an int64 argument, it would be better to use
GG_INT64_C instead.