Correcting branding confusion in OSCrypt for linux |
|||||||||
Issue descriptionWhat happened: - In M53, https://codereview.chromium.org/1973483002 introduced os_crypt in Linux (Gnome+libsecret), followed by https://codereview.chromium.org/2150543002 in M54 for KDE. Before that there was no encryption on Linux, all was stored unencrypted (or encrypted with a hard-coded password). After this change, Chrome on Gnome or KDE started storing the encryption key in Gnome Keyring / KWallet, under one of two possible names. The names differed in "Chrome" vs. "Chromium", but by mistake did not correspond to the branding but to optimisations (optimisations on = Chrome, off = Chromium). In case of optimised non-Chrome build this resulted in a confusing name, but all has worked well. - In M54, https://codereview.chromium.org/2264163002 attempted to fix the choice of names and base it on branding instead of optimisations. As a result, users of optimised non-Chrome builds under KDE effectively lose access to the encrypted part of their profile data from M53 in M54. (Luckily, for Gnome+libsecret the name is not used for lookup, so those are not affected.) Who is affected: Users of optimised non-Chrome M54 builds on Linux with KDE, in particular vanilla Chromium. It is unclear if other distributions of Chrome, including projects like Opera, are affected -- this depends on how they patch they changes over Chromium. As comment #5 below shows, we failed to find a Chromium distribution affected by this. What data is affected: Notably not passwords, because password storage has not been switched over to use os_crypt. Passwords have been and still are stored directly in Keyring or KWallet, and are unchanged. Other profile data use os_crypt: a quick look at the code-base yields cookie storage, credit card numbers, and even sync. The affected users in M54 will lose access to that data. (We did not investigate if syncing can save them, but it is reasonable to assume that a significant part of those users don't sync.) Workaround: OS tools like kwalletmanager for KWallet allow the user to copy out the key as well as to rename entries. If the user is aware of this issue before updating to Chrome 54 containing https://codereview.chromium.org/2264163002, they can try to create a duplicate entry with the name changed from Chrome to Chromium and the same key. Or they can try to move the key from the Chrome entry to the Chromium one after the update. Possible next steps: - "Lossy" option: we can ignore the affected Chromium users, and keep https://codereview.chromium.org/2264163002 in. Risk: data loss for the affected users who have been using M54 non-Chrome with optimisations Cost: none, no migration code bloating the code base - "Graceful" option: we can revert https://codereview.chromium.org/2264163002 and re-do that with proper migration in M55. Risk: very low, assuming that no distribution suddenly toggles their optimisation setting during versions 54-55. Users who run optimised and non-optimised Chromium builds side-by-side will still lose data on the former. Cost: some low maintenance coming from the additional ~150 lines of migration code. Based on the discussion below, thestig@ and vabr@ decided to go with "Lossy", as the risks involved were really low, and the maintenance burden zero.
,
Aug 24 2016
,
Aug 24 2016
The following revision should have referred to this bug: https://chromium.googlesource.com/chromium/src.git/+/c0bfd647f33d33086f7c3643bc4d37e0b72f4e2a commit c0bfd647f33d33086f7c3643bc4d37e0b72f4e2a Author: thestig <thestig@chromium.org> Date: Mon Aug 22 18:10:35 2016 Fix branding/official build confusion in os_crypt. TBR=cfroussios@chromium.org Review-Url: https://codereview.chromium.org/2264163002 Cr-Commit-Position: refs/heads/master@{#413477} [modify] https://crrev.com/c0bfd647f33d33086f7c3643bc4d37e0b72f4e2a/components/os_crypt/key_storage_linux.cc
,
Aug 24 2016
I am requesting a merge of r413477 into M53 (branch 2785). The change affects only a small number of users on Linux using KDE and running either a non-Chrome branded build with optimisation (GOOGLE_CHROME_BUILD off, OFFICIAL_BUILD on) or Chrome build without optimisation (GOOGLE_CHROME_BUILD on, OFFICIAL_BUILD off). These users could lose profile data without the merge. Other users are not affected.
,
Aug 24 2016
I don't know of any distro that, as of this writing, distributes Chromium 53 with the official build set. So affected users are going to be ones who run KDE and built Chromium 53.x (or early 54.x) themselves with the official build flag set. Thus I believe the number of affected users is extremely small and we should just go with the "lossy" option. Thus we should just merge to M53 and call it a day.
,
Aug 24 2016
(I reported this on the CL.) I build Chromium myself, and noticed Chrome (rather than Chromium) in seahorse. I'm on Ubuntu 14.04.2 LTS and Chromium 53.0.2785.70. I have not modified any build flags related to this, so I think this affects non-KDE users too. I don't know for sure, but I think distributions don't set is_official_build precisely for the reason which caused this confusion in the first place: assuming it's related to Chrome, which it isn't. Just for reference, the official definition. # Set to enable the official build level of optimization. This has nothing # to do with branding, but enables an additional level of optimization above # release (!is_debug). This might be better expressed as a tri-state # (debug, release, official) but for historical reasons there are two # separate flags. is_official_build = false https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?l=126
,
Aug 24 2016
re: comment 6 - Yes, we understand the confusion and we were hit by it as well. Well, it was really an oversight on my part in the code review. I hope you understand our reasoning for going the "lossy" route and that you may have to manually rename some password stores.
,
Aug 24 2016
Thanks for the updates. While the string changed in Gnome Keyring as well (that's what visible in seahorse) -- as cfroussios@ assured me, in the case of the Keyring, the name is not really relevant for looking up the key. The impact will stay with KDE, though.
,
Aug 24 2016
Before we approve merge to M53, Could you please confirm whether this change is well baked/verified in Canary and safe to merge? Please note that M53 is very close to Stable launch so bar is very high. We will take this change ONLY if it is important and safe. As per comment #4, the change affects only a small number of users on Linux so can it wait until M54?
,
Aug 24 2016
It's an one-line change and it does not actually affect Google Chrome at all. The change is for the benefit of Chromium users. It affects few users especially because the problem has not hit stable yet. If we don't merge, then it is slightly more likely Chromium users will hit this problem.
,
Aug 24 2016
+1 to comment #10. If we don't merge, we will likely end up having to implement a more complex solution for 54 or 55, and maintain it for some time (months to years).
,
Aug 24 2016
Sorry, forgot to answer the answer about baking in: yes, the change is even in Dev and has not caused any havoc so far.
,
Aug 24 2016
Approving merge to M53 branch 2785 based on comment #10, #11 and #12.Please merge ASAP. Thank you.
,
Aug 24 2016
It's not a straight forward merge, but https://codereview.chromium.org/2272143002/ does the same thing and everything we said so far still applies.
,
Aug 24 2016
+kerz@ as FYI
,
Aug 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a4db545c23e1675c7e94f06b01165b648472b93 commit 9a4db545c23e1675c7e94f06b01165b648472b93 Author: Lei Zhang <thestig@chromium.org> Date: Wed Aug 24 20:32:29 2016 M53: Fix branding/official build confusion in os_crypt. This is the spiritial backport of https://codereview.chromium.org/2264163002 BUG= 640603 R=vabr@chromium.org Review URL: https://codereview.chromium.org/2272143002 . Cr-Commit-Position: refs/branch-heads/2785@{#741} Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382} [modify] https://crrev.com/9a4db545c23e1675c7e94f06b01165b648472b93/components/os_crypt/key_storage_libsecret.cc
,
Aug 24 2016
,
Aug 25 2016
,
Nov 29
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by vabr@chromium.org
, Aug 24 2016