Update policy signature verification to include policy domain.

Update policy signature verification to include policy domain.

CloudPolicyValidator now accpets a "domain" parameter which is used to generate
verification signatures for public keys.

Broke out CloudPolicyValidator cached-key verification code into a separate
validation function: ValidateCachedKey().

Added new hard-coded signatures for our PolicyBuilder test keys for the
example.com domain.

BUG=275291
TBR=rogerta@chromium.org

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

[Andrew T Wilson (Slow), 2014-02-12 15:20:11]

PTAL

[Mattias Nissler (ping if slow), 2014-02-13 10:52:52]

Just a few nits and requests for additional tests.

https://codereview.chromium.org/143183007/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/143183007/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:113: std::string
domain;
nit: Can you move this after the comment?

https://codereview.chromium.org/143183007/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/143183007/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.cc:186: // generated
by session manager aren't signed by DMServer.
technically, policy blobs _are_ signed. We don't check the certificate for the
signing key though, and the reason for that is that we've done these validations
when installing a new key.

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.cc (right):

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:310:
signed_data.set_new_public_key(key);
nit: newline before comment

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
(right):

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:326: }
Might want to add test cases for the cases you added (i.e. domain mismatch, no
domain).

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
File components/policy/core/common/cloud/policy_builder.cc (right):

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.cc:21: 0x30, 0x82, 0x01,
0x55, 0x02, 0x01, 0x00, 0x30, 0x0d, 0x06, 0x09, 0x2a,
I think the previous indentation was correct per the style guide?

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
File components/policy/core/common/cloud/policy_builder.h (right):

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.h:93: std::vector<uint8>
raw_new_signing_key() { return raw_new_signing_key_; }
nit: newline

https://codereview.chromium.org/143183007/diff/100001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/143183007/diff/100001/components/policy/proto...
components/policy/proto/device_management_backend.proto:320: optional bytes
new_public_key = 1;
nit: newline

[Andrew T Wilson (Slow), 2014-02-13 12:15:35]

Roger, PTAL at the change to gaia_auth_util.cc (just an improved logging
message) for your owners stamp.

Mattias: feedback addressed, PTAL.

https://codereview.chromium.org/143183007/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/143183007/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:113: std::string
domain;
On 2014/02/13 10:52:53, Mattias Nissler wrote:
> nit: Can you move this after the comment?

Done.

https://codereview.chromium.org/143183007/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/143183007/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.cc:186: // generated
by session manager aren't signed by DMServer.
On 2014/02/13 10:52:53, Mattias Nissler wrote:
> technically, policy blobs _are_ signed. We don't check the certificate for the
> signing key though, and the reason for that is that we've done these
validations
> when installing a new key.
They are signed, but not signed by DMServer, right? Anyhow, I updated this
comment to be more agnostic about who signs it and to reflect your feedback.

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.cc (right):

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:310:
signed_data.set_new_public_key(key);
On 2014/02/13 10:52:53, Mattias Nissler wrote:
> nit: newline before comment

Done.

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
(right):

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:326: }
On 2014/02/13 10:52:53, Mattias Nissler wrote:
> Might want to add test cases for the cases you added (i.e. domain mismatch, no
> domain).

Done.

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
File components/policy/core/common/cloud/policy_builder.cc (right):

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.cc:21: 0x30, 0x82, 0x01,
0x55, 0x02, 0x01, 0x00, 0x30, 0x0d, 0x06, 0x09, 0x2a,
On 2014/02/13 10:52:53, Mattias Nissler wrote:
> I think the previous indentation was correct per the style guide?

It's ambiguous, but Bartosz + other instances in the source code have convinced
me that 4 spaces is correct. So I've updated this.

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
File components/policy/core/common/cloud/policy_builder.h (right):

https://codereview.chromium.org/143183007/diff/100001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.h:93: std::vector<uint8>
raw_new_signing_key() { return raw_new_signing_key_; }
On 2014/02/13 10:52:53, Mattias Nissler wrote:
> nit: newline

