Implement the ability to whitelist certs from specific issuers

Implement the ability to whitelist leaf-certs that chain to specific issuers.

BUG=473408
Committed: https://crrev.com/cbca7459bce1e95df013543ccba88fa0aa914252
Cr-Commit-Position: refs/heads/master@{#323661}

[Ryan Sleevi, 2015-03-30 21:31:24]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2015-03-30 21:32:22]

agl: FYI
david: WIP.

I'm whitelisting by the root here, but I'm debating whether should cover the
intermediate keys as well.

[davidben, 2015-03-30 21:59:14]

I'm assume you'll update this later with tests and the actual hashes and stuff,
so I'll hold on stamping until then.

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

https://codereview.chromium.org/1042973002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:43: #include
"net/cert/cert_verify_proc_whitelist-inc.cc"
Maybe cert_verify_proc_whitelist.h or cert_verify_proc_whitelist_data.h? The
hyphen and .cc are weird. (.h is also off, but there's precedence with
net_error_list.h and transport_security_state_static.h)

https://codereview.chromium.org/1042973002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:250: }
Just to confirm this is fine: were the underlying OS to completely remove some
root we apply this treatment to, the whitelist will become void and we won't
trust anything from it, whitelist or no. Also if the OS does something similar,
the net whitelist will be the intersection.

(I think this is fine, but wanted to confirm.)

https://codereview.chromium.org/1042973002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:697: bsearch(&leaf_hash,
kWhitelistedIssuers[i].whitelist,
Optional: std::lower_bound might save you some casts and inlines the
element-size computation. Though you'll have to do an extra check, so I dunno.

https://codereview.chromium.org/1042973002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:699: sizeof(leaf_hash.data),
CompareHashValueToRawHash);
Nit: Probably clearer to do sizeof(leaf_data.data) ->
sizeof(kWhitelistIssuers[i].whitelist[0])

(static_assert? This code assumes those two are equal, as are
crypto::kSHA256Length and sizeof(kWhitelistedIssuers[i].public_key). All of
which are blatantly true, of course. I guess if nothing else, this way you won't
accidentally forget to update things if the hash changes.)

[Ryan Sleevi, 2015-03-30 22:27:06]

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

https://codereview.chromium.org/1042973002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:43: #include
"net/cert/cert_verify_proc_whitelist-inc.cc"
On 2015/03/30 21:59:14, David Benjamin wrote:
> Maybe cert_verify_proc_whitelist.h or cert_verify_proc_whitelist_data.h? The
> hyphen and .cc are weird. (.h is also off, but there's precedence with
> net_error_list.h and transport_security_state_static.h)

net_error_list makes some sense because it's included in multiple files.
transport_security_state_static doesn't, and it's bothered me it's a .h

I did -inc, matching registry controlled domains, but it looks like it should be
.inc (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Self_containe...
)

https://codereview.chromium.org/1042973002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:250: }
On 2015/03/30 21:59:14, David Benjamin wrote:
> Just to confirm this is fine: were the underlying OS to completely remove some
> root we apply this treatment to, the whitelist will become void and we won't
> trust anything from it, whitelist or no. Also if the OS does something
similar,
> the net whitelist will be the intersection.
> 
> (I think this is fine, but wanted to confirm.)

Yup

https://codereview.chromium.org/1042973002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:697: bsearch(&leaf_hash,
kWhitelistedIssuers[i].whitelist,
On 2015/03/30 21:59:14, David Benjamin wrote:
> Optional: std::lower_bound might save you some casts and inlines the
> element-size computation. Though you'll have to do an extra check, so I dunno.

Adds an extra compare and the extra check. Also ends up with bigger code-size
hit

https://codereview.chromium.org/1042973002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:699: sizeof(leaf_hash.data),
CompareHashValueToRawHash);
On 2015/03/30 21:59:14, David Benjamin wrote:
> Nit: Probably clearer to do sizeof(leaf_data.data) ->
> sizeof(kWhitelistIssuers[i].whitelist[0])

I don't believe that's guaranteed to optimize the same. It certainly should
raise red-flags for reviewers "what if whitelist_size is zero" and having to
reason about the compile-time correctness.

