Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931020:
AutoFill credit cards should be obfuscated on Linux (Closed)
|
Created:
10 years, 5 months ago by dhollowa Modified:
9 years, 7 months ago CC:
chromium-reviews, Paweł Hajdan Jr., ben+cc_chromium.org Visibility:
Public. |
DescriptionAutoFill credit cards should be obfuscated on Linux
AutoFill credit card information now gets stored in obfuscated form on disc.
BUG=42038, 49115
TEST=EncryptorTest.CypherTextDiffers, EncryptorTest.DecryptError
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52596
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressing review comments. #Patch Set 3 : Addressing review comments. #
Total comments: 2
Patch Set 4 : Adding additional bug to comments. #Patch Set 5 : Reducing keygen iterations to 1. #Patch Set 6 : Removing Mac headers. #Patch Set 7 : Adding versioning information to cyphertext. #
Total comments: 2
Patch Set 8 : Data migration of clear text. #
Total comments: 13
Patch Set 9 : Addressing review comments. #Patch Set 10 : Merge with trunk and Mac changes. #
Messages
Total messages: 28 (0 generated)
Decision for Linux M6 is to obfuscate. Protected key-store come in M7.
+mdm
+agl, crypto master http://codereview.chromium.org/2931020/diff/1/2 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/1/2#newcode17 chrome/browser/password_manager/encryptor_linux.cc:17: const char kSalt[] = "saltysalt"; I believe the correct way to use salt is on a per-pw basis.
http://codereview.chromium.org/2931020/diff/1/2 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/1/2#newcode11 chrome/browser/password_manager/encryptor_linux.cc:11: #include "base/scoped_ptr.h" scoped_ptr should go before utf_str... http://codereview.chromium.org/2931020/diff/1/2#newcode17 chrome/browser/password_manager/encryptor_linux.cc:17: const char kSalt[] = "saltysalt"; Comment this section. http://codereview.chromium.org/2931020/diff/1/2#newcode26 chrome/browser/password_manager/encryptor_linux.cc:26: // http://code.google.com/p/chromium/issues/detail?id=25404 nit: I prefer the short http://crbug.com/25404 http://codereview.chromium.org/2931020/diff/1/2#newcode61 chrome/browser/password_manager/encryptor_linux.cc:61: // This doesn't actually encrypt, we need to work on the Encryptor API. I'd remove this first line of the comment, since it's technically not true anymore. http://codereview.chromium.org/2931020/diff/1/2#newcode89 chrome/browser/password_manager/encryptor_linux.cc:89: // This doesn't actually decrypt, we need to work on the Encryptor API. Same here. http://codereview.chromium.org/2931020/diff/1/3 File chrome/browser/password_manager/encryptor_unittest.cc (right): http://codereview.chromium.org/2931020/diff/1/3#newcode20 chrome/browser/password_manager/encryptor_unittest.cc:20: //#if defined(OS_MACOSX) Intentionally commented out?
On 2010/07/14 22:15:20, Evan Martin wrote: > +agl, crypto master > > http://codereview.chromium.org/2931020/diff/1/2 > File chrome/browser/password_manager/encryptor_linux.cc (right): > > http://codereview.chromium.org/2931020/diff/1/2#newcode17 > chrome/browser/password_manager/encryptor_linux.cc:17: const char kSalt[] = > "saltysalt"; > I believe the correct way to use salt is on a per-pw basis. Yes, but in this case we have single hard-coded password.
http://codereview.chromium.org/2931020/diff/1/2 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/1/2#newcode17 chrome/browser/password_manager/encryptor_linux.cc:17: const char kSalt[] = "saltysalt"; How about seasalt, or koshersalt? :) http://codereview.chromium.org/2931020/diff/1/2#newcode26 chrome/browser/password_manager/encryptor_linux.cc:26: // http://code.google.com/p/chromium/issues/detail?id=25404 I was planning on closing this bug soon, once the user-saved passwords are indeed stored in system-level stores by default. It seems like maybe this is a slightly different bug; could we open a new one for this instead? http://codereview.chromium.org/2931020/diff/1/3 File chrome/browser/password_manager/encryptor_unittest.cc (right): http://codereview.chromium.org/2931020/diff/1/3#newcode124 chrome/browser/password_manager/encryptor_unittest.cc:124: EXPECT_EQ(plaintext, "hel"); You mean result here, right?
http://codereview.chromium.org/2931020/diff/1/2 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/1/2#newcode11 chrome/browser/password_manager/encryptor_linux.cc:11: #include "base/scoped_ptr.h" On 2010/07/14 22:16:21, James Hawkins wrote: > scoped_ptr should go before utf_str... Done. http://codereview.chromium.org/2931020/diff/1/2#newcode17 chrome/browser/password_manager/encryptor_linux.cc:17: const char kSalt[] = "saltysalt"; On 2010/07/14 22:16:21, James Hawkins wrote: > Comment this section. Done. http://codereview.chromium.org/2931020/diff/1/2#newcode26 chrome/browser/password_manager/encryptor_linux.cc:26: // http://code.google.com/p/chromium/issues/detail?id=25404 On 2010/07/14 22:16:21, James Hawkins wrote: > nit: I prefer the short http://crbug.com/25404 Done. http://codereview.chromium.org/2931020/diff/1/2#newcode61 chrome/browser/password_manager/encryptor_linux.cc:61: // This doesn't actually encrypt, we need to work on the Encryptor API. On 2010/07/14 22:16:21, James Hawkins wrote: > I'd remove this first line of the comment, since it's technically not true > anymore. Done. http://codereview.chromium.org/2931020/diff/1/2#newcode89 chrome/browser/password_manager/encryptor_linux.cc:89: // This doesn't actually decrypt, we need to work on the Encryptor API. On 2010/07/14 22:16:21, James Hawkins wrote: > Same here. Done. http://codereview.chromium.org/2931020/diff/1/3 File chrome/browser/password_manager/encryptor_unittest.cc (right): http://codereview.chromium.org/2931020/diff/1/3#newcode20 chrome/browser/password_manager/encryptor_unittest.cc:20: //#if defined(OS_MACOSX) On 2010/07/14 22:16:21, James Hawkins wrote: > Intentionally commented out? Yes, this is temporary. Will merge with in-flight changes on Mac.
http://codereview.chromium.org/2931020/diff/1/2 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/1/2#newcode17 chrome/browser/password_manager/encryptor_linux.cc:17: const char kSalt[] = "saltysalt"; On 2010/07/14 22:15:20, Evan Martin wrote: > I believe the correct way to use salt is on a per-pw basis. This is simply a slow way to embed a password in the source code. Thus, there is no salt. I figure it's just here to give the PRF something to work with. (Is there really no way to just say uint8_t key[] = {1, 2, 3, ...}?)
http://codereview.chromium.org/2931020/diff/1/2 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/1/2#newcode17 chrome/browser/password_manager/encryptor_linux.cc:17: const char kSalt[] = "saltysalt"; On 2010/07/14 22:29:10, Mike Mammarella wrote: > How about seasalt, or koshersalt? :) Or saltypeanuts to match the password? http://codereview.chromium.org/2931020/diff/1/2#newcode26 chrome/browser/password_manager/encryptor_linux.cc:26: // http://code.google.com/p/chromium/issues/detail?id=25404 On 2010/07/14 22:29:10, Mike Mammarella wrote: > I was planning on closing this bug soon, once the user-saved passwords are > indeed stored in system-level stores by default. It seems like maybe this is a > slightly different bug; could we open a new one for this instead? Done. http::/crbug.com/49115 http://codereview.chromium.org/2931020/diff/1/3 File chrome/browser/password_manager/encryptor_unittest.cc (right): http://codereview.chromium.org/2931020/diff/1/3#newcode124 chrome/browser/password_manager/encryptor_unittest.cc:124: EXPECT_EQ(plaintext, "hel"); On 2010/07/14 22:29:10, Mike Mammarella wrote: > You mean result here, right? Yes, thanks.
LGTM
On 2010/07/14 22:38:47, agl wrote: > http://codereview.chromium.org/2931020/diff/1/2 > File chrome/browser/password_manager/encryptor_linux.cc (right): > > http://codereview.chromium.org/2931020/diff/1/2#newcode17 > chrome/browser/password_manager/encryptor_linux.cc:17: const char kSalt[] = > "saltysalt"; > On 2010/07/14 22:15:20, Evan Martin wrote: > > I believe the correct way to use salt is on a per-pw basis. > > This is simply a slow way to embed a password in the source code. Thus, there is > no salt. I figure it's just here to give the PRF something to work with. > > (Is there really no way to just say uint8_t key[] = {1, 2, 3, ...}?) Encryptor::Init() takes a typed SymmetricKey... not sure how to get there from a raw uint8 sequence. Thoughts?
http://codereview.chromium.org/2931020/diff/13001/14001 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/13001/14001#newcode33 chrome/browser/password_manager/encryptor_linux.cc:33: // http://crbug.com/25404 Put the new bug number here, and below?
Oh, one other question - how do we handle migrating the currently-unencrypted data to be obfuscated, and how will we handle migrating that to be truly encrypted? Seems like we need some way to handle that.
http://codereview.chromium.org/2931020/diff/13001/14001 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/13001/14001#newcode33 chrome/browser/password_manager/encryptor_linux.cc:33: // http://crbug.com/25404 On 2010/07/14 22:53:24, Mike Mammarella wrote: > Put the new bug number here, and below? Done.
On 2010/07/14 22:55:22, Mike Mammarella wrote: > Oh, one other question - how do we handle migrating the currently-unencrypted > data to be obfuscated, and how will we handle migrating that to be truly > encrypted? Seems like we need some way to handle that. For current unencrypted data the decrypt will fail and user will lose any existing data. For AutoFill this is acceptable because we're not released. For stored passwords this might not be so acceptable. The only other client of the Encryptor is sync for storing transient connection info, so that should not be an issue. For future changes from obfuscated to encrypted, my thought was small data-loss (cc#s and passwords) seems a small price to pay for greater security. No?
On 2010/07/14 23:05:15, dhollowa wrote: > On 2010/07/14 22:55:22, Mike Mammarella wrote: > > Oh, one other question - how do we handle migrating the currently-unencrypted > > data to be obfuscated, and how will we handle migrating that to be truly > > encrypted? Seems like we need some way to handle that. > > For current unencrypted data the decrypt will fail and user will lose any > existing data. For AutoFill this is acceptable because we're not released. For > stored passwords this might not be so acceptable. The only other client of the > Encryptor is sync for storing transient connection info, so that should not be > an issue. > > For future changes from obfuscated to encrypted, my thought was small data-loss > (cc#s and passwords) seems a small price to pay for greater security. No? For passwords we're all set, I've been working on that and we migrate the existing passwords to the system store. If losing all the stored data is OK for this change, then that's fine if people agree on it. As for losing the data again when we put in real encryption, why not just put a version number or some other flag in now? E.g. prefix the ciphertext with "1" if it's just obfuscated, and later on we can check for that and switch to "2" with encrypted data at some point. (Alternately, see how PasswordStoreDefault deals with migrating passwords from the web data store. This would be better than prefixing, probably.)
On 2010/07/14 23:14:10, Mike Mammarella wrote: > On 2010/07/14 23:05:15, dhollowa wrote: > > On 2010/07/14 22:55:22, Mike Mammarella wrote: > > > Oh, one other question - how do we handle migrating the > currently-unencrypted > > > data to be obfuscated, and how will we handle migrating that to be truly > > > encrypted? Seems like we need some way to handle that. > > > > For current unencrypted data the decrypt will fail and user will lose any > > existing data. For AutoFill this is acceptable because we're not released. > For > > stored passwords this might not be so acceptable. The only other client of > the > > Encryptor is sync for storing transient connection info, so that should not be > > an issue. > > > > For future changes from obfuscated to encrypted, my thought was small > data-loss > > (cc#s and passwords) seems a small price to pay for greater security. No? > > For passwords we're all set, I've been working on that and we migrate the > existing passwords to the system store. If losing all the stored data is OK for > this change, then that's fine if people agree on it. > > As for losing the data again when we put in real encryption, why not just put a > version number or some other flag in now? E.g. prefix the ciphertext with "1" if > it's just obfuscated, and later on we can check for that and switch to "2" with > encrypted data at some point. (Alternately, see how PasswordStoreDefault deals > with migrating passwords from the web data store. This would be better than > prefixing, probably.) Using the version number and then +1 version combined with platform checks is ideal.
On 2010/07/14 22:52:01, dhollowa wrote: > On 2010/07/14 22:38:47, agl wrote: > > http://codereview.chromium.org/2931020/diff/1/2 > > File chrome/browser/password_manager/encryptor_linux.cc (right): > > > > http://codereview.chromium.org/2931020/diff/1/2#newcode17 > > chrome/browser/password_manager/encryptor_linux.cc:17: const char kSalt[] = > > "saltysalt"; > > On 2010/07/14 22:15:20, Evan Martin wrote: > > > I believe the correct way to use salt is on a per-pw basis. > > > > This is simply a slow way to embed a password in the source code. Thus, there > is > > no salt. I figure it's just here to give the PRF something to work with. > > > > (Is there really no way to just say uint8_t key[] = {1, 2, 3, ...}?) > > Encryptor::Init() takes a typed SymmetricKey... not sure how to get there from > a raw uint8 sequence. Thoughts? Reduced keygen iterations to 1. To address perf issues.
On 2010/07/14 23:22:53, James Hawkins wrote: > On 2010/07/14 23:14:10, Mike Mammarella wrote: > > On 2010/07/14 23:05:15, dhollowa wrote: > > > On 2010/07/14 22:55:22, Mike Mammarella wrote: > > > > Oh, one other question - how do we handle migrating the > > currently-unencrypted > > > > data to be obfuscated, and how will we handle migrating that to be truly > > > > encrypted? Seems like we need some way to handle that. > > > > > > For current unencrypted data the decrypt will fail and user will lose any > > > existing data. For AutoFill this is acceptable because we're not released. > > For > > > stored passwords this might not be so acceptable. The only other client of > > the > > > Encryptor is sync for storing transient connection info, so that should not > be > > > an issue. > > > > > > For future changes from obfuscated to encrypted, my thought was small > > data-loss > > > (cc#s and passwords) seems a small price to pay for greater security. No? > > > > For passwords we're all set, I've been working on that and we migrate the > > existing passwords to the system store. If losing all the stored data is OK > for > > this change, then that's fine if people agree on it. > > > > As for losing the data again when we put in real encryption, why not just put > a > > version number or some other flag in now? E.g. prefix the ciphertext with "1" > if > > it's just obfuscated, and later on we can check for that and switch to "2" > with > > encrypted data at some point. (Alternately, see how PasswordStoreDefault deals > > with migrating passwords from the web data store. This would be better than > > prefixing, probably.) > > Using the version number and then +1 version combined with platform checks is > ideal. Done. I've added "v10" as a prefix string to the cypher text.
LGTM http://codereview.chromium.org/2931020/diff/31001/32001 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/31001/32001#newcode113 chrome/browser/password_manager/encryptor_linux.cc:113: return false; A thought: if the prefix is not found, there's a very good chance that this is just an unobfuscated password and we could just return it as-is here to prevent data loss. (Of course, old data would remain unobfuscated until modified.)
On 2010/07/15 00:58:24, Mike Mammarella wrote: > LGTM > > http://codereview.chromium.org/2931020/diff/31001/32001 > File chrome/browser/password_manager/encryptor_linux.cc (right): > > http://codereview.chromium.org/2931020/diff/31001/32001#newcode113 > chrome/browser/password_manager/encryptor_linux.cc:113: return false; > A thought: if the prefix is not found, there's a very good chance that this is > just an unobfuscated password and we could just return it as-is here to prevent > data loss. (Of course, old data would remain unobfuscated until modified.) My preference is toward data-loss in this case... As a user I'd be happier to have the clear-text ccs gone from my drive. But that's just me.
On 2010/07/15 01:05:34, dhollowa wrote: > On 2010/07/15 00:58:24, Mike Mammarella wrote: > > LGTM > > > > http://codereview.chromium.org/2931020/diff/31001/32001 > > File chrome/browser/password_manager/encryptor_linux.cc (right): > > > > http://codereview.chromium.org/2931020/diff/31001/32001#newcode113 > > chrome/browser/password_manager/encryptor_linux.cc:113: return false; > > A thought: if the prefix is not found, there's a very good chance that this is > > just an unobfuscated password and we could just return it as-is here to > prevent > > data loss. (Of course, old data would remain unobfuscated until modified.) > > My preference is toward data-loss in this case... As a user I'd be happier to > have the clear-text ccs gone from my drive. But that's just me. Will they actually be deleted if they fail to decrypt? Or will they just become inaccessible but remain on disk?
On 2010/07/15 01:07:11, Mike Mammarella wrote: > On 2010/07/15 01:05:34, dhollowa wrote: > > On 2010/07/15 00:58:24, Mike Mammarella wrote: > > > LGTM > > > > > > http://codereview.chromium.org/2931020/diff/31001/32001 > > > File chrome/browser/password_manager/encryptor_linux.cc (right): > > > > > > http://codereview.chromium.org/2931020/diff/31001/32001#newcode113 > > > chrome/browser/password_manager/encryptor_linux.cc:113: return false; > > > A thought: if the prefix is not found, there's a very good chance that this > is > > > just an unobfuscated password and we could just return it as-is here to > > prevent > > > data loss. (Of course, old data would remain unobfuscated until modified.) > > > > My preference is toward data-loss in this case... As a user I'd be happier to > > have the clear-text ccs gone from my drive. But that's just me. > > Will they actually be deleted if they fail to decrypt? Or will they just become > inaccessible but remain on disk? Ah, good point! I'll make the change then.
http://codereview.chromium.org/2931020/diff/31001/32001 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/31001/32001#newcode113 chrome/browser/password_manager/encryptor_linux.cc:113: return false; On 2010/07/15 00:58:24, Mike Mammarella wrote: > A thought: if the prefix is not found, there's a very good chance that this is > just an unobfuscated password and we could just return it as-is here to prevent > data loss. (Of course, old data would remain unobfuscated until modified.) Done.
LGTM
http://codereview.chromium.org/2931020/diff/32003/39001 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/32003/39001#newcode32 chrome/browser/password_manager/encryptor_linux.cc:32: base::SymmetricKey* GetEncryptionKey() { Needs a function-level comment, which should mention that the client owns the returned key. http://codereview.chromium.org/2931020/diff/32003/39001#newcode41 chrome/browser/password_manager/encryptor_linux.cc:41: scoped_ptr<base::SymmetricKey> encryption_key( Why a scoped_ptr when the only possible path through the code is to release() it? http://codereview.chromium.org/2931020/diff/32003/39001#newcode74 chrome/browser/password_manager/encryptor_linux.cc:74: // http://crbug.com/25404 and http://crbug.com/49115 Why is this comment repeated here and below? Other than versioning prefix, nothing outside of GetEncryptionKey would have to change, right? http://codereview.chromium.org/2931020/diff/32003/39001#newcode112 chrome/browser/password_manager/encryptor_linux.cc:112: // old data saved as clear text and we'll return it directly. Maybe mention that the only case this should apply to in CC numbers, so we don't need to worry about false-positives? http://codereview.chromium.org/2931020/diff/32003/39002 File chrome/browser/password_manager/encryptor_unittest.cc (right): http://codereview.chromium.org/2931020/diff/32003/39002#newcode22 chrome/browser/password_manager/encryptor_unittest.cc:22: //#endif Remove this. http://codereview.chromium.org/2931020/diff/32003/39002#newcode48 chrome/browser/password_manager/encryptor_unittest.cc:48: // base::Encryptor::Crypt() on Mac. Isn't this part of your other change?
http://codereview.chromium.org/2931020/diff/32003/39001 File chrome/browser/password_manager/encryptor_linux.cc (right): http://codereview.chromium.org/2931020/diff/32003/39001#newcode32 chrome/browser/password_manager/encryptor_linux.cc:32: base::SymmetricKey* GetEncryptionKey() { On 2010/07/15 18:16:51, stuartmorgan wrote: > Needs a function-level comment, which should mention that the client owns the > returned key. Done. http://codereview.chromium.org/2931020/diff/32003/39001#newcode41 chrome/browser/password_manager/encryptor_linux.cc:41: scoped_ptr<base::SymmetricKey> encryption_key( On 2010/07/15 18:16:51, stuartmorgan wrote: > Why a scoped_ptr when the only possible path through the code is to release() > it? This is a defensive coding strategy. In case new code gets added that returns, we won't leak. http://codereview.chromium.org/2931020/diff/32003/39001#newcode74 chrome/browser/password_manager/encryptor_linux.cc:74: // http://crbug.com/25404 and http://crbug.com/49115 On 2010/07/15 18:16:51, stuartmorgan wrote: > Why is this comment repeated here and below? Other than versioning prefix, > nothing outside of GetEncryptionKey would have to change, right? I left these duplicate comments in for obvious-ness to a new reader who might not notice the comment in GetEncryptionKey(). http://codereview.chromium.org/2931020/diff/32003/39001#newcode112 chrome/browser/password_manager/encryptor_linux.cc:112: // old data saved as clear text and we'll return it directly. On 2010/07/15 18:16:51, stuartmorgan wrote: > Maybe mention that the only case this should apply to in CC numbers, so we don't > need to worry about false-positives? Done. http://codereview.chromium.org/2931020/diff/32003/39002 File chrome/browser/password_manager/encryptor_unittest.cc (right): http://codereview.chromium.org/2931020/diff/32003/39002#newcode22 chrome/browser/password_manager/encryptor_unittest.cc:22: //#endif On 2010/07/15 18:16:51, stuartmorgan wrote: > Remove this. Yes, not part of this change. Will land Mac changes first and then remove this. The only thing that will change in this file is removal of #ifdefs that are part of the Mac change. http://codereview.chromium.org/2931020/diff/32003/39002#newcode48 chrome/browser/password_manager/encryptor_unittest.cc:48: // base::Encryptor::Crypt() on Mac. On 2010/07/15 18:16:51, stuartmorgan wrote: > Isn't this part of your other change? Correct. Sorry for the confusion. Most of these changes will get landed in the Mac CL.
LGTM http://codereview.chromium.org/2931020/diff/32003/39002 File chrome/browser/password_manager/encryptor_unittest.cc (right): http://codereview.chromium.org/2931020/diff/32003/39002#newcode48 chrome/browser/password_manager/encryptor_unittest.cc:48: // base::Encryptor::Crypt() on Mac. On 2010/07/15 21:11:45, dhollowa wrote: > On 2010/07/15 18:16:51, stuartmorgan wrote: > > Isn't this part of your other change? > > Correct. Sorry for the confusion. Most of these changes will get landed in the > Mac CL. May I recommend git? ;) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