Done.

https://codereview.chromium.org/143183007/diff/100001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/143183007/diff/100001/components/policy/proto...
components/policy/proto/device_management_backend.proto:320: optional bytes
new_public_key = 1;
On 2014/02/13 10:52:53, Mattias Nissler wrote:
> nit: newline

Done.

[Mattias Nissler (ping if slow), 2014-02-13 13:35:54]

LGTM.

https://codereview.chromium.org/143183007/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/143183007/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.cc:186: // generated
by session manager aren't signed by DMServer.
On 2014/02/13 12:15:36, Andrew T Wilson wrote:
> On 2014/02/13 10:52:53, Mattias Nissler wrote:
> > technically, policy blobs _are_ signed. We don't check the certificate for
the
> > signing key though, and the reason for that is that we've done these
> validations
> > when installing a new key.
> They are signed, but not signed by DMServer, right? Anyhow, I updated this
> comment to be more agnostic about who signs it and to reflect your feedback.

They are signed by DMServer. We just store the PolicyFetchResponse received from
the server.

[Andrew T Wilson (Slow), 2014-02-13 17:22:56]

The CQ bit was checked by atwilson@chromium.org

[commit-bot: I haz the power, 2014-02-13 17:24:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/143183007/370001

[Roger Tawa OOO till Jul 10th, 2014-02-13 17:38:23]

gaia_auth_util.cc lgtm

[commit-bot: I haz the power, 2014-02-13 19:10:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 19:10:48]

Retried try job too often on win_rel for step(s) printing_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Andrew T Wilson (Slow), 2014-02-14 08:11:30]

The CQ bit was checked by atwilson@chromium.org

[commit-bot: I haz the power, 2014-02-14 08:12:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/143183007/370001

[commit-bot: I haz the power, 2014-02-14 13:50:58]

Change committed as 251292

[palmer, 2014-02-14 21:52:48]

https://codereview.chromium.org/143183007/diff/370001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/143183007/diff/370001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.cc:186: // key is
validated when it is installed.
What is the harm of checking it again?

https://codereview.chromium.org/143183007/diff/370001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.cc (right):

https://codereview.chromium.org/143183007/diff/370001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:394: if
(!cached_key_signature_.empty() && !verification_key_.empty() &&
So if an attacker can supply an empty signature somehow, they can pass this
check?

[Andrew T Wilson (Slow), 2014-02-17 17:28:39]

https://codereview.chromium.org/143183007/diff/370001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/143183007/diff/370001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.cc:186: // key is
validated when it is installed.
On 2014/02/14 21:52:48, Chromium Palmer wrote:
> What is the harm of checking it again?
None, really - I've also spoken with mnissler about this. The problem is that
there are cases where this data is signed with the owners key for the device
(see the comment above about "policy auto-generated by session manager" - not
downloaded/signed by DMServer), and this owners key is not currently signed with
a verification key. Before we can start checking owner key verification
signatures, we need some way to migrate all of the existing devices to *have*
owner key verification signatures, which is an issue we're punting on now since
the verification signature is currently only critical for desktop.

I have crbug.com/328038 open to track this (and other) cros key verification
signature issues.

https://codereview.chromium.org/143183007/diff/370001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.cc (right):

https://codereview.chromium.org/143183007/diff/370001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:394: if
(!cached_key_signature_.empty() && !verification_key_.empty() &&
On 2014/02/14 21:52:48, Chromium Palmer wrote:
> So if an attacker can supply an empty signature somehow, they can pass this
> check?

Good catch.

The idea was that this code should never be called with an empty key - there is
code at user_cloud_policy_store.cc:335 that checks for an empty key and deals
with it specially (currently we allow this to pass unmolested for migration).
However, given this special handling at the caller, there's no reason why this
routine should also have special handling for an empty cached_key_signature_ -
it makes this API fragile.

I'll drive a followup CL tomorrow to cause this case (no cached_key_signature
passed to the CloudPolicyValidator) to generate an error.