> (static_assert? This code assumes those two are equal, as are
> crypto::kSHA256Length and sizeof(kWhitelistedIssuers[i].public_key). All of
> which are blatantly true, of course. I guess if nothing else, this way you
won't
> accidentally forget to update things if the hash changes.)

I actually lean against that entirely and favoured just an explicit 32. The size
of a SHA-2 hash isn't going to change :/

[davidben, 2015-03-30 22:29:22]

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

https://codereview.chromium.org/1042973002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:699: sizeof(leaf_hash.data),
CompareHashValueToRawHash);
On 2015/03/30 22:27:05, Ryan Sleevi wrote:
> On 2015/03/30 21:59:14, David Benjamin wrote:
> > Nit: Probably clearer to do sizeof(leaf_data.data) ->
> > sizeof(kWhitelistIssuers[i].whitelist[0])
> 
> I don't believe that's guaranteed to optimize the same. It certainly should
> raise red-flags for reviewers "what if whitelist_size is zero" and having to
> reason about the compile-time correctness.
> 
> > (static_assert? This code assumes those two are equal, as are
> > crypto::kSHA256Length and sizeof(kWhitelistedIssuers[i].public_key). All of
> > which are blatantly true, of course. I guess if nothing else, this way you
> won't
> > accidentally forget to update things if the hash changes.)
> 
> I actually lean against that entirely and favoured just an explicit 32. The
size
> of a SHA-2 hash isn't going to change :/

Oh, I just meant if you were to switch to a different hash function. Though I
suppose tests would notice that anyway, so *shrug*.

[Ryan Sleevi, 2015-03-31 00:28:45]

Updated with unittests; removed specific whitelist for now.

