Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2196613002:
Fix browser not revealing user password in some cases. (Closed)
|
Created:
4 years, 4 months ago by Mikhail Goncharov Modified:
4 years, 4 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix browser not revealing user password in some cases.
LOGON32_LOGON_NETWORK mode requires user account to have
"Access this computer from the network" privilege which could be revoked
for several reasons.
LOGON32_LOGON_INTERACTIVE makes more sense for our case as it requires
"Log on Locally" privilege.
The code is invoked only occasionally and there will be no performance hit
of switching from network to interactive logon.
References:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa378184(v=vs.85).aspx
http://www.codeproject.com/Articles/21050/Security-User-Impersonation
https://www.bitvise.com/wug-logontype
BUG=506862, 630317
Committed: https://crrev.com/17b08b10ca345c198389f062ef0f24b7b9a2a6ab
Cr-Commit-Position: refs/heads/master@{#410009}
Patch Set 1 #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== Fix browser not revealing user password in some cases. LOGON32_LOGON_NETWORK mode requires user account to have "Access this computer from the network" privilege which could be revoked for several reasons. LOGON32_LOGON_INTERACTIVE makes more sense for our case as it requires "Log on Locally" privilege. The code is invoked only occasionally and there will be no performance hit of switching from network to interactive logon. References: https://msdn.microsoft.com/ru-ru/library/windows/desktop/aa378184(v=vs.85).aspx http://www.codeproject.com/Articles/21050/Security-User-Impersonation https://www.bitvise.com/wug-logontype BUG=506862,630317 ========== to ========== Fix browser not revealing user password in some cases. LOGON32_LOGON_NETWORK mode requires user account to have "Access this computer from the network" privilege which could be revoked for several reasons. LOGON32_LOGON_INTERACTIVE makes more sense for our case as it requires "Log on Locally" privilege. The code is invoked only occasionally and there will be no performance hit of switching from network to interactive logon. References: https://msdn.microsoft.com/ru-ru/library/windows/desktop/aa378184(v=vs.85).aspx http://www.codeproject.com/Articles/21050/Security-User-Impersonation https://www.bitvise.com/wug-logontype BUG=506862,630317 ==========
metaflow@yandex-team.ru changed reviewers: + vabr@chromium.org, wfh@chromium.org
On 2016/07/29 04:48:47, Mikhail Goncharov wrote: > mailto:metaflow@yandex-team.ru changed reviewers: > + mailto:vabr@chromium.org, mailto:wfh@chromium.org I have managed to reproduce 630317 and check that changing login type fixes the issue.
Thanks for the patch! I don't see any concerns, but would prefer Will to have a look, because he has more experience with Windows API. Cheers, Vaclav
Description was changed from ========== Fix browser not revealing user password in some cases. LOGON32_LOGON_NETWORK mode requires user account to have "Access this computer from the network" privilege which could be revoked for several reasons. LOGON32_LOGON_INTERACTIVE makes more sense for our case as it requires "Log on Locally" privilege. The code is invoked only occasionally and there will be no performance hit of switching from network to interactive logon. References: https://msdn.microsoft.com/ru-ru/library/windows/desktop/aa378184(v=vs.85).aspx http://www.codeproject.com/Articles/21050/Security-User-Impersonation https://www.bitvise.com/wug-logontype BUG=506862,630317 ========== to ========== Fix browser not revealing user password in some cases. LOGON32_LOGON_NETWORK mode requires user account to have "Access this computer from the network" privilege which could be revoked for several reasons. LOGON32_LOGON_INTERACTIVE makes more sense for our case as it requires "Log on Locally" privilege. The code is invoked only occasionally and there will be no performance hit of switching from network to interactive logon. References: https://msdn.microsoft.com/en-us/library/windows/desktop/aa378184(v=vs.85).aspx http://www.codeproject.com/Articles/21050/Security-User-Impersonation https://www.bitvise.com/wug-logontype BUG=506862,630317 ==========
Hi Thanks for the CL. I will take a look at this. I remember at the time I landed this code we made a very deliberate choice to use LOGON32_LOGON_NETWORK but I think it /might/ have been for XP/Vista compat (which we don't need to worry about now). Give me some time to check this.
On 2016/07/29 16:21:05, Will Harris wrote: > Hi Thanks for the CL. > > I will take a look at this. I remember at the time I landed this code we made a > very deliberate choice to use LOGON32_LOGON_NETWORK but I think it /might/ have > been for XP/Vista compat (which we don't need to worry about now). Give me some > time to check this. NETWORK was chosen over INTERACTIVE because INTERACTIVE is expensive to call... see: https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m... How are you testing that this fixes the bug?
On 2016/07/29 16:49:16, Will Harris wrote: > On 2016/07/29 16:21:05, Will Harris wrote: > > Hi Thanks for the CL. > > > > I will take a look at this. I remember at the time I landed this code we made > a > > very deliberate choice to use LOGON32_LOGON_NETWORK but I think it /might/ > have > > been for XP/Vista compat (which we don't need to worry about now). Give me > some > > time to check this. > > NETWORK was chosen over INTERACTIVE because INTERACTIVE is expensive to call... > > see: > https://codereview.chromium.org/34393007/diff/40001/chrome/browser/password_m... > I understand that and consider it acceptable to do interactive for case when we authenticating one particular user after she already spend seconds on typing password (comparing that to intended use of network logon to auth hundreds of requests from different users to a single server). I haven't managed to find any good benchmarks online. On my machine one NETWORK takes 0.32 s while INTERACTIVE - 0.34 s (mean of 100 runs). > How are you testing that this fixes the bug? I can clearly reproduce the issue in my environment where my user is in the domain and has no right to network login. Without fix LoginUser returns false with GetLastError() of ERROR_LOGON_TYPE_NOT_GRANTED 1385 "Logon failure: the user has not been granted the requested logon type at this computer."
okay, we can give this a go, but vabr@ can you watch any perf regressions as a result of this? lgtm
The CQ bit was checked by metaflow@yandex-team.ru
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
metaflow@yandex-team.ru changed reviewers: + dvadym@chromium.org
On 2016/08/03 01:30:53, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) I have added dvadym as a correct owner. Sorry for that.
LGTM, thanks for the patch, and the review. I confirm that I'll check the incoming bugs and other channels for perf regressions. Cheers, Vaclav
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix browser not revealing user password in some cases. LOGON32_LOGON_NETWORK mode requires user account to have "Access this computer from the network" privilege which could be revoked for several reasons. LOGON32_LOGON_INTERACTIVE makes more sense for our case as it requires "Log on Locally" privilege. The code is invoked only occasionally and there will be no performance hit of switching from network to interactive logon. References: https://msdn.microsoft.com/en-us/library/windows/desktop/aa378184(v=vs.85).aspx http://www.codeproject.com/Articles/21050/Security-User-Impersonation https://www.bitvise.com/wug-logontype BUG=506862,630317 ========== to ========== Fix browser not revealing user password in some cases. LOGON32_LOGON_NETWORK mode requires user account to have "Access this computer from the network" privilege which could be revoked for several reasons. LOGON32_LOGON_INTERACTIVE makes more sense for our case as it requires "Log on Locally" privilege. The code is invoked only occasionally and there will be no performance hit of switching from network to interactive logon. References: https://msdn.microsoft.com/en-us/library/windows/desktop/aa378184(v=vs.85).aspx http://www.codeproject.com/Articles/21050/Security-User-Impersonation https://www.bitvise.com/wug-logontype BUG=506862,630317 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix browser not revealing user password in some cases. LOGON32_LOGON_NETWORK mode requires user account to have "Access this computer from the network" privilege which could be revoked for several reasons. LOGON32_LOGON_INTERACTIVE makes more sense for our case as it requires "Log on Locally" privilege. The code is invoked only occasionally and there will be no performance hit of switching from network to interactive logon. References: https://msdn.microsoft.com/en-us/library/windows/desktop/aa378184(v=vs.85).aspx http://www.codeproject.com/Articles/21050/Security-User-Impersonation https://www.bitvise.com/wug-logontype BUG=506862,630317 ========== to ========== Fix browser not revealing user password in some cases. LOGON32_LOGON_NETWORK mode requires user account to have "Access this computer from the network" privilege which could be revoked for several reasons. LOGON32_LOGON_INTERACTIVE makes more sense for our case as it requires "Log on Locally" privilege. The code is invoked only occasionally and there will be no performance hit of switching from network to interactive logon. References: https://msdn.microsoft.com/en-us/library/windows/desktop/aa378184(v=vs.85).aspx http://www.codeproject.com/Articles/21050/Security-User-Impersonation https://www.bitvise.com/wug-logontype BUG=506862,630317 Committed: https://crrev.com/17b08b10ca345c198389f062ef0f24b7b9a2a6ab Cr-Commit-Position: refs/heads/master@{#410009} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/17b08b10ca345c198389f062ef0f24b7b9a2a6ab Cr-Commit-Position: refs/heads/master@{#410009} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
