Closed Bug 1862111 Opened 7 months ago Closed 5 days ago

[IMAP] too many login attempts using the old password. Can cause locked account

Categories

(MailNews Core :: Networking: IMAP, defect)

Thunderbird 121
defect

Tracking

(thunderbird_esr115+ affected, thunderbird123 wontfix, thunderbird125 wontfix, thunderbird126 wontfix, thunderbird127 affected)

RESOLVED FIXED
124 Branch
Tracking Status
thunderbird_esr115 + affected
thunderbird123 --- wontfix
thunderbird125 --- wontfix
thunderbird126 --- wontfix
thunderbird127 --- affected

People

(Reporter: gds, Assigned: gds)

References

Details

Attachments

(3 files, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #1768121 +++

This essentially re-opens closed bug 1768121 since changes proposed for bug 1854586, if approved, will cause bug 1768121 to be un-fixed.
Edit: bug 1854586 has been approved and landed.
(I couldn't find a way to re-open a bug that has been closed for about 1 year.)

Some of original bug description:

Steps to reproduce:

I changed the password for my imap account 2 days ago, while Thunderbird was not running and my Mac was switched off.

Actual results:

On the first run of Thunderbird after the password change it tried the old password 6 times within a few seconds and got me locked out for 10 minutes.

There was a popup asking for the new password, but this did not help me to bypass the locked account. Too late. And there was no indication why I could not login.

With inital imap authentication with a stored password, if SASL
authentication is supported, e.g., AUTH=PLAIN, and it fails, don't
also attempt the imap LOGIN command but prompt for a correct password.
This can reduce the number of failed attempts from 4 to 2 and better
prevent a bad password entry lockout.
This will still allow the imap LOGIN to occur if no AUTH= capability
is advertised, or after a new password has been entered for the
connection or if the connection doesn't have TLS/STARTLS security.

See Also: → 1854586
Attachment #9361127 - Attachment is obsolete: true

Only allow one imap thread per server at a time to do authentication.
This prevents multiple prompts from occurring and also reduces the
number of bad passwords per server potentially sent.

This is needed because of changes in Bug 163964 which causes folder discovery and select of Inbox to be triggered together at startup. This caused two authentications in separate imap threads to occur almost simultaneously. If the stored password is wrong, this will cause twice as many bad passwords to be sent to the server compared to before Bug 163964 landed. Because of this, a user reported (in Bug #1768121) that his account was lock by the provider due to too many invalid passwords being received.

The fix in comment 2 essentially "serializes" the authentication so that the thread doing discovery is authenticated first and then the thread for Inbox select is authenticated next. So if the stored password is wrong (or is not stored at all), only one prompt to enter the correct password occurs and is sent to the server. If the server imap capabilities indicate support for multiple authentication methods, e.g., PLAIN, LOGIN, old-style etc, each will still be tried (assuming a bad password is stored) as was done pre-Bug 163964. So with this example set of capabilities and with a bad password stored, the change at comment 2 reduces the number of bad passwords sent from 6 to 3 before the prompt for a corrected password occurs, and now only one prompt (not two) occurs.
Note: Most servers that don't use OAuth2 authentication support PLAIN and also old-style (default authentication specified in imap RFC). So the typical reduction in bad passwords sent will be from 4 to 2.

See Also: → 163964
See Also: → 1617060
Target Milestone: --- → 124 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/c33e9e7ecef1
Reduce likelihood of imap account password lockout. r=mkmelin

Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED

Gene, we're getting a lot of debug assertions with the message "nsWeakReference not thread-safe" since this landed.

Flags: needinfo?(gds)

(In reply to Geoff Lankow (:darktrojan) from comment #5)

Gene, we're getting a lot of debug assertions with the message "nsWeakReference not thread-safe" since this landed.

I'm not seeing it with my debug build with normal usage and it's definitely going through the code that uses the weak pointer.
Are you seeing this maybe in a test? Maybe with just windows or macos? I'm only running a debug build on linux and it has my patch applied, but I haven't updated c-c (hg pull -u) since Jan 5th.
Can you show me the exact message that prints in console?

Flags: needinfo?(gds)

Hmm, I don't know why that would be, but it's definitely failing on the CI.

I pulled in and updated CC and MC and rebuilt. Then manually ran the failing tests for the try build. I don't see any errors when I do that.
Double checked that my patch is really present in nsImapProtocol.cpp, and it is.
FWIW, before I submitted the patch, on Jan 20 I ran a try build and it didn't show the errors now reported: Jan20 try
Never mind, that try build was optimized.

I found another way like they do here
https://searchfox.org/comm-central/rev/91e927ae548dde6d9c8f1d9a26effa60da929ee5/mailnews/base/src/nsMsgAccountManager.cpp#1505
and here
https://searchfox.org/comm-central/rev/91e927ae548dde6d9c8f1d9a26effa60da929ee5/mailnews/base/src/nsMsgAccountManager.cpp#1595

root (pointer to the server's root folder) is constant between imap threads for a given server. So using it as a parameter to the monitor works too (just like the weak ptr to the server). I re-tested with real servers to make sure it works.

I'll provide a formal fix tomorrow if you are sure or fairly sure that the weak ptr is causing the test failures. I was never able to see failures about weak ptrs when manually running the tests with my up-to-date debug build using CC head.

Note: The current CC head code never de-references the weak ptr. From what I understand, the monitor code just uses it to index into a "monitor cache". Re: https://searchfox.org/comm-central/source/mozilla/docs/nspr/reference/pr_centermonitor.rst

Gene, I backed out your changes for causing the test failures highlighted above, and am reopening the bug.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Fixes debug mode test failures due to using weak reference in
imap thread.
Passing try build with debug enabled: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=4f253184b16ec4ed547e4ff10bda876a6bd9ee9d

Attachment #9375639 - Attachment is obsolete: true

Pushed by solange@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/4cafc3b1d83f
Reduce likelihood of imap account password lockout v2 r=mkmelin

Status: REOPENED → RESOLVED
Closed: 4 months ago4 months ago
Resolution: --- → FIXED
Summary: [IMAP] too many login attempts using the old password → [IMAP] too many login attempts using the old password. Can cause locked account
Component: Security → Networking: IMAP
Product: Thunderbird → MailNews Core

ready for 115?

Flags: needinfo?(gds)

(In reply to Wayne Mery (:wsmwk) from comment #14)

ready for 115?

It's OK.

Edit 4-26-24:
https://support.mozilla.org/en-US/questions/1441054
Had I seen that or if it had made it into a bug report, I wouldn't have said "It's OK".
The user "Dudley F" claims clearing "Empty trash on exit" didn't help at that time. So either he was wrong or maybe there is another URL that can occur at shutdown that causes the same problem as URL "deleteallmsgs".
Same reporter here (using a different handle) claims 115.10.1 (my patch backed out) doesn't help: bug 1891889 comment 50.
But then here he says the problem is fixed with 115.10.1: bug 1891889 comment 64.

Flags: needinfo?(gds)

Comment on attachment 9378396 [details]
Bug 1862111 - Reduce likelihood of imap account password lockout v2 r=mkmelin

[Triage Comment]
Approved for esr115

Attachment #9378396 - Flags: approval-comm-esr115+
Regressions: 1891889
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout by alessandro@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/02f1d7aa04be
Backed out changeset 4cafc3b1d83f for causing process hang on shutdown. r=backout DONTBUILD

Even with patch for this bug backed out I'm seeing another problem when "empty trash on shutdown" is configured. If Trash mailbox has never been logged into, an attempt is made to login to Trash to do the delete/expunge. But a check is made to see if a shutdown is in progress which is true and when true, AuthLogin() returns an error which causes the login to Trash to report as failed and the "deleteallmessages" URL never runs on Trash mailbox, leaving all the messages still in Trash.

I'm seeing this on linux with a debug build, so timing may vary with windows or mac.

Also linux with debug build, with the patch for this bug still in place, shutting down hangs. That is, I click the X or do ctrl-q and the TB screen remains in place but unresponsive and I can only completely shut it down with ctrl-c at the bash terminal where I started TB. The last thing that appears in IMAP:4 log is "enter mon". So it's getting stuck between the "enter mon" and the "exit mon" which sounds similar to what was observed on windows at bug 1891889.

Attached patch shutdown-fix-v0.diff (obsolete) — Splinter Review

Here's a first attempt at a fix for the regression (and it also empties Trash on shutdown). It now works on linux. I need to make a try build so I can test it on windows (where the regression was discovered).

Try build just for windows: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=05c898ac9027dbddc4cb2bfb90bf9e5a169b4e8b
The build works correctly with empty trash on exit on Windows 11 when shutting down. There is a clean shutdown with no TB processes still running.

Also, the AuthLogin() failure due to shutdown flag is also caught with windows build and allows Trash to be emptied. But I did notice that with 115.10.1 (has the bug backed out) that AuthLogin() didn't see the shutting down flag so it still authenticated and emptied trash. This just shows that the failure to empty trash on shutdown is timing dependent due to when the shutting down flag goes true.

FWIW, a workaround if still using 115.10.0 (for bug 1891889 and its dupes) is select the Trash folder right before shutting down. This establishes an authenticated connection to the Trash mailbox so that auth doesn't occur during shutdown. Of course, this may not be practical if there are a lot of accounts all having "empty on shutdown" configured. Mostly, this just verifies what the problem is when I did this with 115.10.0.

This now allows only the "kinds of URLs that occur during shutdown" to occur
with AuthLogin() in progress. Now, in C-C, no URL can occur during
shutdown that requires authorization so expunge Inbox and empty trash fail.
However, release 115 currently allows any URL requiring auth during
shutdown so expunge/empty still works with release.
See bug 1768344 comment 88 for more info.

I never did explain why 115.10.0 was failing on shutdown with the "clean up" items selected in server settings (empty trash / expunge inbox on shutdown). The reason is because both ImapProtocol and AccountManager were waiting on the same semaphore (monitor value) so there was a deadlock. As pointed to in comment 9 above, I ended up using the same method in ImapProtocol as AccountManager to obtain the pointer parameter to the monitor calls and during the shutdown cleanup operation both were waiting on the same parameter in the monitor calls, causing the failure to completely shutdown.

Attachment #9378396 - Flags: approval-comm-esr115+

(In reply to gene smith from comment #24)

Created attachment 9398754 [details]
Bug 1862111 - Reduce chance of imap account password lockout, w/o regression r=mkmelin

Hmm. If we're shutting down while there's no valid username/password to log into the server, shouldn't any subsequent attempts to run a URL just silently fail without prompting the user for login details?
If the user shuts down the app, it seems bad to then prompt them for login details.
So the shutdown expunge/emptytrash/deletefolder operations should just fail or - even better - not even be issued in the first place if there's no pre-existing authentication!

Ben,
As I alluded to in comment 24 and describe in more detail at bug 1768344 comment 88, the changes in AuthLogin() that you reference were put in to make sure the URLs that trigger "empty trash at shutdown" and "expunge inbox at shutdown" are allowed to occur. Typically when shutdown occurs, there is already a stored password so this won't automatically cause a password prompt when shutting down. I don't think I've ever seen that occur.
I guess technically the changes I propose to AuthLogin() should actually be a separate bug from the "too many password prompts" issue. But since I saw the problem (no clean up at shutdown) when testing the regression fix with daily, I put the proposed change to AuthLogin in with this bug.

See Also: → 1768344

Ben,
What do you think of this? This is similar to my original attempt at a fix as seen in my 2nd (now marked obsolete) attachment here: https://phabricator.services.mozilla.com/D199183. That worked OK but I had to abandon it due to test failures caused by use of an nsWeakPtr (see comment 5 to comment 8 above).

This changes it so the weak ptr is cast to a void pointer and I use that in the monitor calls (enter and exit) and it's working as it should. I did a try build in debug mode using cast to void and I don't see the crashes:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=9ab5c9edd7544b821175363c8aa6beb37c677d81

I also ran the build again with the original weak pointer and it does cause crashes:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=bab45e9866c28edf4261e608625e66e7190846b7

Edit: Added link to marked obsolete patch in 1st paragraph.

Attachment #9378396 - Attachment is obsolete: true
Attachment #9397317 - Attachment is obsolete: true
See Also: → 1896545
Flags: needinfo?(benc)

Quick question: Is there any reason not to put the whole of TryToLogon() under the monitor, rather than just that core chunk?

(In reply to gene smith from comment #28)

Created attachment 9401530 [details] [diff] [review]
lockout-fix--use-void-ptr.diff
Ben,
What do you think of this? This is similar to my original attempt at a fix as seen in my 2nd (now marked obsolete) attachment here: https://phabricator.services.mozilla.com/D199183. That worked OK but I had to abandon it due to test failures caused by use of an nsWeakPtr (see comment 5 to comment 8 above).

This changes it so the weak ptr is cast to a void pointer and I use that in the monitor calls (enter and exit) and it's working as it should.

I think your approach is right (excluding other threads from the login core), but I still don't like the way the nsImapProtocol is unilaterally locking upon the server without the server's cooperation or knowledge.

TD;DR: I'm trying out an alternative locking approach - I should have something to show you tomorrow.

I'd much rather have an explicit monitor object on the nsImapIncomingServer for arbitrating logons.
The other issue is that I'm not 100% clear what value the void* will actually be. If it's the server object itself, it'll also be in contention with lots of other places in the IMAP server that set up locks with PR_CEnterMonitor(this). I could certainly see that being problematic. I suspect the void* will actually be the address of something in the WeakPtr mechanism, which should be fine? but I couldn't tell you for sure without digging deep... and I'll certainly have forgotten by the next time I come to work in this code! So I'd rather have a proper, explicit monitor object on the server, even if we have to jump through a few hoops.

It's tricky because the nsImapProtocol and nsImapIncomingServer don't know about each other's concrete types - they go via xpcom. Which is silly really (but out of scope here: briefly, they are bound intimately together and depend upon each other's implementation, so may as well cooperate. The xpcom interfaces just get in the way!).
And we can't just add a mozilla::Monitor object to the server because there's no way to pass it over the xpcom boundary.
We could just do some static casting, but that seems like an icky hack.
We could just add an xpcom method/attr to nsIImapIncomingServer which provides a unique integer for that server, for PR_CEnterMonitor() to lock on, but that seems very icky indeed.

But I did think of an XPCOM-friendly way to do it, which I'm trying out now. I will report back in the morning.

Quick question: Is there any reason not to put the whole of TryToLogon() under the monitor, rather than just that core chunk?

I suspect it would work OK. I haven't tried it though.

About the "void" and "weak" pointer values, I have no idea what their "value" really is either. I originally used the weak pointer to server in monitor parameter and it worked but the tests didn't like it even thought, AFAIK, it was never dereferenced. The test don't mind when I cast the weak ptr to void and use it and it "seems" to work. But probably not good to do something not well understood (i.e., the "value" of void etc.).

I also proposed to use the server key number as the mon enter/exit parameter which will be unique and is a well understood number. But I think you didn't like that. I also did a 32-bit hash of the full server key string which also worked but seemed a bit too complicated so I never proposed it.

I originally tried to just find a way to limit the number of prompts that occur when logging in directly in the code but then hit on the monitor method as much easier. Maybe I should look at that again.

But I did think of an XPCOM-friendly way to do it, which I'm trying out now. I will report back in the morning.

Sounds good. If you figure that out, we can just go with that. Thank!

Had to rethink a little bit due to threading issues, but here's my general proposal.

I still don't like that we're just hacking things to deal with this specific case rather than stepping back and taking the bigger view on how IMAP login should work and refactor the whole lot... but that'd take way too long :-(
The main thing here is that the server has to actively participate in the logon locking, so it's explicit.

I wrapped lock around the TryToLogon() call entirely, for simplicity. I didn't spot anything in there that would cause a problem with that, but it's tricky to tell for sure.

try run going here:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=c251d92670151737e76996a8dea0b8d3d78024a1

Hi Ben, I tested the patch and it does what it should: serializes the login for each server's mailbox. I also checked to make sure that it still works when a prompt for a password is needed and it does.
If maybe understand this, you are somehow calling "TryToLogon" running on an imap thread (one imap thread for each connected mailbox) from the incoming server's "sink" running on "main" thread? (It's never been clear to me what a "sink" is and what a runable helper is.)

(In reply to gene smith from comment #33)

If maybe understand this, you are somehow calling "TryToLogon" running on an imap thread (one imap thread for each connected mailbox) from the incoming server's "sink" running on "main" thread? (It's never been clear to me what a "sink" is and what a runable helper is.)

The Sink interfaces are implemented by the nsImapIncomingServer object, and there are Proxy sink objects for each of them. The idea is that an nsImapProtocol object running on another thread can just call a method on the sink proxy, which runs that method on the real sink (the nsImapIncomingServer), on the main thread before returning. The proxy takes care of all the thread safety.

I wanted the nsImapIncomingServer to handle the logon locking, so I added the runLogonExclusive() method to the server (which implements the server sink interface). But in that specific case, the TryToLogon() function is supposed to run on the current (IMAP) thread, not the main thread. So I purposely sabotaged the Proxy sink implementation of runLogonExclusive() - it'll assert. You have to use the one on the real sink (i.e. the nsImapIncomingServer).

I used nsIRunnable as the callback type, because I needed a generic XPCOM callback that the server could invoke. nsIRunnable is just an XPCOM object with a run() method. it's often used for threads, or for dispatching routines to other threads, but really it's just something you can run().
I'd much rather all this was done directly with concrete C++ classes rather than through XPCOM interfaces but, annoyingly, the nsImapServer and nsImapProtocol only know about each other through XPCOM (and the sink interfaces). It's frustrating - there's no reason to use XPCOM for such internal-to-IMAP interfaces, but here we are :-(

Anyway.
That all sounds waaaay more complicated than it should be, but hey. That's our IMAP code for you :-)
The main incentive for me was to make it really clear that the server had to be arbitrating the logon exclusivity. We can put that kind of thing in comments (as you did!), but it's nice to have it explicit in code.

Flags: needinfo?(benc)
Attachment #9403405 - Attachment description: WIP: Bug 1862111 - Prevent IMAP login dogpile. → Bug 1862111 - Prevent IMAP login dogpile. r=#thunderbird-reviewers

Pushed by benc@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/57efc195a312
Prevent IMAP login dogpile. r=darktrojan

Status: REOPENED → RESOLVED
Closed: 4 months ago5 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: