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

Status: REOPENED

(MailNews Core :: Networking: IMAP, defect)

Description

[gene smith]

+++ 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.

Comment 1

[gene smith]

Attachment: WIP: Bug 1862111 - Reduce likelihood of imap account password lockout. r=mkmelin

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.

Comment 2

[gene smith]

Attachment: Bug 1862111 - Reduce likelihood of imap account password lockout. r=mkmelin

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.

Comment 3

[gene smith]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c33e9e7ecef1
Reduce likelihood of imap account password lockout. r=mkmelin

Comment 5

[Geoff Lankow (:darktrojan)]

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

Comment 6

[gene smith]

(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?

Comment 7

[Geoff Lankow (:darktrojan)]

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

Comment 8

[gene smith]

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.

Comment 9

[gene smith]

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

Comment 10

[Vineet Deo]

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

Comment 11

[Magnus Melin [:mkmelin]]

Backout: https://hg.mozilla.org/comm-central/rev/1c1e5dd4a351f57c80d0327879a77c64487e8386 (wrong bug number though)

Comment 12

[gene smith]

Attachment: Bug 1862111 - Reduce likelihood of imap account password lockout v2 r=mkmelin

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4cafc3b1d83f
Reduce likelihood of imap account password lockout v2 r=mkmelin

Comment 14

[Wayne Mery (:wsmwk)]

ready for 115?

Comment 15

[gene smith]

(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.

Comment 16

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for esr115

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 115.10.0:
https://hg.mozilla.org/releases/comm-esr115/rev/235669209e4e

Comment 18

[Rob Lemley [:rjl]]

Backout Thunderbird 115.10.1:
https://hg.mozilla.org/releases/comm-esr115/rev/24e9961527d4

Comment 19

[Rob Lemley [:rjl]]

Backout Thunderbird 126.0b1:
https://hg.mozilla.org/releases/comm-beta/rev/155f23b65f54

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/02f1d7aa04be
Backed out changeset 4cafc3b1d83f for causing process hang on shutdown. r=backout DONTBUILD

Comment 21

[gene smith]

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.

Comment 22

[gene smith]

Attachment: shutdown-fix-v0.diff

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).

Comment 23

[gene smith]

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.

Comment 24

[gene smith]

Attachment: Bug 1862111 - Reduce chance of imap account password lockout, w/o regression r=mkmelin

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.

Comment 25

[gene smith]

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.