atwilson can you decide the correct priority for this issue?

Joao, is the extension policy cache data signed at all currently?

Mattias, what would be sufficient to address this vulnerability? Is the cache accessible by the chrome process, or is it maintained by session manager as other types of policy are?

Cache is maintained by Chrome currently, so more exposed to malicious tampering.

For the question to Joao, I was assuming that the server responses are signed, but I haven't verified. It's pretty simple to do however, just configure some policy for extension settings and check what ends up on disk (you might have to peek into the leveldb though).

FWIW the blobs for components on my profile are signed. That TODO is embarrassing ._.

I thought the main policy blob had a SHA1 hash for the policy blob for each extension.

Yeah, looks like ExternalPolicyData has a secure_hash attribute, so if we keep this around in our device/user policy store, then we could use this to verify the cached ExternalPolicyData. We have some nooglers joining in a month or so and this might be a good task for them to take on.

Re #6: The problem is not with the secure_hash field, but with the signature checking on the ExternalPolicyData which contains it. The latter is embedded in a PolicyFetchResponse, and the signature on that is not validated.

Reassigning this to Maksim as I suspect this is a launch blocker for Extended Login stuff. Up to you whether you want to tackle this early, or wait for nooglers to join Nov 2nd and guide them through it.

Some additional notes:

* The signature validation should support signing key rotation. It should probably just refer to the same key as the related user/device policy. But it also have to be checked that everything behaves reasonably well when the signature validation fails (probably, this could happen during some short window during the actual key rotation).

* Generally, the component cloud policy validation should include all typical validation steps that are currently applied to other policies. For instance, looks like currently the timestamp validation is missing for the component cloud policy.

Correction of comment #9:

The signature validation of the component policy should track the signing key according to the new_public_key in corresponding policy fetch responses. I.e. it should _NOT_ refer to the signing key used by for the user cloud policy.

That's because, according to Dennis, the server fills the new_public_key field independently for different PolicyFetchResponse's, and there is no guarantee that the signing key is the same in all of them.

Apparently, the client implementation is written in such a way that the same public_key_version is used for all policy requests coming from the same policy client:
it's stored in member CloudPolicyClient::public_key_version_ and is then used when constructing all policy fetch requests:
https://cs.chromium.org/chromium/src/components/policy/core/common/cloud/cloud_policy_client.cc?rcl=1477021054&l=232

This probably means that there's different understanding of how signing keys are expected to work on the server side and on the client side: the former seems to put key information into different PolicyFetchResponse's independently, while the latter seems to expect the key information to be shared among them.


Does anybody has the ideas on how to proceed with this?
I see two options:
1) Take for granted that client expects the signing key to be shared among all current policy responses.
Implement component policy validation by using the same signing key that is used for the user policy validation.
Change DMServer to guarantee that the same signing key will always be used.
2) Consider this as the client's bug.
Start tracking and storing somewhere the signing key for the component policies - and do this independently from how this all works for the user policy.
Refactor client to allow to supply different public_key_version for different policy requests.

Not sure which one has more sense - from the robustness perspective, complexity of the changes, etc...

Note that if you allow different signing keys and track them independently, you'll also need to take into account client-side security considerations: We currently rely on session_manager to maintain the signing key for device and user policy, so we can be relatively confident Chrome exploits can't change it. If you track extension signing keys separately, you'd have add tracking logic for them to session_manager as well?

The simplest solution from my perspective is to do key maintenance only with respect to the main user or device policy blob, and have any subordinate policy blob signed by the same key.

Agreed, that feels like the right solution. There's no reason why we'd ever want to rotate extension policy keys separately from user policy keys.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40777b773f442cd06df172d812a5ad2751e3552b

commit 40777b773f442cd06df172d812a5ad2751e3552b
Author: emaxx <emaxx@chromium.org>
Date: Wed Nov 16 02:35:33 2016

Don't pass domain and verification key to validation when not required

This CL decouples the CloudPolicyValidator::ValidateSignature method
into two different methods. One is purely for signature checking, and
the other is for signature checking _and_ for the key rotation
validation.

