Consider disabling timestamp validation of enterprise policies |
||||||||
Issue description(forked discussion from http://crbug.com/700244#c10) Generally, I never understood the benefit of checking the policy timestamps. I actually had a brief discussion of this with mnissler@ a couple of months ago, and looks like the main reason for the timestamp validation is preventing rolling the policy back to some previous value. However, from my point of view: 1. When the policy is fetched from the server, it's done through an HTTPS connection. If somebody manages to break into HTTPS - which by itself is an emergency situation - then probably sacrificing the resistance to policy rollbacks is not so bad. (Note that the policy blob itself is validated) 2. When the policy is loaded from local cache, we can only compare its timestamp with the local device clocks, which are untrusted. This implies that we're not resistant to local policy rollbacks even now. So I'm personally standing for removing the timestamp validation at all: the benefits brought by it are quite small, while the disadvantages are those false negatives, which lead to ignoring a perfectly valid policy blob.
,
Apr 11 2017
Quick summary of rationale: #1: Most importantly, rollback prevention. The main threat here is not breaking HTTPS, but a compromised Chrome, for example a student resetting their policy to an earlier version that didn't have some restriction. The enforcement of this is on the session_manager side (I hope it is, didn't check), the checks in Chrome are mostly sanity checks to see if things look sane. #2: Since we're using timestamps for rollback protection, it makes sense to guard against bugs where the server accidentally sets the timestamp to far in the future. If we blindly accepted this (i.e. only enforce monotonicity), then it's hard to recover from that. You'd have to either (1) allow rollback or (2) wait for real time to catch up or (3) have the server continue generating timestamps relative to the accidental large value. Regarding the false negatives: If these show up too frequently, why not adjust the guard intervals as necessary?
,
Apr 11 2017
As a rule, I want to stop relying on client clocks being accurate. They aren't, and we keep getting bitten by this. So +1 to removing *all* reliance on clocks being correct (modulo cases like cert verification that inherently must have some kind of local clock). For #1 (rollback prevention), agreed that we need to compare *policy* timestamps to prevent injection of previous policy. So let's preserve that. For #2, I'm 100% OK with not catching server bugs that set an invalid timestamp. I can think of lots of changes we could push that would allow us to recover from invalid policy timestamps (for example, we could skip the "check policy timestamp vs existing policy timestamp" check in the case that we're doing a key rotation - the key rotation would protect against rollbacks since the new key has to be signed with the existing key). It doesn't make sense to do this until the case that we have a server bug though.
,
Apr 11 2017
Re comment 2: Thanks for the explanation. Agreed that it makes sense to compare timestamps on the session_manager side to prevent rollbacks. However, I don't think this is implemented currently: AFAICS, it only checks the protobuf validity and the policy data signature: https://cs.corp.google.com/chromeos_public/src/platform2/login_manager/user_policy_service.cc?l=69 https://cs.corp.google.com/chromeos_public/src/platform2/login_manager/policy_service.cc?l=158 We should probably file a bug for adding rollback prevention to session_manager. RE comment 3: Regarding server side bugs - it seems that at least this never happened in the past (didn't manage to find anything at b/).
,
Apr 11 2017
Yup, session_manager is missing a number of critical checks, the master signing key check being another one. I have that on my list, but it's not high priority at the moment. How paranoid you want to be about sanity checks is your decision now. For background, I wrote the policy validation logic after we had generated multiple situations of request loops and only avoided self-inflicted DoS by sheer luck :)
,
Apr 18 2017
,
Apr 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5a159859f7164e629c68d8212db34a711fc5245d commit 5a159859f7164e629c68d8212db34a711fc5245d Author: emaxx <emaxx@chromium.org> Date: Thu Apr 20 10:25:48 2017 Remove the "not_after" validation of policy timestamps This CL disables the validation of policy timestamps against the current device clocks, which was previously there to prevent loading policies "from the future". The disabled validation was originally intended as a protection against a potential bug on DMServer side if it started sending wrongly big timestamps. This could be a problem in theory, as after such a bug occurs on the server side and is fixed then, the clients will refuse the subsequent policy updates due to the protection on the client side against policy rollbacks. However, the decision now is made that this validation brings more drawbacks than benefits as the device's clocks are quite often wrong. BUG= 701045 Review-Url: https://codereview.chromium.org/2820063005 Cr-Commit-Position: refs/heads/master@{#465964} [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/chrome/browser/chromeos/policy/device_local_account_policy_store.cc [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/chrome/browser/chromeos/settings/session_manager_operation.cc [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/components/policy/core/common/cloud/cloud_policy_validator.cc [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/components/policy/core/common/cloud/cloud_policy_validator.h [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/components/policy/core/common/cloud/component_cloud_policy_store.cc [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/components/policy/core/common/cloud/component_cloud_policy_store_unittest.cc [modify] https://crrev.com/5a159859f7164e629c68d8212db34a711fc5245d/components/policy/core/common/cloud/user_cloud_policy_store.cc
,
Apr 21 2017
,
Apr 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880 commit 35629b9c2d21ae7af4ec8ebdcec1c2c21a042880 Author: emaxx <emaxx@chromium.org> Date: Fri Apr 21 14:06:55 2017 Use {To/From}JavaTime for policy timestamps Use base::Time::ToJavaTime() and base::Time::FromJavaTime() for conversions between base::Time and timestamps in the policy protos. No change of behavior should be introduced, it's just a refactoring. BUG= 701045 TEST=existing tests Review-Url: https://codereview.chromium.org/2830033003 Cr-Commit-Position: refs/heads/master@{#466325} [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/chrome/browser/chromeos/policy/device_status_collector.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/chrome/browser/policy/cloud/cloud_policy_invalidator.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/core/common/cloud/cloud_policy_client.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/core/common/cloud/cloud_policy_client_unittest.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/core/common/cloud/cloud_policy_refresh_scheduler_unittest.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/core/common/cloud/cloud_policy_service.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/core/common/cloud/cloud_policy_service_unittest.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/core/common/cloud/cloud_policy_validator.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/core/common/cloud/component_cloud_policy_store.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/core/common/cloud/component_cloud_policy_store_unittest.cc [modify] https://crrev.com/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880/components/policy/proto/device_management_backend.proto
,
Apr 21 2017
,
May 8 2017
Verified in 60.0.3086.0: 9521.0.0 dev caroline, the fix was working fine with the following regressions carried out: 1. Via shell, changed the device local date-time to past. Changed policies on DM server. Tried both manual reloading of policies and waited for policies push from server. Upon policies update, local date-time were reset to present and policies updates were successfully. 2. Same as 1 but changed the local date-time to future. No issue as well. 3. Using shell script, changed local date-time to past/future repeatedly every one sec. Changed policies on DM server. The policies were updated accordingly. Upon stopping the scirpt, the local date-time were reset to present.
,
May 19 2017
Issue 265507 has been merged into this issue.
,
Jun 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/efffaf05f0a47a357ab6955c0a3caf7a4a06ef65 commit efffaf05f0a47a357ab6955c0a3caf7a4a06ef65 Author: tnagel <tnagel@chromium.org> Date: Fri Jun 02 10:00:46 2017 Remove validation retry logic from DeviceSettingsService A benefit of removing policy validation against system time is that there are no transient policy validation errors anymore and thus the retry logic in DeviceSettingsService can be scrapped. Also removing the unused STORE_POLICY_ERROR status code. BUG= 701045 Review-Url: https://codereview.chromium.org/2922453002 Cr-Commit-Position: refs/heads/master@{#476611} [modify] https://crrev.com/efffaf05f0a47a357ab6955c0a3caf7a4a06ef65/chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc [modify] https://crrev.com/efffaf05f0a47a357ab6955c0a3caf7a4a06ef65/chrome/browser/chromeos/settings/device_settings_provider.cc [modify] https://crrev.com/efffaf05f0a47a357ab6955c0a3caf7a4a06ef65/chrome/browser/chromeos/settings/device_settings_service.cc [modify] https://crrev.com/efffaf05f0a47a357ab6955c0a3caf7a4a06ef65/chrome/browser/chromeos/settings/device_settings_service.h [modify] https://crrev.com/efffaf05f0a47a357ab6955c0a3caf7a4a06ef65/chrome/browser/chromeos/settings/session_manager_operation.cc
,
Jun 2 2017
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by emaxx@chromium.org
, Apr 11 2017