Enterprise policy not working for UnsafelyTreatInsecureOriginAsSecure |
|||||||||||||||||
Issue descriptionChrome Version: 68+ OS: Win/Mac/Linux/ChromeOS What steps will reproduce the problem? (1) Set an enterprise policy for UnsafelyTreatInsecureOriginAsSecure (not the command line flag) for http://example.com (2) Load Chrome and visit http://example.com (3) "Not Secure" chip is shown in the omnibox What is the expected result? The UnsafelyTreatInsecureOriginAsSecure enterprise policy is supposed to disable showing the "Not Secure" UI for the listed origins. What happens instead? The enterprise policy sets Prefs::kUnsafelyTreatInsecureOriginAsSecure, which gets turned into a command-line switch in https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_client.cc?l=1954 but _only for the renderer process_ (see the surrounding if {} block). This means that the enterprise policy correctly causes the origins to be treated as secure contexts in Blink (you can open the devtools console and check window.isSecureContext) but it does not apply correctly in the browser process (where security indicator UI decisions are made). This flag is what is used by secure_origin_whitelist::GetWhitelist(), which is called from content::IsOriginSecure() (which is what is passed into the security_state component as a callback to check the origin when getting the SecurityLevel and other related logic). There does not seem to be a simple way to make the enterprise policy/pref convert to a browser-process command-line switch -- the switch adding logic in chrome_content_browser_client.cc only applies to child processes only. It may be easiest to make a fix that adds a custom IsOriginSecureCallback to SecurityStateTabHelper (which has the current BrowserContext/Profile and can look up prefs). This should be sufficient to handle the security indicator UI cases. However, I don't think this would help some other chrome/ consumers of content::IsOriginSecure (e.g., PaymentRequest). The total number of places that use/pass content::IsOriginSecure in the code is fairly small though, so they could potentially all be made policy/pref aware, but this seems lower priority for now.
,
Jul 31
,
Jul 31
We have not shipped 68 stable for CrOS yet, should this hold the release? If not can we drop the blocking label? We plan to build tonight, is there a CL we can merge in now? Raising priority as this may further delay the CrOS 68 stable release.
,
Jul 31
I have a CL going through review now: https://chromium-review.googlesource.com/c/chromium/src/+/1157029 It should be fairly safe to merge, but would still likely benefit from at least one canary before merging. Let me double check whether this is needed on ChromeOS and I'll give another update.
,
Jul 31
ATM, not hearing much on this issue from Ent. customers - RBS with understanding we'll still see this merged down in 68 refresh
,
Jul 31
It does not look like the old policy name "UnsafelyTreatInsecureOriginAsSecure" was ever supported on ChromeOS (for better or worse). The policy template never included a chrome_os entry in the supported_on key (despite some of our labeling and documentation saying otherwise). Our external documentation at https://www.chromium.org/administrators/policy-list-3#UnsafelyTreatInsecureOriginAsSecure _does_ correctly state this is only for Windows/Mac/Linux though. The new name for the policy (OverrideSecurityRestrictionsOnInsecureOrigin) landed in https://chromium-review.googlesource.com/c/chromium/src/+/1107849 which also enables the policy on ChromeOS and Android, but that new policy is only for M69+. I'll leave the ChromeOS bit set because the followup policy has the same bug (as it is just an alias for the old policy, updated to also include ChromeOS and Android). But it does not affect ChromeOS M68.
,
Jul 31
Actually Chrome OS is a bit moot of a point if our admin console doesn't support setting the policy which to my knowledge it doesn't.
,
Aug 1
This one is an issue primarily on Win / Mac. We've received many more customer inquiries about this through our browser specialist team.
,
Aug 1
Currently my customers testing this policy are on Chrome browser on Windows. I do believe that this policy will come up with ChromeOS customers that use HTTP internally hosted apps. I believe this policy will be be useful to have in the console when HTTP sites will be marked in red as not secure.
,
Aug 1
#8: Thanks for the update on impact. I'll get the CL landed as soon as possible and then we will coordinate a merge to be included the next M68 release and M69 beta. #9: The policy UnsafelyTreatInsecureOriginAsSecure [1] is only available on Windows/Mac/Linux. My understanding is that the new policy OverrideSecurityRestrictionsOnInsecureOrigin will be available on all non-iOS platforms in M69+. I don't know the details on how that is/will be surfaced in the Chrome Admin Console. [1] https://www.chromium.org/administrators/policy-list-3#UnsafelyTreatInsecureOriginAsSecure [2] https://chromium-review.googlesource.com/c/chromium/src/+/1107849
,
Aug 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/af855dc85932a14378771b13363d01c10ba1e5b1 commit af855dc85932a14378771b13363d01c10ba1e5b1 Author: Christopher Thompson <cthomp@chromium.org> Date: Wed Aug 01 16:43:12 2018 Fix for UnsafelyTreatInsecureOriginAsSecure policy in browser UI This CL updates secure_origin_whitelist::GetWhitelist() to delegate parsing the set of whitelisted sites to a new function, ParseWhitelist, which takes in a string and returns the parsed vector of strings. This allows callers in the browser process to explicitly parse a whitelist from either prefs or command-line switches. This allows SecurityStateTabHelper to have its own custom IsOriginSecureWithWhitelist function to which it can bind an explicitly passed whitelist of origins, rather than just using content::IsOriginSecure as the callback to security_state functions. content::IsOriginSecure uses GetWhitelist() which only loads the whitelist from command-line flags, which are only correctly set for renderer processes. The custom callback for SecurityStateTabHelper allows it to also check prefs for the whitelist, which is how the enterprise policy is accessible in the browser process (where security indicator UI logic occurs). This can cause the pref and the switch to be two different sources of truth for the origin whitelist, however this simpler fix will be easier to backport. This fix favors the switch over the pref if both are set, allowing developers to still set temporary overrides while maintaining the policy behavior for general users. More general fixes may involve changing how the whitelist propagates between parts of Chrome (including in content and blink). Bug: 869422 Change-Id: I93b46d66844af8cee00d919537ce66fc2c56cd46 Reviewed-on: https://chromium-review.googlesource.com/1157029 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Mustafa Emre Acer <meacer@chromium.org> Commit-Queue: Christopher Thompson <cthomp@chromium.org> Cr-Commit-Position: refs/heads/master@{#579835} [modify] https://crrev.com/af855dc85932a14378771b13363d01c10ba1e5b1/chrome/browser/secure_origin_whitelist_browsertest.cc [modify] https://crrev.com/af855dc85932a14378771b13363d01c10ba1e5b1/chrome/browser/ssl/security_state_tab_helper.cc [modify] https://crrev.com/af855dc85932a14378771b13363d01c10ba1e5b1/chrome/browser/ssl/security_state_tab_helper.h [modify] https://crrev.com/af855dc85932a14378771b13363d01c10ba1e5b1/chrome/common/secure_origin_whitelist.cc [modify] https://crrev.com/af855dc85932a14378771b13363d01c10ba1e5b1/chrome/common/secure_origin_whitelist.h
,
Aug 1
,
Aug 1
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 1
Removing OS=Chrome since it isn't relevant to the merge requests. The fix for this should be safe to merge, but I think it should go out in at least one Canary release before doing so. I'm not sure when a new M68 release is being cut though.
,
Aug 1
We just respun M68 yesterday for Desktop and there are no respins planned. Why is this issue critical? Why is this P0? Can we wait until M69 for the fix?
,
Aug 1
The practical impact of this bug is that there are enterprise customers that will skip M68 entirely if this is not working.
,
Aug 1
Are there workaround solutions that can be provided to impacted enterprises? How many enterprise customers does this impact? It's unclear to me how enterprises will skip M68? Seems like the impact is that sites that enterprises want to disable showing "Not Secure" UI in Omnibox will show "Not Secure". Is this really a P0?
,
Aug 1
I'm not sure of the overall scope of the impact and how we should prioritize this. This was a specific request from the Enterprise team for M68. privard@ Could you provide more details on the impact here?
,
Aug 1
Chatted with emilyschechter@ offline - our plan is to go ahead with ramp-up for M68. Once the change has baked in Canary, we can merge this to M68. And then if there is a respin, we can include it.
,
Aug 2
,
Aug 2
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 2
,
Aug 2
I've verified that the fix works in today's Canary on Windows. I'll get the merge to M69 done today.
,
Aug 2
Pls merge your change to M69 branch 3497 before 3:00 PM PT today if possible. Thank you.
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6df094c4fd2b47351bffec149833b4f4620ec653 commit 6df094c4fd2b47351bffec149833b4f4620ec653 Author: Christopher Thompson <cthomp@chromium.org> Date: Thu Aug 02 19:59:33 2018 Fix for UnsafelyTreatInsecureOriginAsSecure policy in browser UI [Merge to M68] This CL updates secure_origin_whitelist::GetWhitelist() to delegate parsing the set of whitelisted sites to a new function, ParseWhitelist, which takes in a string and returns the parsed vector of strings. This allows callers in the browser process to explicitly parse a whitelist from either prefs or command-line switches. This allows SecurityStateTabHelper to have its own custom IsOriginSecureWithWhitelist function to which it can bind an explicitly passed whitelist of origins, rather than just using content::IsOriginSecure as the callback to security_state functions. content::IsOriginSecure uses GetWhitelist() which only loads the whitelist from command-line flags, which are only correctly set for renderer processes. The custom callback for SecurityStateTabHelper allows it to also check prefs for the whitelist, which is how the enterprise policy is accessible in the browser process (where security indicator UI logic occurs). This can cause the pref and the switch to be two different sources of truth for the origin whitelist, however this simpler fix will be easier to backport. This fix favors the switch over the pref if both are set, allowing developers to still set temporary overrides while maintaining the policy behavior for general users. More general fixes may involve changing how the whitelist propagates between parts of Chrome (including in content and blink). Bug: 869422 Change-Id: I93b46d66844af8cee00d919537ce66fc2c56cd46 Reviewed-on: https://chromium-review.googlesource.com/1157029 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Mustafa Emre Acer <meacer@chromium.org> Commit-Queue: Christopher Thompson <cthomp@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#579835}(cherry picked from commit af855dc85932a14378771b13363d01c10ba1e5b1) Reviewed-on: https://chromium-review.googlesource.com/1160942 Reviewed-by: Christopher Thompson <cthomp@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#348} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/6df094c4fd2b47351bffec149833b4f4620ec653/chrome/browser/secure_origin_whitelist_browsertest.cc [modify] https://crrev.com/6df094c4fd2b47351bffec149833b4f4620ec653/chrome/browser/ssl/security_state_tab_helper.cc [modify] https://crrev.com/6df094c4fd2b47351bffec149833b4f4620ec653/chrome/browser/ssl/security_state_tab_helper.h [modify] https://crrev.com/6df094c4fd2b47351bffec149833b4f4620ec653/chrome/common/secure_origin_whitelist.cc [modify] https://crrev.com/6df094c4fd2b47351bffec149833b4f4620ec653/chrome/common/secure_origin_whitelist.h
,
Aug 2
I'm assuming that the chat with emilyschechter@ already resolved #c18. I can confirm that several customers have gotten in touch with us about this issue -- and it is blocking the update to M68 from their point of view.
,
Aug 2
privard@ - can you please provide more context on why specifically this is blocking update to 68 from their perspective? How many customers?
,
Aug 2
Chatted with privard@ offline. We will plan for a respin next week, with this fix. Approving merge to M68, branch:3440. Please verify and ensure it merges cleanly to M68. We will also need a postmortem for this.
,
Aug 2
+gorgesak@chromium.org, robertshield@chromium.org & yanglee@ for visibility. PTAL comment #28. Thank you.
,
Aug 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e3016ee055ec7f749ad9e5b60524bc63f2bd2285 commit e3016ee055ec7f749ad9e5b60524bc63f2bd2285 Author: Christopher Thompson <cthomp@chromium.org> Date: Fri Aug 03 17:51:53 2018 [M68] Fix for UnsafelyTreatInsecureOriginAsSecure policy in browser UI This CL updates secure_origin_whitelist::GetWhitelist() to delegate parsing the set of whitelisted sites to a new function, ParseWhitelist, which takes in a string and returns the parsed vector of strings. This allows callers in the browser process to explicitly parse a whitelist from either prefs or command-line switches. This allows SecurityStateTabHelper to have its own custom IsOriginSecureWithWhitelist function to which it can bind an explicitly passed whitelist of origins, rather than just using content::IsOriginSecure as the callback to security_state functions. content::IsOriginSecure uses GetWhitelist() which only loads the whitelist from command-line flags, which are only correctly set for renderer processes. The custom callback for SecurityStateTabHelper allows it to also check prefs for the whitelist, which is how the enterprise policy is accessible in the browser process (where security indicator UI logic occurs). This can cause the pref and the switch to be two different sources of truth for the origin whitelist, however this simpler fix will be easier to backport. This fix favors the switch over the pref if both are set, allowing developers to still set temporary overrides while maintaining the policy behavior for general users. More general fixes may involve changing how the whitelist propagates between parts of Chrome (including in content and blink). TBR=avi@chromium.org,meacer@chromium.org (cherry picked from commit af855dc85932a14378771b13363d01c10ba1e5b1) Bug: 869422 Change-Id: I93b46d66844af8cee00d919537ce66fc2c56cd46 Reviewed-on: https://chromium-review.googlesource.com/1157029 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Mustafa Emre Acer <meacer@chromium.org> Commit-Queue: Christopher Thompson <cthomp@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#579835} Reviewed-on: https://chromium-review.googlesource.com/1162436 Reviewed-by: Christopher Thompson <cthomp@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#781} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/e3016ee055ec7f749ad9e5b60524bc63f2bd2285/chrome/browser/secure_origin_whitelist_browsertest.cc [modify] https://crrev.com/e3016ee055ec7f749ad9e5b60524bc63f2bd2285/chrome/browser/ssl/security_state_tab_helper.cc [modify] https://crrev.com/e3016ee055ec7f749ad9e5b60524bc63f2bd2285/chrome/browser/ssl/security_state_tab_helper.h [modify] https://crrev.com/e3016ee055ec7f749ad9e5b60524bc63f2bd2285/chrome/common/secure_origin_whitelist.cc [modify] https://crrev.com/e3016ee055ec7f749ad9e5b60524bc63f2bd2285/chrome/common/secure_origin_whitelist.h
,
Aug 8
Verified this issue on Windows, Mac with chrome #68.0.3440.106, as per steps mentioned in the comment #0 and didn't observed the "Not Secure" chip in the omnibox for http://example.com site. Hence adding TE-Verified labels. Attaching the screen-cast for reference |
|||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by emilyschechter@chromium.org
, Jul 31