Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2525593002:
PasswordReuseDetector implementation (Closed)
DescriptionPasswordReuseDetector implementation.
PasswordReuseDetector is class for detecting password reuse. It consumes passwords from PassworStore, builds password index and then it can be used for detecting of password reuse by calling method CheckReuse().
This class is not created anywhere in production code yet. It will be done in next CLs.
BUG=657041
Committed: https://crrev.com/696bedaf20944503cf73fc7a5d5cda65dc30e61f
Cr-Commit-Position: refs/heads/master@{#434975}
Patch Set 1 #Patch Set 2 : Comments #
Total comments: 19
Patch Set 3 : comments #
Total comments: 3
Patch Set 4 : Addressed comments #
Total comments: 2
Patch Set 5 : Addressed comments #
Total comments: 6
Patch Set 6 : Addressed comments #
Total comments: 1
Messages
Total messages: 33 (15 generated)
Description was changed from ========== PasswordReuseDetector implementation BUG=657041 ========== to ========== PasswordReuseDetector implementation. PasswordReuseDetector is class for detecting password reuse. It consumes passwords from PassworStore, builds password index and then it can be used for detecting of password reuse by calling method CheckReuse(). This class is not created anywhere in production call, it will be done in next CLs. BUG=657041 ==========
Description was changed from ========== PasswordReuseDetector implementation. PasswordReuseDetector is class for detecting password reuse. It consumes passwords from PassworStore, builds password index and then it can be used for detecting of password reuse by calling method CheckReuse(). This class is not created anywhere in production call, it will be done in next CLs. BUG=657041 ========== to ========== PasswordReuseDetector implementation. PasswordReuseDetector is class for detecting password reuse. It consumes passwords from PassworStore, builds password index and then it can be used for detecting of password reuse by calling method CheckReuse(). This class is not created anywhere in production code yet. It will be done in next CLs. BUG=657041 ==========
dvadym@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, Could you please review this CL? Regards, Vadym
Thanks, Vadym. I have one bigger concern (keeping the passwords in memory, see my very last comment) and some lesser ones. LGTM assuming my comments are resolved. Cheers, Vaclav https://codereview.chromium.org/2525593002/diff/20001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:13: int kMinPasswordLengthToCheck = 8; size_t, not int; also: please make it constexpr (or at least const). https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:39: const base::string16 input_suffix = input.substr(i); I wonder if it would make sense to pull |input_suffix| out of the for-cycle to avoid heap-allocation in every iteration (its contents is getting smaller, so the initial allocation would sustain all iterations). If you do so, please also comment on this being for performance reasons. Ideally we would spare also the copy each time and use StringPiece16 somehow, but for that we would likely need a custom key comparator (for std::string vs StringPiece16). Not sure if that's worth the effort. https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:47: if (domains.find(registry_controlled_domain) == domains.end()) { Do we really need to check all domains where the password is saved? Imagine "pwd" is saved on a.com and b.org. Also imagine we would only store a.com in passwords_["pwd"]. Now the user is typing it on b.org, where it is also saved, only PasswordReuseDector ignores that and flags that as a reuse. So what? It is a reuse, in fact: the password is used on two domains. If you agree, let's shrink the std::set in passwords_'s value to just std::string. https://codereview.chromium.org/2525593002/diff/20001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.h (right): https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:24: // |password| is typed password that was already saved. nit: I'm not sure what "typed" means here. It does not seem important whether the password was typed. If you drop this line and just keeps lines 23 and 25, the comment seems to be telling the same information already. https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:25: // |saved_domain| is domain on which |password| is saved. nit: domain -> the domain https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:44: // Checks that some suffix of |input| equals to the password saved on another nit: the password -> a password (Because you would be also able to say "some password" here.) https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:46: // If such suffix is found |consumer|->OnReuseFound() is called on the same nit: Add missing comma after "found". https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:50: PasswordReuseDetectorConsumer* consumer); nit: Could you mention that |consumer| must not be null? https://codereview.chromium.org/2525593002/diff/20001/components/password_man... File components/password_manager/core/browser/password_reuse_detector_unittest.cc (right): https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector_unittest.cc:39: std::unique_ptr<PasswordForm> form(new PasswordForm); nit: Writing auto form = base::MakeUnique<PasswordForm>(); avoids the duplicated mention of the type. https://codereview.chromium.org/2525593002/diff/40001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.h (right): https://codereview.chromium.org/2525593002/diff/40001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:57: std::map<base::string16, std::set<std::string>> passwords_; Keeping all the passwords in memory is something potentially concerning for security. Please get an opinion from somebody in Chrome security on whether it is a good idea to keep the password values inside browser process' memory indefinitely. We do this for short periods (e.g., showing passwords in settings), and the browser process is trusted, so hopefully this is OK. But let's err on the safe side and get an expert opinion.
Thanks Vaclav for comments. I've addressed them. PTAL https://codereview.chromium.org/2525593002/diff/20001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:13: int kMinPasswordLengthToCheck = 8; On 2016/11/22 12:02:02, vabr (Chromium) wrote: > size_t, not int; also: please make it constexpr (or at least const). Thanks, I forgot to set it const. https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:39: const base::string16 input_suffix = input.substr(i); On 2016/11/22 12:02:02, vabr (Chromium) wrote: > I wonder if it would make sense to pull |input_suffix| out of the for-cycle to > avoid heap-allocation in every iteration (its contents is getting smaller, so > the initial allocation would sustain all iterations). If you do so, please also > comment on this being for performance reasons. > > Ideally we would spare also the copy each time and use StringPiece16 somehow, > but for that we would likely need a custom key comparator (for std::string vs > StringPiece16). Not sure if that's worth the effort. Ok, it makes sense. I pulled |input_suffix| out of for-loop. Yeah, that for sure that in principle it's possible to make more effective (for example to use suffix trees instead of map<string,>), but since we don't have some standard ways to do this, I agree, I don't think that it's worth it, at least in the first iteration. https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:47: if (domains.find(registry_controlled_domain) == domains.end()) { On 2016/11/22 12:02:02, vabr (Chromium) wrote: > Do we really need to check all domains where the password is saved? Imagine > "pwd" is saved on a.com and b.org. Also imagine we would only store a.com in > passwords_["pwd"]. Now the user is typing it on b.org, where it is also saved, > only PasswordReuseDector ignores that and flags that as a reuse. So what? It is > a reuse, in fact: the password is used on two domains. > > If you agree, let's shrink the std::set in passwords_'s value to just > std::string. A solution with using only one domain is not ideal: 1.Which domain to store, if we make it in random (or the same as in order in which passwords are coming), we get non-determenistic behavior. 2.For example the user has "a.com" and "b.org", but we store only the first. Then Password Manager will find reuse on the second, but not on the first. That looks strange. WDYT? https://codereview.chromium.org/2525593002/diff/20001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.h (right): https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:24: // |password| is typed password that was already saved. On 2016/11/22 12:02:02, vabr (Chromium) wrote: > nit: I'm not sure what "typed" means here. It does not seem important whether > the password was typed. If you drop this line and just keeps lines 23 and 25, > the comment seems to be telling the same information already. Done. https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:25: // |saved_domain| is domain on which |password| is saved. On 2016/11/22 12:02:02, vabr (Chromium) wrote: > nit: domain -> the domain Done. https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:44: // Checks that some suffix of |input| equals to the password saved on another On 2016/11/22 12:02:02, vabr (Chromium) wrote: > nit: the password -> a password > (Because you would be also able to say "some password" here.) Done. https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:46: // If such suffix is found |consumer|->OnReuseFound() is called on the same On 2016/11/22 12:02:02, vabr (Chromium) wrote: > nit: Add missing comma after "found". Done. https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:50: PasswordReuseDetectorConsumer* consumer); On 2016/11/22 12:02:02, vabr (Chromium) wrote: > nit: Could you mention that |consumer| must not be null? Done. https://codereview.chromium.org/2525593002/diff/40001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.h (right): https://codereview.chromium.org/2525593002/diff/40001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:57: std::map<base::string16, std::set<std::string>> passwords_; On 2016/11/22 12:02:02, vabr (Chromium) wrote: > Keeping all the passwords in memory is something potentially concerning for > security. > > Please get an opinion from somebody in Chrome security on whether it is a good > idea to keep the password values inside browser process' memory indefinitely. We > do this for short periods (e.g., showing passwords in settings), and the browser > process is trusted, so hopefully this is OK. But let's err on the safe side and > get an expert opinion. There was a discussion on the design doc about it. I'll add someone from security team for review of this CL.
Thank, Vadym. Let's drill down a little bit more into the suffix searching algorithm, otherwise still LGTM. Cheers, Vaclav https://codereview.chromium.org/2525593002/diff/20001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:39: const base::string16 input_suffix = input.substr(i); On 2016/11/23 10:15:54, dvadym wrote: > On 2016/11/22 12:02:02, vabr (Chromium) wrote: > > I wonder if it would make sense to pull |input_suffix| out of the for-cycle to > > avoid heap-allocation in every iteration (its contents is getting smaller, so > > the initial allocation would sustain all iterations). If you do so, please > also > > comment on this being for performance reasons. > > > > Ideally we would spare also the copy each time and use StringPiece16 somehow, > > but for that we would likely need a custom key comparator (for std::string vs > > StringPiece16). Not sure if that's worth the effort. > > Ok, it makes sense. I pulled |input_suffix| out of for-loop. > > Yeah, that for sure that in principle it's possible to make more effective (for > example to use suffix trees instead of map<string,>), but since we don't have > some standard ways to do this, I agree, I don't think that it's worth it, at > least in the first iteration. Sorry for the back and forth on this. Only now I realised that as long as we call substr(), we are not avoiding the heap allocation, and despite input_suffix having enough space allocated, it's buffer gets replaced by that of the temporary string which is a result of substr. You already took a step back and thought about how to make the whole searching more efficient by mentioning suffix trees. I think this is actually promising and worth doing even in the first version. Let me argue that there are in fact pretty standard ways to do this. First: let's just reverse the input and the stored passwords. That translates the problem from suffixes to prefixes. Second: an incremental search is not directly possible in a map. What you can do instead is to have a vector of key-value pairs instead of the map. Then you std::sort the vector explicitly, and use std::binary_search with a custom comparator to find a key which is a prefix of the input (remember -- both are reversed, so in fact this is about suffixes). This should spare you not only the allocation and string copying, but also some asymptotical complexity. https://codereview.chromium.org/2525593002/diff/20001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:47: if (domains.find(registry_controlled_domain) == domains.end()) { On 2016/11/23 10:15:54, dvadym wrote: > On 2016/11/22 12:02:02, vabr (Chromium) wrote: > > Do we really need to check all domains where the password is saved? Imagine > > "pwd" is saved on a.com and b.org. Also imagine we would only store a.com in > > passwords_["pwd"]. Now the user is typing it on b.org, where it is also saved, > > only PasswordReuseDector ignores that and flags that as a reuse. So what? It > is > > a reuse, in fact: the password is used on two domains. > > > > If you agree, let's shrink the std::set in passwords_'s value to just > > std::string. > > A solution with using only one domain is not ideal: > 1.Which domain to store, if we make it in random (or the same as in order in > which passwords are coming), we get non-determenistic behavior. > 2.For example the user has "a.com" and "b.org", but we store only the first. > Then Password Manager will find reuse on the second, but not on the first. That > looks strange. > > WDYT? I agree it will look strange, but perhaps that's a good thing -- we want the user to get out of the state when they reuse passwords already. If this non-determinism helps to flag this to the user, that's might be a plus. And if it does not come up as suspicious, then it is not a concern, IMO. https://codereview.chromium.org/2525593002/diff/40001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.h (right): https://codereview.chromium.org/2525593002/diff/40001/components/password_man... components/password_manager/core/browser/password_reuse_detector.h:57: std::map<base::string16, std::set<std::string>> passwords_; On 2016/11/23 10:15:55, dvadym wrote: > On 2016/11/22 12:02:02, vabr (Chromium) wrote: > > Keeping all the passwords in memory is something potentially concerning for > > security. > > > > Please get an opinion from somebody in Chrome security on whether it is a good > > idea to keep the password values inside browser process' memory indefinitely. > We > > do this for short periods (e.g., showing passwords in settings), and the > browser > > process is trusted, so hopefully this is OK. But let's err on the safe side > and > > get an expert opinion. > > There was a discussion on the design doc about it. I'll add someone from > security team for review of this CL. Acknowledged.
Vadym clarified the open concerns with me offline. The CL LGTM, with one more small unrelated suggestion. https://codereview.chromium.org/2525593002/diff/60001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2525593002/diff/60001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:27: const base::string16& password = form->password_value; Could we exit early if the password is shorter than kMinPasswordLengthToCheck?
Thanks Vaclav for review! https://codereview.chromium.org/2525593002/diff/60001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2525593002/diff/60001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:27: const base::string16& password = form->password_value; On 2016/11/23 11:50:47, vabr (Chromium) wrote: > Could we exit early if the password is shorter than kMinPasswordLengthToCheck? Thanks, sure we can skip such passwords.
Thanks! Two more comments, still LGTM. Cheers, Vaclav https://codereview.chromium.org/2525593002/diff/80001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2525593002/diff/80001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:40: // TODO(668155): Optimize password look-up. nit: TODO(crbug.com/668155) matches better the common pattern than just TODO(668155). https://codereview.chromium.org/2525593002/diff/80001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:45: const base::string16& input_suffix = input.substr(i); Please use a value, not reference: const base::string16 input_suffix = ... What you do here works because of "lifetime extension", which allows the temporary result of input.substr(i) to survive until the reference you kept gets out of scope. However, this is a fragile guarantee, a subtle change in the code can cause the lifetime extension to stop working, in which case you get a use-after-deletion bug. For more information feel free to consult go/totw/107 and go/totw/101. Even there, it is recommended to steer away from relying on the lifetime extension. Note that it does not bring any performance benefit here because there is no additional copy involved even without reference (thanks to the move constructor coming to play).
https://codereview.chromium.org/2525593002/diff/80001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2525593002/diff/80001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:16: size_t constexpr kMinPasswordLengthToCheck = 8; tiny nit: Please swap the order to constexpr size_t kMinPasswordLengthToCheck = 8; Not sure if the style guide prescribes the order, but const/constexpr seems to be used before the type in Chromium code.
https://codereview.chromium.org/2525593002/diff/80001/components/password_man... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2525593002/diff/80001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:16: size_t constexpr kMinPasswordLengthToCheck = 8; On 2016/11/23 17:52:01, vabr (Chromium) wrote: > tiny nit: Please swap the order to > constexpr size_t kMinPasswordLengthToCheck = 8; > > Not sure if the style guide prescribes the order, but const/constexpr seems to > be used before the type in Chromium code. Done. https://codereview.chromium.org/2525593002/diff/80001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:40: // TODO(668155): Optimize password look-up. On 2016/11/23 17:41:45, vabr (Chromium) wrote: > nit: TODO(crbug.com/668155) matches better the common pattern than just > TODO(668155). Done. https://codereview.chromium.org/2525593002/diff/80001/components/password_man... components/password_manager/core/browser/password_reuse_detector.cc:45: const base::string16& input_suffix = input.substr(i); On 2016/11/23 17:41:45, vabr (Chromium) wrote: > Please use a value, not reference: > const base::string16 input_suffix = ... > > What you do here works because of "lifetime extension", which allows the > temporary result of input.substr(i) to survive until the reference you kept gets > out of scope. However, this is a fragile guarantee, a subtle change in the code > can cause the lifetime extension to stop working, in which case you get a > use-after-deletion bug. > > For more information feel free to consult go/totw/107 and go/totw/101. Even > there, it is recommended to steer away from relying on the lifetime extension. > Note that it does not bring any performance benefit here because there is no > additional copy involved even without reference (thanks to the move constructor > coming to play). Thanks, for noting that and for explaining with "lifetime extension".
LGTM! Thanks for your patience. :)
dvadym@chromium.org changed reviewers: + jschuh@chromium.org
Hi Justin, could you please have a look at this CL from security point of view? This CL is the first CL of the implementation of go/password_reuse_detection_client_side, on which you made some comments. It implements the class for storing passwords in memory and matching an input text against these passwords. Namely passwords are stored in a variable |passwords_| of PasswordReuseDetector. Could you please confirm that this is ok from security point of view? Please note that in the current implementation passwords will be in memory of the browser process all the time Chrome is running. Regards, Vadym
jschuh@chromium.org changed reviewers: + nparker@chromium.org
Sorry, I forsee having time to look at this. I'm comfortable with what was proposed in the doc, so maybe Nathan or someone from his team can take a pass to verify.
lgtm https://codereview.chromium.org/2525593002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_reuse_detector.cc (right): https://codereview.chromium.org/2525593002/diff/100001/components/password_ma... components/password_manager/core/browser/password_reuse_detector.cc:40: // TODO(crbug.com/668155): Optimize password look-up. Ah, nice. I read this bug and removed my optimization comments. :-)
The CQ bit was checked by dvadym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dvadym@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dvadym@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480424917624420,
"parent_rev": "b1638e6ebca91eb23b46c402cea4c8e45c71952e", "commit_rev":
"fe63afa268833e4c5eff827d4b0bce22bc180bf6"}
Message was sent while issue was closed.
Description was changed from ========== PasswordReuseDetector implementation. PasswordReuseDetector is class for detecting password reuse. It consumes passwords from PassworStore, builds password index and then it can be used for detecting of password reuse by calling method CheckReuse(). This class is not created anywhere in production code yet. It will be done in next CLs. BUG=657041 ========== to ========== PasswordReuseDetector implementation. PasswordReuseDetector is class for detecting password reuse. It consumes passwords from PassworStore, builds password index and then it can be used for detecting of password reuse by calling method CheckReuse(). This class is not created anywhere in production code yet. It will be done in next CLs. BUG=657041 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== PasswordReuseDetector implementation. PasswordReuseDetector is class for detecting password reuse. It consumes passwords from PassworStore, builds password index and then it can be used for detecting of password reuse by calling method CheckReuse(). This class is not created anywhere in production code yet. It will be done in next CLs. BUG=657041 ========== to ========== PasswordReuseDetector implementation. PasswordReuseDetector is class for detecting password reuse. It consumes passwords from PassworStore, builds password index and then it can be used for detecting of password reuse by calling method CheckReuse(). This class is not created anywhere in production code yet. It will be done in next CLs. BUG=657041 Committed: https://crrev.com/696bedaf20944503cf73fc7a5d5cda65dc30e61f Cr-Commit-Position: refs/heads/master@{#434975} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/696bedaf20944503cf73fc7a5d5cda65dc30e61f Cr-Commit-Position: refs/heads/master@{#434975} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
