Certificate Transparency: Parse Signed Tree Heads and validate them

Certificate Transparency: Parse Signed Tree Heads and validate them
This change lays the groundwork for fetching STHs from CT logs and
using them for proof inclusion validation.
This change contains:
* A SignedTreeHead struct representing a signed tree head.
* CTLogResponseParser - a class to parse the STH returned by the log in
  JSON format and fill in SignedTreeHead.
* An encoding function to create the binary blob over which the signature
  in the STH applies.
* Addition to the CTLogVerifier class to validate and store provided STHs,
  which uses the encoding function mentioned above.

BUG=

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

[Eran Messeri, 2014-04-09 16:03:40]

Another round of Certificate Transparency-related changes for you to enjoy!

This CL just lays out the groundwork, does not do any HTTP fetching, so I hope
it's straightforward to review.

[Ryan Sleevi, 2014-04-09 18:23:10]

A little confused why the STH is added to the CTLogVerifier. Can you add a bit
more of context of how you see that being used?

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
File net/cert/ct_log_response_parser.cc (right):

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
net/cert/ct_log_response_parser.cc:22: base::Value* json =
json_reader.Read(json_sth);
Have you looked at base/json/json_value_converter.h for what may be an easier
conversion?

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
File net/cert/ct_log_response_parser.h (right):

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
net/cert/ct_log_response_parser.h:22: static bool FillSignedTreeHead(const
base::StringPiece& json_sth,
Style: Classes with just a static method are generally discouraged - see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...

naming: s/sth/signed_tree_head/ - sth is not going to be a commonly-understood
abbreviation.

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_verifier.cc
File net/cert/ct_log_verifier.cc (right):

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier.cc:52: }
nit: no braces here (or on 30/32), consistent with the existing code (eg: 18/19)

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

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier.h:55: bool
SetSignedTreeHead(scoped_ptr<ct::SignedTreeHead> sth);
nit: s/sth/signed_tree_head/

[Eran Messeri, 2014-04-09 20:49:13]

My goal was to have the CTLogVerifier ultimately verify inclusion proofs. To do
that it needs the signed tree head and an inclusion proof. Since the
CTLogVerifier holds the log's key it can validate the signed tree head prior to
using it.
Ultimately the signed tree head can be stored/used in another class, do you have
a suggestion for a different place to store it?
(As I mentioned the design isn't fully fleshed-out but it's certain we'll need
STH parsing and validation for verification of inclusion proofs).

Will address all other comments tomorrow.

On 2014/04/09 18:23:10, Ryan Sleevi wrote:
> A little confused why the STH is added to the CTLogVerifier. Can you add a bit
> more of context of how you see that being used?
> 
>
https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
> File net/cert/ct_log_response_parser.cc (right):
> 
>
https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
> net/cert/ct_log_response_parser.cc:22: base::Value* json =
> json_reader.Read(json_sth);
> Have you looked at base/json/json_value_converter.h for what may be an easier
> conversion?
> 
>
https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
> File net/cert/ct_log_response_parser.h (right):
> 
>
https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
> net/cert/ct_log_response_parser.h:22: static bool FillSignedTreeHead(const
> base::StringPiece& json_sth,
> Style: Classes with just a static method are generally discouraged - see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...
> 
> naming: s/sth/signed_tree_head/ - sth is not going to be a commonly-understood
> abbreviation.
> 
>
https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_verifier.cc
> File net/cert/ct_log_verifier.cc (right):
> 
>
https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_verifier...
> net/cert/ct_log_verifier.cc:52: }
> nit: no braces here (or on 30/32), consistent with the existing code (eg:
18/19)
> 
>
https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_verifier.h
> File net/cert/ct_log_verifier.h (right):
> 
>
https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_verifier...
> net/cert/ct_log_verifier.h:55: bool
> SetSignedTreeHead(scoped_ptr<ct::SignedTreeHead> sth);
> nit: s/sth/signed_tree_head/

