Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2494843002:
Don't pass domain and verification key to validation when not required (Closed)
DescriptionDon'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
Committed: https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b
Cr-Commit-Position: refs/heads/master@{#432360}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address feedback #
Total comments: 6
Patch Set 3 : Fix nit #Patch Set 4 : Rebase #Messages
Total messages: 48 (31 generated)
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Don't pass domain and verification key to validation when not required BUG=644632 ========== to ========== 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 ==========
emaxx@chromium.org changed reviewers: + atwilson@chromium.org, mnissler@chromium.org, tnagel@chromium.org
PTAL. If I'm reading the validator code correctly, the domain and the verification key were not used for anything when allow_key_rotation was passed as false. This is not clear from the interface, and fooled me in my other CL (thanks to Thiemo for raising the question of why a domain is necessary for performing the signature check).
LGTM https://codereview.chromium.org/2494843002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/cloud_policy_validator.h (right): https://codereview.chromium.org/2494843002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/cloud_policy_validator.h:190: // verifies against |key|. Nit: I'd suggest to add a comment that ValidateSignature() and ValidateSignatureAllowingRotation() are mutually exclusive. https://codereview.chromium.org/2494843002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/cloud_policy_validator.h:198: // |new_public_key_verification_signature| field. Nit: Actually, it's the new_public_key_verification_signature_deprecated field. And instead of "and the" I'd write "against the proto's" to make it clearer where that field belongs to.
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2494843002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/cloud_policy_validator.h (right): https://codereview.chromium.org/2494843002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/cloud_policy_validator.h:190: // verifies against |key|. On 2016/11/11 15:07:54, Thiemo Nagel (slow) wrote: > Nit: I'd suggest to add a comment that ValidateSignature() and > ValidateSignatureAllowingRotation() are mutually exclusive. I managed to change their implementation so that they can be used together (with DCHECKs that they don't overwrite internal fields) - like the other validator methods behave. https://codereview.chromium.org/2494843002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/cloud_policy_validator.h:198: // |new_public_key_verification_signature| field. On 2016/11/11 15:07:54, Thiemo Nagel (slow) wrote: > Nit: Actually, it's the new_public_key_verification_signature_deprecated field. > And instead of "and the" I'd write "against the proto's" to make it clearer > where that field belongs to. Done.
LGTM https://codereview.chromium.org/2494843002/diff/20001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_validator_unittest.cc (right): https://codereview.chromium.org/2494843002/diff/20001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:117: validator->ValidateInitialKey(GetPolicyVerificationKey(), owning_domain_); nit: it seems worthwhile to fold this line into the conditional you added?
LGTM, I just want to make sure the provenance of the key returned by DeviceSettingsService is known (i.e. that VerifyCachedKey() is invoked on the key when it's loaded, or if the key comes from SessionManager, that ValidateInitialKey() is invoked on the first key before injecting it into SessionManager). https://codereview.chromium.org/2494843002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_local_account_policy_store.cc (right): https://codereview.chromium.org/2494843002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_local_account_policy_store.cc:145: if (!key.get() || !key->is_loaded() || !device_policy_data) { Just to be clear, when is ValidateCachedKey() invoked on this key, to make sure it's valid? Does DeviceSettingsService() validate the key when it's loaded? https://codereview.chromium.org/2494843002/diff/20001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_validator.cc (right): https://codereview.chromium.org/2494843002/diff/20001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_validator.cc:127: DCHECK(key_.empty() || key_ == key); DCHECK(!allow_key_rotation_) ?
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/14 09:29:37, Andrew T Wilson (Slow) wrote:
> LGTM, I just want to make sure the provenance of the key returned by
> DeviceSettingsService is known (i.e. that VerifyCachedKey() is invoked on the
> key when it's loaded, or if the key comes from SessionManager, that
> ValidateInitialKey() is invoked on the first key before injecting it into
> SessionManager).
I agree that it's possible that some security risk was overlooked due to this
tricky CloudPolicyValidator semantics.
That's why I added several reviewers - maybe somebody can see a place where the
expectation from the validation was different from what was actually performed.
From my side, after looking into the validator clients I can make the following
summary (maybe I'm wrong):
1. Data (keys and policy) returned by DeviceSettingsService is not validated on
many code paths.
DeviceCloudPolicyStoreChromeOS, DeviceLocalAccountPolicyStore in most cases just
check the signature using the key from DeviceSettingsService. There's one
special case - installation of the initial policy in the device policy store -
which asks the validator to ValidateInitialKey against the verification key.
2. UserCloudPolicyStoreChromeOS is also not validating the key loaded from the
cache file ("policy.pub").
(This file is owned by the root though.)
3. UserCloudPolicyStore is the only one which is validating the key loaded from
the cache file ("Signing Key") using the CloudPolicyValidator::ValidateCachedKey
method.
(This file has the usual access attributes.)
I guess the idea was that the cached key doesn't need to be validated when it's
owned by some "daemon" running under the root...
https://codereview.chromium.org/2494843002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account_policy_store.cc
(right):
https://codereview.chromium.org/2494843002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:145: if
(!key.get() || !key->is_loaded() || !device_policy_data) {
On 2016/11/14 09:29:37, Andrew T Wilson (Slow) wrote:
> Just to be clear, when is ValidateCachedKey() invoked on this key, to make
sure
> it's valid? Does DeviceSettingsService() validate the key when it's loaded?
I don't think that the keys from DeviceSettingsService are validated
additionally anywhere.
Not sure whether this is a security risk, as, if I'm not mistaken, the key is
loaded from the file owned by the root.
https://codereview.chromium.org/2494843002/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.cc (right):
https://codereview.chromium.org/2494843002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:127:
DCHECK(key_.empty() || key_ == key);
On 2016/11/14 09:29:37, Andrew T Wilson (Slow) wrote:
> DCHECK(!allow_key_rotation_) ?
I intentionally omitted such DCHECKS in the second review patch, so that the
ValidateSignature and the ValidateSignatureAllowingRotation methods can be used
simultaneously. As the latter one can be considered as the extended version of
the former one, with added support of the key rotation.
Do you think it's better to disallow mixing of these two methods?
https://codereview.chromium.org/2494843002/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
(right):
https://codereview.chromium.org/2494843002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:117:
validator->ValidateInitialKey(GetPolicyVerificationKey(), owning_domain_);
On 2016/11/14 08:46:46, Mattias Nissler (ping if slow) wrote:
> nit: it seems worthwhile to fold this line into the conditional you added?
Good point, done.
Actually, after the refactoring done in this CL it becomes obvious that calling
both methods (ValidateSignatureAllowingRotation and ValidateInitialKey as they
are named now) is a bit strange here, as they instruct the validator to check
the same new_public_key field, with the only difference is that the former
allows the new_public_key to be empty, while the latter requires it to be
non-empty.
So it would be probably better if there were _three_ groups of tests: 1) without
key rotation checking, 2) with optional key rotation checking, 3) with initial
key checking.
But I think that leaving the tests as is should be also fine, as at least they
cover all code paths in the validator. (As long as there are no complaints from
the reviewers.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/14 16:27:07, emaxx wrote:
> On 2016/11/14 09:29:37, Andrew T Wilson (Slow) wrote:
> > LGTM, I just want to make sure the provenance of the key returned by
> > DeviceSettingsService is known (i.e. that VerifyCachedKey() is invoked on
the
> > key when it's loaded, or if the key comes from SessionManager, that
> > ValidateInitialKey() is invoked on the first key before injecting it into
> > SessionManager).
>
> I agree that it's possible that some security risk was overlooked due to this
> tricky CloudPolicyValidator semantics.
> That's why I added several reviewers - maybe somebody can see a place where
the
> expectation from the validation was different from what was actually
performed.
>
>
> From my side, after looking into the validator clients I can make the
following
> summary (maybe I'm wrong):
>
> 1. Data (keys and policy) returned by DeviceSettingsService is not validated
on
> many code paths.
> DeviceCloudPolicyStoreChromeOS, DeviceLocalAccountPolicyStore in most cases
just
> check the signature using the key from DeviceSettingsService. There's one
> special case - installation of the initial policy in the device policy store -
> which asks the validator to ValidateInitialKey against the verification key.
>
> 2. UserCloudPolicyStoreChromeOS is also not validating the key loaded from the
> cache file ("policy.pub").
> (This file is owned by the root though.)
Yeah, it probably should, although I bet we're not storing the signature for the
key anywhere so there's some migration issues.
Mattias, do you know why we fetch the policy from session manager, but then read
the signing key from a file? In any case, as you say, not a big deal since the
file is not easily writable by Chrome.
>
> 3. UserCloudPolicyStore is the only one which is validating the key loaded
from
> the cache file ("Signing Key") using the
CloudPolicyValidator::ValidateCachedKey
> method.
> (This file has the usual access attributes.)
And that's to be expected since this is what's used on desktop.
>
>
> I guess the idea was that the cached key doesn't need to be validated when
it's
> owned by some "daemon" running under the root...
In general, I didn't bother adding key validation to Chrome OS when I rolled it
out, because I didn't want to bother with migration issues. We may want to
address that, but again it requires migration (we need to start saving key
signatures).
>
>
https://codereview.chromium.org/2494843002/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/policy/device_local_account_policy_store.cc
> (right):
>
>
https://codereview.chromium.org/2494843002/diff/20001/chrome/browser/chromeos...
> chrome/browser/chromeos/policy/device_local_account_policy_store.cc:145: if
> (!key.get() || !key->is_loaded() || !device_policy_data) {
> On 2016/11/14 09:29:37, Andrew T Wilson (Slow) wrote:
> > Just to be clear, when is ValidateCachedKey() invoked on this key, to make
> sure
> > it's valid? Does DeviceSettingsService() validate the key when it's loaded?
>
> I don't think that the keys from DeviceSettingsService are validated
> additionally anywhere.
> Not sure whether this is a security risk, as, if I'm not mistaken, the key is
> loaded from the file owned by the root.
Given the security of the key storage, that's probably OK. Would be good to
understand if the initial policy storage goes through ValidateInitialKey() -
this is outside the scope of this CL though.
LGTM. It's fine to allow both ValidateSignatureXXX() calls to be made, I guess. Shouldn't break anything at least.
On 2016/11/15 16:13:11, Andrew T Wilson (Slow) wrote: > Mattias, do you know why we fetch the policy from session manager, but then read > the signing key from a file? In any case, as you say, not a big deal since the > file is not easily writable by Chrome. The reason is elevated levels of paranoia during the early days (i.e. doubts about trusting dbus-daemon). Meanwhile, we've moved into a place where noone can reasonably claim we're not factually trusting dbus-daemon fully already. I think in the current world, it'd actually be acceptable to trust whatever data we receive from DBus without further validation on the basis that if dbus-daemon is compromised we got bigger problems than just incorrect policy appearing in Chrome.
OK, so looks like it's all good, and no faults were found. I'll land this refactoring CL then.
The CQ bit was checked by emaxx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/2494843002/#ps40001 (title: "Fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by emaxx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org, atwilson@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/2494843002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by emaxx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b Cr-Commit-Position: refs/heads/master@{#432360} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/40777b773f442cd06df172d812a5ad2751e3552b Cr-Commit-Position: refs/heads/master@{#432360} |