The previous solution was to have a single method that accepts all
credentials and a boolean flag. This is a bit confusing, as it's not
obvious which parameters will be actually used. Seems that there's a
number of places in the code currently where this confusion resulted
in passing (and obtaining from somewhere) the credentials that will
be actually ignored.

BUG= 644632 

Review-Url: https://codereview.chromium.org/2494843002
Cr-Commit-Position: refs/heads/master@{#432360}

[modify] https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b/chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
[modify] https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b/chrome/browser/chromeos/policy/device_local_account_policy_store.cc
[modify] https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc
[modify] https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b/chrome/browser/chromeos/settings/session_manager_operation.cc
[modify] https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b/components/policy/core/common/cloud/cloud_policy_validator.cc
[modify] https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b/components/policy/core/common/cloud/cloud_policy_validator.h
[modify] https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
[modify] https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b/components/policy/core/common/cloud/user_cloud_policy_store.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1

commit cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1
Author: emaxx <emaxx@chromium.org>
Date: Tue Nov 22 17:28:50 2016

Expose signing key from cloud policy stores

This change makes the cloud policy stores (user, device, device-local)
to expose more information about the policies that they own: the public
part of the signing key. This was not exposed by their interfaces
previously.

No behavior change is expected to be introduced by this CL.

This change is intended to be a base for the subsequent implementation
of the component cloud policy signature verification.

BUG= 644632 
TEST=new unit tests

Review-Url: https://codereview.chromium.org/2488573003
Cr-Commit-Position: refs/heads/master@{#433902}

[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos_unittest.cc
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/chrome/browser/chromeos/policy/device_local_account_policy_store.cc
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/chrome/browser/chromeos/policy/device_local_account_policy_store.h
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/components/policy/core/common/cloud/cloud_policy_store.h
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/components/policy/core/common/cloud/mock_cloud_policy_store.h
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/components/policy/core/common/cloud/mock_user_cloud_policy_store.h
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/components/policy/core/common/cloud/user_cloud_policy_store.cc
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/components/policy/core/common/cloud/user_cloud_policy_store.h
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/components/policy/core/common/cloud/user_cloud_policy_store_base.cc
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/components/policy/core/common/cloud/user_cloud_policy_store_base.h
[modify] https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1/components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc

(Better late than never) Here's the Design Doc for the client side changes:
https://docs.google.com/document/d/1MdeYDguulkyaM8zW5ZyP54TKy2fx7yFgCezCp4Cb6LE

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f

commit 4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f
Author: emaxx <emaxx@chromium.org>
Date: Mon Nov 28 21:54:27 2016

Implement component cloud policy signature validation

This adds signature validation for the component cloud policy
(e.g. policy for extensions). The signature is validated against
the same key that is used for the "superior" policy (e.g. the
user policy, the device local account policy, etc.).

This CL also adds keeping a copy of the most recent component
cloud policy and rechecking it when some of the credentials
change. This allows to handle key rotations gracefully: even
though the component cloud policy may fail the validation
immediately after the cloud policy refresh with the rotated key,
it will be re-validated and applied when the superior policy gets
processed and the credentials get propagated.

BUG= 644632 
TEST=existing tests (now with the signature checks enabled),
     new unit tests and new browser test

Review-Url: https://codereview.chromium.org/2493603002
Cr-Commit-Position: refs/heads/master@{#434730}

[modify] https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f/chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc
[modify] https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f/components/policy/core/common/cloud/component_cloud_policy_service.cc
[modify] https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f/components/policy/core/common/cloud/component_cloud_policy_service_unittest.cc
[modify] https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f/components/policy/core/common/cloud/component_cloud_policy_store.cc
[modify] https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f/components/policy/core/common/cloud/component_cloud_policy_store.h
[modify] https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f/components/policy/core/common/cloud/component_cloud_policy_store_unittest.cc
[modify] https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f/components/policy/core/common/cloud/component_cloud_policy_updater.cc
[modify] https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f/components/policy/core/common/cloud/component_cloud_policy_updater.h
[modify] https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f/components/policy/core/common/cloud/component_cloud_policy_updater_unittest.cc

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot