Thanks for the report!

Interesting, I was able to confirm this works. 

But I don't think much can be done to defend against attacks by a malicious user against the account he/she is using.
For that reason, I'm inclined to remove the security bug Type from this bug.

For more details see https://chromium.googlesource.com/chromium/src/+/lkcr/docs/security/faq.md#Why-arent-physically_local-attacks-in-Chromes-threat-model

jdufault@ what do you think? Does this seem like something worth the effort to protect against?
(feel free to reassign/unassign yourself from this bug if you aren't the right person for this kind of bug).

- Disabling screen lock is possible because that is just changing the pref value
- Changing PIN state (enabling, disabling, changing PIN value) does not work, as the underlying API requires password.

To fix this we will have to prevent the settings UI page from being allowed to write the "settings.enable_screen_lock" pref value and move it behind an API which checks for a password before changing the pref state.

There may be other UI surfaces though which have a similar API and can change prefs which would also need to be protected.

Alternatively we could move away from using a pref to store settings.enable_screen_lock - though at this moment I'm not sure what we would use.

I think this is worth fixing, but not a an especially high priority, because it seems pretty noticeable to the user and requires local access.

FYI, enable/disable screen lock did not used to be behind a password prompt. That was a new addition in md-settings after PIN was developed.

This should not be a low severity issue, changing priority to High. It should not be this trivial to disable a password on the device without actually knowing the password. It's pretty easy to snatch an unlocked Chromebook, at which point access could be retained indefinitely.

One possible fix is on the UI side, but ideally the backend setting should not be modifiable without knowing the password.

To confirm behavior for everyone:

- chrome settings does not let you disable the lock screen unless you type the password
- you can, if you know what you're doing, disable the lock screen using devtools
- lock screen is not on by default

Pre md-settings turning off lock screen was just a checkbox, no password required. So this is not a regression in any way.

What do you mean pre-md settings? The fact that the lockscreen is not on by default isn't relevant to this, this is about how users who opted into lockscreen security are affected.

And if the current settings requires you to enter a password, and this allows you to disable the lockscreen with a password, that's definitely a security bug that needs to be fixed.

> What do you mean pre-md settings? 

Settings was recently refreshed to follow material design conventions. Before the refresh, the settings page just had a top-level checkbox asking the user if they wanted to require the lock screen after suspending.

I agree this needs to be fixed, just giving some context/history to make sure everyone is on the same page. This is not a new regression.

Thanks for the info. I think the right path forward is to just fix the current UI bug, while I think of a separate and longer term fix to try and actually tie disabling the lockscreen to the user credentials.

jdufault: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

Any chance to fix this for 66 Jacob?

> Any chance to fix this for 66 Jacob?

I'll take a look soon, but it may not be in time for 66. If it needs to happen in 66 this should be reassigned to someone who works on/in settings.

Based on the current triage severity, we absolutely need this High-severity bug fixed for Chrome 66 as it's over 40 days old and at risk of blowing our deadlines.

Having said that, I'm skeptical that this is really even a security bug at all under Chrome's triage criteria: This is a physically-local attack.
https://chromium.googlesource.com/chromium/src/+/master/docs/security/faq.md#Why-arent-physically_local-attacks-in-Chromes-threat-model

This is a Chrome OS bug which has slightly different criteria. For example, Chrome OS has verified boot which defends against physically local attack to an extent. 

Given the optics around those macOS password bypasses, we absolutely need to fix this for our users in M-66. Who can own this? rkc@ can you find an owner?

stevenjb@, can you fix this? 

Hey Eric, those are Chrome browser guidelines, but on Chrome OS we're responsible for the entire machine, including the OS and the hardware, so we definitely consider physical attacks to be in-scope. Yes, physical presence is definitely an ameliorating factor, but by no means it results in this not being a security bug.

Greg and I agree that this can probably be dialed down to Medium.

This is not an area I am especially familiar with.

->sammiequon@

+abodenha@

Sammie, are you the right owner for this? If not we should assign back to Jacob, given that fixing in 67 is fine.

Assigning back to Jacob since he has some ideas how to fix this.

The NextAction date has arrived: 2018-02-21

M-66 is branching in a few weeks, is there any progress on this? We need to fix this for our users on a reasonable time frame.

Setting to M-67 per #21.

I'll defer to Jorge on that, but Jorge did you mean M-67 or M-66?

Given that this is not a regression (there was no way to do prevent this either before MD settings were enabled), I'd argue that M67 is fine. I'd rather give Jacob time to fix this in M67 (with the assumption that this should be high priority for 67) than try to rush things through in M66. That was my thinking.

Is this still targeted for M67?

rkc@ to find an owner, there is a good chance I will not have cycles for M67.

While I agree this is an important bug to fix, my team does not have the bandwidth to do this for 67. We are completely tapped out.

Assigning to Tom to assign to an engineer in the settings team.

As per comment #4, this isn't really a Settings bug. No UI only change can prevent a user from invoking the JS call that changes this setting.

The change needs to happen in the API and the change needs to be made by someone familiar with how login and lockscreen works.

rkc@ - I am sure your team is as stretched thin as everyone, but this is a developer resource prioritization issue and I think that you and Albert are best suited to determine if/when/how this will get resolved.

P.S. There is no CrOS knowledgeable/focused "settings team" at the moment :P

Shouldn't we just disable being able to inspect the settings page under dev-tools?

That is a pretty extreme solution. It breaks a lot of our ability to debug issues in the field. I'm also not sure how robust that is, but would have to defer to dpapad@ for that.

In general one should never rely on hidden or disabled UI for security, the security should be built into the model. Otherwise UI bugs (which tend to be more common due to higher velocity, and more difficult to test) can cause security vulnerabilities.

I agree with stevenjb@. Disabling dev-tools does not seem to be the right way of fixing such issues. C++ side checks should be guarding against anything the user should not be able to do. Just like opening the DevTools on an e-banking website should not allow you to make unauthorized transactions.

As a another data point, in the past there was a bug where a user could powerwash a chromebook from the Settings' DevTools as a guest user, which was fixed by modifying the C++ handlers. Also note that because this issue requires physical access to the device, is not considered very high severity (in my experience).

This is not very high severity. We'd really like to fix it in 67, that's
all.

I understand that everyone is tapped out, including the security team. But if we have no bandwidth to fix security bugs in our current product, then we are really losing touch with the 4 S's that made our product great. Having too many new features to develop cannot be a reason to let security bugs go unfixed.

rkc@, if you don't have resources should the security team escalate this so that we need to come up with engineering resources to maintain the current product?

Since my team is out of bandwidth and this overlaps with both settings and auth - maybe someone in the settings team can take this on? The 4 S's should be every team's responsibility, I presume?

Otherwise I can and will prioritize this for 68.

The expectation is that product teams are owners for the areas that they,
well, own. I'd argue that fixing security bugs is not really "optional", or
something you do when there's no other work available. The expectation is
that, just as the security team is available to consult on features
essentially at any time, because that's our role, fixing security bugs in
the code that one owns is one's job, especially with a very targeted fix
like for this bug.

Rahul, should we escalate this to Zel for an M67 fix? I think that was the
crux of Greg's comment: how do we move forward? We'd like to have this
fixed on 67.

I understood the crux of Greg's comment and I did reply to him. Settings aren't my team's responsibility and hence hardening them against physical attacks isn't either. There *is* a Chrome settings team, that should ideally be taking this on. I understand if they don't have bandwidth - in which case, I am willing for us to fix this; but when we can.

Is there a specific reason this really needs to go into M-67, then please do let me know and I will prioritize this for 67. Otherwise from my perspective, this is an important fix and I will get it done in 68.


"We want this in 67" is not a reason.

(not sure how parisa got removed, adding her back)

I think the reason why we would like to fix this in 67 is because we punted
it from 66 for the same "cycles" reason that we're using to punt it from
67. Given that we punted it from 66 originally, the expectation was that
this would make it into the prioritized work for 67.

Though not ideal, multiple punts do happen at times - 67 has turned out to be an exceptionally harsh milestone for my team. If you feel very strongly that this should go into 67, I'll find someone to work on it.

Just to be clear, we have guidelines on when bugs need to be fixed by priority: https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md#Medium-severityf

Medium issues should be fixed in the current milestone and merged if possible, so we've already let this go way beyond our general guidelines because of mitigating factors and lack of bandwidth. We can let this slip no further; we committed to fixing this in M-67 and our official security guidelines specify that this be done.

Sorry to chime in late on this. This got lost in the hundreds of other bugs I'm currently CCed on.

Having password protected security settings protected by a pref seems like an unwise situation but doing a substantial redesign of how that works in time for 67 given our current hard deadlines around 67 and 69 isn't going to work.

FWIW there ISN'T anyone currently in a good place to own Chrome OS settings which is why we're in this pickle. I'm hoping to resolve this soon

rkc@ let's chat about this today and see if we can come up with a simple mitigation that we can apply in 67. Hopefully something we can give to someone other than your team to implement.

I agree. The right thing to do is apply a simple mitigation and then lay out the longer term fix so that we can plan engineering cycles for it.

FWIW I did a little digging and this low grade vulnerability has existed since we introduced the pref in 2010: https://codereview.chromium.org/3532010/.

Which isn't to say we shouldn't still prioritize fixing this, but I can't think of a quick fix that would gain us much. I think we would be better off delaying another release cycle and finding someone to fix this properly (and audit the existing lock screen settings and api for potential vulnerabilities). Preferably someone already familiar with the lock screen code :)

Just to be clear, the deadlines are based on when we became aware of the issue, not when it was introduced.

I would prefer if abodenha@ and rkc@ chatted about a simple mitigation to apply in 67. Meanwhile, who will own designing and implementing a proper fix for this in a later milestone? 

I suspect disabling devtools will be the simplest mitigation. However, I do not expect fixing this bug to require a big design - here is what needs to happen:

- remove lockscreen pref whitelist from settings handler
- add new private API method that verifies password and sets the lockscreen pref value (quickUnlock private APIs can be used as inspiration, or we can add a method to it)
- settings is updated to use the private API

Re #50 I'm concerned that trying to lock down that pref solves the immediate issue but leaves us open to a general CLASS of issues caused by allowing dev tools to run on the OS's UI surfaces.

rkc@ and I chatted and I'm increasingly convinced that users should not be able to use dev tools to muck about with the OS itself while running in normal mode.

I appreciate that being able to run dev tools on our surfaces is useful to our devs though.

What I'm proposing is that we allow dev tools to run on webui in Chrome OS ONLY if the device is in dev mode. That should allow our devs to make use of the tools without them becoming an ongoing security risk.

Allowing users to bring up dev tools on system UI is akin to allowing a debugger to be connected. It's not something that should happen in normal mode.

As defense in depth, I think we should ALSO do the steps jdufault@ outlined in #50 in some later milestone.

Adding RBS since getting SOME mitigation in place should be considered a blocker for 67.

+zelidrag as FYI

achuith@ is there any chance you could dig into this?  I know it's outside your normal space, but I'm out of options.

Thanks for looking into this and driving some action.

#51: Disabling DevTools has no precedent so far in any of the WebUI pages AFAIK, so I am very reluctant concluding that this is the right way forward. Users opening the devtools and corrupting their profile is accepted (they do it on their own risk), but users opening DevTools and escaping security mechanisms is clearly an symptom of enforcing security in the UI, which is sub-optimal already.

I am looping in a few more people that might have more historical context on how such issues have been addressed in the past. Also being able to run DevTools in any release channel, is useful for debugging bugs on the wild (I've done this myself several times).

Also, there might be more short term fixes we could explore. Just off the top of my head, without having thought about it much.
1) Detect dialog deletion with MutationObserver and re-open it immediately.
2) Detect dialog deletion with MutationObserver, and navigate to about:blank, or display some error message.

Minor addition to my previous comment. Just for experimentation I tried the following:

1) Disable DevTools via policy https://www.chromium.org/administrators/policy-list-3#DeveloperToolsDisabled.
2) Launch chrome with --remote-debugging-port=9222
3) Launch a different Chrome instance (one without the policy)
4) Go to chrome://inspect/#devices
5) Succeeded in opening DevTools on the original Chrome instance.

I can still open DevTools this way, so does not seem 100% robust, even if that was the solution.

Re#54 I agree that security enforcement needs to ALSO be done at a lower level. Please keep in mind that that doing THAT only works because we don't allow random users to go mucking about with those bits.

The issue here is that the security considerations for the UI are different between a browser and an OS.  At the browser level if a user can use devtools to hack the local profile that's acceptable.  When they can use devtools to hack fundamental pieces of the OS UI that's a problem.

I'm not proposing restricting dev tools based on channel. Chrome OS offers dev MODE which exists precisely to allow users to do developer level stuff with their devices that isn't safe to allow in normal mode. This is completely independent of channel.  See https://www.chromium.org/chromium-os/poking-around-your-chrome-os-device

Re#54 The process you describe is not possible in normal mode in Chrome OS.

I meant to say 55 not 54.

I'd actually say that that the process described in #55 sounds like a security bug in itself if a user-specified command line can be used to circumvent policy.