[Eran Messeri, 2014-04-10 21:08:28]

Addressed all comments, PTAL.

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
File net/cert/ct_log_response_parser.cc (right):

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
net/cert/ct_log_response_parser.cc:22: base::Value* json =
json_reader.Read(json_sth);
On 2014/04/09 18:23:11, Ryan Sleevi wrote:
> Have you looked at base/json/json_value_converter.h for what may be an easier
> conversion?
> 

It does make life easier - used than instead, thanks!

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
File net/cert/ct_log_response_parser.h (right):

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_response...
net/cert/ct_log_response_parser.h:22: static bool FillSignedTreeHead(const
base::StringPiece& json_sth,
On 2014/04/09 18:23:11, Ryan Sleevi wrote:
> Style: Classes with just a static method are generally discouraged - see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...
> 
> naming: s/sth/signed_tree_head/ - sth is not going to be a commonly-understood
> abbreviation.

Done and done.

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_verifier.cc
File net/cert/ct_log_verifier.cc (right):

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier.cc:52: }
On 2014/04/09 18:23:11, Ryan Sleevi wrote:
> nit: no braces here (or on 30/32), consistent with the existing code (eg:
18/19)

Done.

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

https://codereview.chromium.org/230713002/diff/20001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier.h:55: bool
SetSignedTreeHead(scoped_ptr<ct::SignedTreeHead> sth);
On 2014/04/09 18:23:11, Ryan Sleevi wrote:
> nit: s/sth/signed_tree_head/

Done.

[Eran Messeri, 2014-04-24 14:02:14]

Ping?

[Ryan Sleevi, 2014-04-25 23:33:19]

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
File net/cert/ct_log_response_parser.cc (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
net/cert/ct_log_response_parser.cc:41:
&JsonSignedTreeHead::tree_head_signature);
SECURITY: Should there be any customer converters attached to these, so that you
can validate their well-formedness?

tree_size or timestamp being negative numbers immediately springs to mind as
being potential security issues

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
File net/cert/ct_log_response_parser.h (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
net/cert/ct_log_response_parser.h:22: SignedTreeHead* signed_tree_head);
Was this the format done by git-cl-format?