[davidben, 2015-03-31 02:02:13]

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:15: #include "crypto/sha2.h"
No longer necessary here?

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_whitelist.cc (right):

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist.cc:22: const size_t kBuiltinWhitelistSize =
0;
I'm guessing this will later be replaced with a #include blahblah.inc?

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist.cc:42: return false;
The for loop still works out fine. i < g_whitelist_size is still defined when i
= 0 and g_whitelist_size = 0. It'll be false, but that's what you want. (I'm
guessing you added this to deal with the compiler warning when it deduced that
g_whitelist_size is always zero? I don't think this is a concern anymore with
SetCertificateWhitelistForTesting existing.)

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist.cc:44: for (const auto& hash :
public_key_hashes) {
Potential nuisance: if a root we whitelist ever cross-signs an otherwise valid
root and the chain builder is being obnoxious, we might end up rejecting certs.
Hopefully we've flushed out those problems with the SHA-256 work. (Though worst
comes to worst, we could always add the ability to whitelist intermediates.)

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_whitelist_unittest.cc (right):

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist_unittest.cc:34:
public_key_hashes.push_back(GetTestHashValue(0x05, HASH_VALUE_SHA1));
It's a little confusing that this specifies a fake leaf hash while the whitelist
uses the real one.

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist_unittest.cc:44: const uint8
kWhitelistCerts[][crypto::kSHA256Length] = {
uint8_t

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist_unittest.cc:48: 0xc3, 0x9a, 0x14, 0x53,
0xc3, 0x83, 0xa0, 0x5a },
[Verified this is the SHA-256 hash of ok_cert.pem's DER form.]

It does annoy me that the value's hardcoded in here. It may be worth a comment
here just so that, when it comes time to update ok_cert.pem, you don't have to
reread the test to figure out how you're meant to generate these.

(I was going to suggest making this non-const and computing the hash during the
test... I suppose the upside is it's clear what's going on. The downside is that
you call the same leaf-hashing function that the real code uses. Maybe pull it
up into a const uint8_t kOkCertWhitelist constant up top, so it's only there
once?)

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist_unittest.cc:67:
TEST(CertVerifyProcWhitelistTest, AcceptsWhitelistedEEByIntermediate) {
Probably also worth a RejectsNonWhitelistedEEByIntermediate. This doesn't
distinguish between the cert being acceptable because it's whitelisted and the
cert being acceptable because the root is ignored.

[Ryan Sleevi, 2015-03-31 18:33:52]

PTAL

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_whitelist.cc (right):

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist.cc:22: const size_t kBuiltinWhitelistSize =
0;
On 2015/03/31 02:02:13, David Benjamin wrote:
> I'm guessing this will later be replaced with a #include blahblah.inc?

Yup. Potentially in a subsequent CL.

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist.cc:42: return false;
On 2015/03/31 02:02:13, David Benjamin wrote:
> The for loop still works out fine. i < g_whitelist_size is still defined when
i
> = 0 and g_whitelist_size = 0. It'll be false, but that's what you want. (I'm
> guessing you added this to deal with the compiler warning when it deduced that
> g_whitelist_size is always zero? I don't think this is a concern anymore with
> SetCertificateWhitelistForTesting existing.)

Right, it's defined, I just thought it was more readable with the initial
condition :)

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist.cc:44: for (const auto& hash :
public_key_hashes) {
On 2015/03/31 02:02:13, David Benjamin wrote:
> Potential nuisance: if a root we whitelist ever cross-signs an otherwise valid
> root and the chain builder is being obnoxious, we might end up rejecting
certs.
> Hopefully we've flushed out those problems with the SHA-256 work. (Though
worst
> comes to worst, we could always add the ability to whitelist intermediates.)

We already implicitly have the ability to whitelist intermediates. We just
whitelist based on key hash. So we could whitelist on either degree. I chose
root to be more specific.

But yes, it's valid in as much as it's an "attack" scenario, at which point, the
CA would just lose their whitelist because they were engaging in a DoS.

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_whitelist_unittest.cc (right):

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist_unittest.cc:34:
public_key_hashes.push_back(GetTestHashValue(0x05, HASH_VALUE_SHA1));
On 2015/03/31 02:02:13, David Benjamin wrote:
> It's a little confusing that this specifies a fake leaf hash while the
whitelist
> uses the real one.

It's a public key hash. The whitelist uses a *cert* hash.

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist_unittest.cc:48: 0xc3, 0x9a, 0x14, 0x53,
0xc3, 0x83, 0xa0, 0x5a },
On 2015/03/31 02:02:13, David Benjamin wrote:
> [Verified this is the SHA-256 hash of ok_cert.pem's DER form.]
> 
> It does annoy me that the value's hardcoded in here. It may be worth a comment
> here just so that, when it comes time to update ok_cert.pem, you don't have to
> reread the test to figure out how you're meant to generate these.

Fair enough.

> (I was going to suggest making this non-const and computing the hash during
the
> test... I suppose the upside is it's clear what's going on. The downside is
that
> you call the same leaf-hashing function that the real code uses. Maybe pull it
> up into a const uint8_t kOkCertWhitelist constant up top, so it's only there
> once?)

Even if non-const, because it's an array of array, you'd have to memcpy in.
Unless you made it a pointer to a pointer, but then you lose the RO-data
optimization.

[davidben, 2015-03-31 18:42:05]

lgtm

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_whitelist.cc (right):

https://codereview.chromium.org/1042973002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_whitelist.cc:42: return false;
On 2015/03/31 18:33:52, Ryan Sleevi wrote:
> On 2015/03/31 02:02:13, David Benjamin wrote:
> > The for loop still works out fine. i < g_whitelist_size is still defined
when
> i
> > = 0 and g_whitelist_size = 0. It'll be false, but that's what you want. (I'm
> > guessing you added this to deal with the compiler warning when it deduced
that
> > g_whitelist_size is always zero? I don't think this is a concern anymore
with
> > SetCertificateWhitelistForTesting existing.)
> 
> Right, it's defined, I just thought it was more readable with the initial
> condition :)

Mmm. I generally don't like unnecessary special-cases. They make me stop to
wonder why the general case didn't work. I suppose the compiler will figure it
out one way or another. *shrug*

[Ryan Sleevi, 2015-04-02 22:09:44]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-04-03 01:02:58]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-04-03 01:02:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1042973002/#ps100001
(title: "Fix test")

[Ryan Sleevi, 2015-04-03 02:24:02]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-04-03 02:24:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1042973002/#ps120001
(title: "Stupid windows")

[commit-bot: I haz the power, 2015-04-03 04:43:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1042973002/120001

[commit-bot: I haz the power, 2015-04-03 07:45:53]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-03 20:34:02]

Patchset 7 (id:??) landed as
https://crrev.com/cbca7459bce1e95df013543ccba88fa0aa914252
Cr-Commit-Position: refs/heads/master@{#323661}