I am going to fix this specific issue (basically as described in comment #50) for the reasons expressed in comment #55 and because I am convinced that it will be considerably less work than actually disabling Dev Tools just for chrome://settings (which I suspect will be much less straightforward than it sounds in order to be robust and well tested).

Re#59: I filed it as issue 822088.

Re#56
> This is completely independent of channel

Good to know. I thought Dev Mode also forces Dev channel.

> At the browser level if a user can use devtools to hack the local profile that's acceptable.

The browser still has similar concerns. For example there are many enterprise policies that result to showing disabled UI elements (checkboxes, dropdowns, buttons). The user can enable those buttons via DevTools, but they should still not be able to escape what a policy dictates.

If that is not the case, that would be a security bug also.

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

commit 4867a12b680de17381bb68044ad3b01dbf3ce301
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Tue Mar 20 17:20:54 2018

Add getAuthToken to quickUnlockPrivate

This improves the API for setting quick unlock properties and
prepares the API to include get/setLockEnabled.

Bug:  799711 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I7c1b3dba750063b8104135b63ff03041b19e8119
Reviewed-on: https://chromium-review.googlesource.com/965396
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544419}
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/chromeos/login/quick_unlock/quick_unlock_storage.cc
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/chromeos/login/quick_unlock/quick_unlock_storage.h
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/chromeos/login/quick_unlock/quick_unlock_storage_unittest.cc
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/resources/settings/people_page/compiled_resources2.gyp
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/resources/settings/people_page/lock_screen.js
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/resources/settings/people_page/lock_state_behavior.js
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/resources/settings/people_page/password_prompt_dialog.html
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/resources/settings/people_page/password_prompt_dialog.js
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/browser/resources/settings/people_page/setup_pin_dialog.js
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/common/extensions/api/quick_unlock_private.idl
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/test/data/webui/settings/fake_quick_unlock_private.js
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/chrome/test/data/webui/settings/quick_unlock_authenticate_browsertest_chromeos.js
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/extensions/browser/extension_function_histogram_value.h
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/third_party/closure_compiler/externs/quick_unlock_private.js
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/third_party/closure_compiler/interfaces/quick_unlock_private_interface.js
[modify] https://crrev.com/4867a12b680de17381bb68044ad3b01dbf3ce301/tools/metrics/histograms/enums.xml

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