note that for return values, the NET_EXPORT appears before, not after, the
return value.

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
File net/cert/ct_log_response_parser_unittest.cc (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
net/cert/ct_log_response_parser_unittest.cc:23:
"\"tree_size\":2903698,\"timestamp\":1395761621447,\"sha256_root_hash\":";
Rather than wrap format strings in constants, create a helper function to create
dummy STHs for you. That way, you can be assured that any changes to the format
string (which would be a function-local constant) are kept in sync with the
argument calls.

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
net/cert/ct_log_response_parser_unittest.cc:43:
ASSERT_TRUE(DecodeDigitallySigned(&sp, &expected_signature_));
Why do you do these in a SetUp, instead of just initializing the variables you
want?

Namely, this implies a tighter coupling to the behaviour of these functions
(base64decode, decodedigitallysigned) than is necessary for your test. Is it
just a convenience thing?

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_verifier.cc
File net/cert/ct_log_verifier.cc (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier.cc:64: const ct::DigitallySigned& signature) {
Should this be a helper on ct::DigitallySigned?

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

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier.h:81: scoped_ptr<ct::SignedTreeHead> sth_;
s/sth_/signed_tree_head_/

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_serialization.h
File net/cert/ct_serialization.h (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_serializatio...
net/cert/ct_serialization.h:54: NET_EXPORT_PRIVATE void
EncodeTreeHeadSignature(const SignedTreeHead& sth,
s/sth/signed_tree_head/

or just tree_head (which would imply updating all the other locations). Just
eschew abbreviating CT types.

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_serializatio...
File net/cert/ct_serialization_unittest.cc (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_serializatio...
net/cert/ct_serialization_unittest.cc:167: GetSignedTreeHead(&sth);
tree_head/signed_tree_head

[Eran Messeri, 2014-04-29 15:22:23]

Thanks for the quick review!
Addressed all comments, PTAL.

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
File net/cert/ct_log_response_parser.cc (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
net/cert/ct_log_response_parser.cc:41:
&JsonSignedTreeHead::tree_head_signature);
On 2014/04/25 23:33:19, Ryan Sleevi wrote:
> SECURITY: Should there be any customer converters attached to these, so that
you
> can validate their well-formedness?
> 
> tree_size or timestamp being negative numbers immediately springs to mind as
> being potential security issues

Good catch - I've added custom converters for the sha256_root_hash and
tree_head_signature. Since I'm further processing this struct later on I've
opted for checks in the FillSignedTreeHead function below for the timestamp and
tree_size.
I could use custom converters for these fields as well but that would require
manually converting the StringPiece to the Int/Double value before doing any
validation checks on them.

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
File net/cert/ct_log_response_parser.h (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
net/cert/ct_log_response_parser.h:22: SignedTreeHead* signed_tree_head);
On 2014/04/25 23:33:19, Ryan Sleevi wrote:
> Was this the format done by git-cl-format?
> 
> note that for return values, the NET_EXPORT appears before, not after, the
> return value.

Done - re-ordered so NET_EXPORT is first.

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
File net/cert/ct_log_response_parser_unittest.cc (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
net/cert/ct_log_response_parser_unittest.cc:23:
"\"tree_size\":2903698,\"timestamp\":1395761621447,\"sha256_root_hash\":";
On 2014/04/25 23:33:19, Ryan Sleevi wrote:
> Rather than wrap format strings in constants, create a helper function to
create
> dummy STHs for you. That way, you can be assured that any changes to the
format
> string (which would be a function-local constant) are kept in sync with the
> argument calls.

Done - it does read much better, the way I originally did it was rather messy.

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_response...
net/cert/ct_log_response_parser_unittest.cc:43:
ASSERT_TRUE(DecodeDigitallySigned(&sp, &expected_signature_));
On 2014/04/25 23:33:19, Ryan Sleevi wrote:
> Why do you do these in a SetUp, instead of just initializing the variables you
> want?
> 
> Namely, this implies a tighter coupling to the behaviour of these functions
> (base64decode, decodedigitallysigned) than is necessary for your test. Is it
> just a convenience thing?

Done - moved to the test, this is indeed was not necessary in the setUp, only
for a specific test.

And yes, the use of base64decode is for convenience - I could write down the
equivalent of the base64-encoded root hash and tree head signature in arrays,
but if anything, I think it'll reduce the readability of the test.

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_verifier.cc
File net/cert/ct_log_verifier.cc (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier.cc:64: const ct::DigitallySigned& signature) {
On 2014/04/25 23:33:19, Ryan Sleevi wrote:
> Should this be a helper on ct::DigitallySigned?

Done.

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

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_log_verifier...
net/cert/ct_log_verifier.h:81: scoped_ptr<ct::SignedTreeHead> sth_;
On 2014/04/25 23:33:19, Ryan Sleevi wrote:
> s/sth_/signed_tree_head_/

Done.

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_serialization.h
File net/cert/ct_serialization.h (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_serializatio...
net/cert/ct_serialization.h:54: NET_EXPORT_PRIVATE void
EncodeTreeHeadSignature(const SignedTreeHead& sth,
On 2014/04/25 23:33:19, Ryan Sleevi wrote:
> s/sth/signed_tree_head/
> 
> or just tree_head (which would imply updating all the other locations). Just
> eschew abbreviating CT types.

Done - changed sth to signed_tree_head.

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_serializatio...
File net/cert/ct_serialization_unittest.cc (right):

https://codereview.chromium.org/230713002/diff/60001/net/cert/ct_serializatio...
net/cert/ct_serialization_unittest.cc:167: GetSignedTreeHead(&sth);
On 2014/04/25 23:33:19, Ryan Sleevi wrote:
> tree_head/signed_tree_head

Done.

[Ryan Sleevi, 2014-05-08 01:11:54]

LGTM! Sorry for the delays - feel free to ping me (email or chat) if I ever have
a delay of > 24 hours.

https://codereview.chromium.org/230713002/diff/100001/net/cert/ct_log_respons...
File net/cert/ct_log_response_parser.cc (right):

https://codereview.chromium.org/230713002/diff/100001/net/cert/ct_log_respons...
net/cert/ct_log_response_parser.cc:123: if
(!IsJsonSTHStructurallyValid(parsed_sth)) {
nit: no braces for the one-liner

https://codereview.chromium.org/230713002/diff/100001/net/cert/ct_log_respons...
File net/cert/ct_log_response_parser_unittest.cc (right):

https://codereview.chromium.org/230713002/diff/100001/net/cert/ct_log_respons...
net/cert/ct_log_response_parser_unittest.cc:46: };
You don't actually need a fixture here, since there is no friending going on
AFAICT.

You can just write them as 

TEST(CTLogResponseParserTest, ...) and drop 43-46 entirely.

https://codereview.chromium.org/230713002/diff/100001/net/cert/signed_certifi...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/230713002/diff/100001/net/cert/signed_certifi...
net/cert/signed_certificate_timestamp.h:68: bool SignatureParametersMatch(
nit: newline
nit: Document

https://codereview.chromium.org/230713002/diff/100001/net/test/ct_test_util.h
File net/test/ct_test_util.h (right):

https://codereview.chromium.org/230713002/diff/100001/net/test/ct_test_util.h...
net/test/ct_test_util.h:69: // The Sha256 root hash for the sample STH
SHA256

[Eran Messeri, 2014-05-12 20:33:55]

https://codereview.chromium.org/230713002/diff/100001/net/cert/ct_log_respons...
File net/cert/ct_log_response_parser.cc (right):

https://codereview.chromium.org/230713002/diff/100001/net/cert/ct_log_respons...
net/cert/ct_log_response_parser.cc:123: if
(!IsJsonSTHStructurallyValid(parsed_sth)) {
On 2014/05/08 01:11:54, Ryan Sleevi wrote:
> nit: no braces for the one-liner

Done.

https://codereview.chromium.org/230713002/diff/100001/net/cert/ct_log_respons...
File net/cert/ct_log_response_parser_unittest.cc (right):

https://codereview.chromium.org/230713002/diff/100001/net/cert/ct_log_respons...
net/cert/ct_log_response_parser_unittest.cc:46: };
On 2014/05/08 01:11:54, Ryan Sleevi wrote:
> You don't actually need a fixture here, since there is no friending going on
> AFAICT.
> 
> You can just write them as 
> 
> TEST(CTLogResponseParserTest, ...) and drop 43-46 entirely.

Done.

https://codereview.chromium.org/230713002/diff/100001/net/cert/signed_certifi...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/230713002/diff/100001/net/cert/signed_certifi...
net/cert/signed_certificate_timestamp.h:68: bool SignatureParametersMatch(
On 2014/05/08 01:11:54, Ryan Sleevi wrote:
> nit: newline
> nit: Document

Done and Done.

https://codereview.chromium.org/230713002/diff/100001/net/test/ct_test_util.h
File net/test/ct_test_util.h (right):

https://codereview.chromium.org/230713002/diff/100001/net/test/ct_test_util.h...
net/test/ct_test_util.h:69: // The Sha256 root hash for the sample STH
On 2014/05/08 01:11:54, Ryan Sleevi wrote:
> SHA256

Done, here and in one other place.

[Eran Messeri, 2014-05-12 20:34:04]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2014-05-12 20:34:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eranm@chromium.org/230713002/120001

[commit-bot: I haz the power, 2014-05-12 21:56:35]

Change committed as 269887