commit eeaf77b7bdf55de7e1b0270042853fe2cead7d8f
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Tue Mar 20 20:32:19 2018

Add setLockScreenEnabled to quickUnlockPrivate

This CL:
* Makes kEnableAutoScreenLock read-only in settingsPrivate
* Adds setLockScreenEnabled to quickUnlockPrivate, requiring a valid
  authentication token.

Bug:  799711 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I1913aa52559d9eeacf7697c2bc7814e994f3ea3e
Reviewed-on: https://chromium-review.googlesource.com/967321
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544508}
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/browser/extensions/api/settings_private/prefs_util.cc
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/browser/resources/settings/people_page/compiled_resources2.gyp
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/browser/resources/settings/people_page/lock_screen.html
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/browser/resources/settings/people_page/lock_screen.js
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/browser/resources/settings/people_page/lock_state_behavior.js
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/browser/resources/settings/people_page/password_prompt_dialog.html
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/browser/resources/settings/people_page/password_prompt_dialog.js
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/common/extensions/api/quick_unlock_private.idl
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/test/data/webui/settings/fake_quick_unlock_private.js
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/chrome/test/data/webui/settings/quick_unlock_authenticate_browsertest_chromeos.js
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/extensions/browser/extension_function_histogram_value.h
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/third_party/closure_compiler/externs/quick_unlock_private.js
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/third_party/closure_compiler/interfaces/quick_unlock_private_interface.js
[modify] https://crrev.com/eeaf77b7bdf55de7e1b0270042853fe2cead7d8f/tools/metrics/histograms/enums.xml

Fixed in ToT. We should verify once this makes it to canary.

Thanks Steven for the quick fix. I do agree that disabling DevTools is likely a way to paper over the bug and leave unexpected avenues open.

We are dealing with a similar (i.e. dev tools required) bug and I think the conclusion is that hardening the underlying API is the robust way.

This also might be a rewardable report under our new guidelines.

*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Greetings!  Thanks for the report. The VRP panel took a look and decided to award $500 for this, noting that it's classed as low severity per https://chromium.googlesource.com/chromiumos/docs/+/master/security_severity_guidelines.md

Also, how would you like to be credited in release notes? Cheers!

Hi,

Thank you so much for this, I'm delighted. Please could I be credited as Chris Menon.
Thanks again,

Chris

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