POP3 AUTH Login uses incorrect utf-8 Password

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: infofrommozilla, Assigned: jorgk)

Tracking

Thunderbird 64.0

Thunderbird Tracking Flags

(thunderbird_esr6062+ fixed, thunderbird63 fixed, thunderbird64 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 months ago
+++ This bug was initially created as a clone of Bug #1491755 +++

tried to authenticte with the password "1ä2a%00"

While I was fiddling with AUTH PLAIN and AUTH LOGIN, I discovered a related error while logging in with POP3.

With AUTH PLAIN everything is fine (server log):
| > AUTH PLAIN
| < +
| TSrvPOP3Cli.Recv2(5064): [more:0-22 Bytes]AGVkZQAxw6QyYSUwMA==
| => <00>ede<00>1<C3><A4>2a%00
| => USER: ede
| => PASSWORD: 1<C3><A4>2a%00

But by simply switching the server to AUTH LOGIN, TB sends a different PASSWORD:

| > AUTH LOGIN
| < + VXNlcm5hbWU6
| TSrvPOP3Cli.Recv2(2528): [more:0-6 Bytes]ZWRl
| => USER: ede
| < + UGFzc3dvcmQ6
| TSrvPOP3Cli.Recv2(2528): [more:0-14 Bytes]MeQyYSUwMA==
| => PASSWORD: 1<E4>2a%00

<C3><A4> is the utf-8 encoded "ä" while <E4> is windows-1252/iso-8859-1 encoded.

Since we also use utf-8 for SMTP and IMAP, we should do the same for POP3.
(Reporter)

Updated

7 months ago
Depends on: 312593
Summary: smtp AUTH PLAIN incomplete utf8 Password → POP3 AUTH Login uses windows-1252 Password
(Assignee)

Comment 1

7 months ago
It's this code:

  else if (m_currentAuthMethod == POP3_HAS_AUTH_LOGIN)
  {
    MOZ_LOG(POP3LOGMODULE, LogLevel::Debug, (POP3LOG("LOGIN password")));
    NS_LossyConvertUTF16toASCII asciiPassword(m_passwordResult);
    char * base64Str = PL_Base64Encode(asciiPassword.get(),
                                       asciiPassword.Length(), nullptr);
    cmd.Adopt(base64Str);
  }

ASCII here is most likely windows-1252.

Let's check bug 312593:
https://hg.mozilla.org/comm-central/rev/23725f858c42#l33.106
-    char * base64Str =
-        PL_Base64Encode(m_passwordResult.get(), m_passwordResult.Length(),
-                        nullptr);
+    NS_LossyConvertUTF16toASCII asciiPassword(m_passwordResult);
+    char * base64Str = PL_Base64Encode(asciiPassword.get(),
+                                       asciiPassword.Length(), nullptr);

The fix is obvious, but what do the standards say?
(Assignee)

Comment 2

7 months ago
Posted patch 1493542-pop3-auto-utf8.patch (obsolete) — Splinter Review
Trivial fix, but is it correct??
Assignee: nobody → jorgk
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9011301 - Flags: review?(infofrommozilla)
(Assignee)

Comment 3

7 months ago
Bug 312593 comment #45 mentions https://tools.ietf.org/html/rfc2595 and https://tools.ietf.org/html/rfc4616 and both mention UTF-8. I don't think windows-1252 will fly here :-(
(Assignee)

Comment 4

7 months ago
Fixed white-space issue. More worrying is the code below:

  else if (m_currentAuthMethod == POP3_HAS_AUTH_USER)
  {
    MOZ_LOG(POP3LOGMODULE, LogLevel::Debug, (POP3LOG("PASS password")));
    cmd = "PASS ";
    cmd += NS_LossyConvertUTF16toASCII(m_passwordResult);

No base64 encoding here, so this will transmit 8bit characters, regardless of whether I convert to ASCII (well ANSI/windows-1252) or UTF-8.

Alfred, do you have an opinion here?
Attachment #9011301 - Attachment is obsolete: true
Attachment #9011301 - Flags: review?(infofrommozilla)
Attachment #9011302 - Flags: review?(infofrommozilla)
(Reporter)

Comment 5

7 months ago
(In reply to Jorg K (GMT+2) from comment #2)
> Created attachment 9011301 [details] [diff] [review]
> 1493542-pop3-auto-utf8.patch
> 
> Trivial fix, but is it correct??

Yes, but I would like to fix another problem while we're here.
Stay tuned...
(Reporter)

Updated

7 months ago
Summary: POP3 AUTH Login uses windows-1252 Password → POP3 AUTH Login uses incorrect utf-8 Password
(Reporter)

Comment 6

7 months ago
(In reply to Alfred Peters from comment #5)
> Yes, but I would like to fix another problem while we're here.

APOP and CRAM-MD5 also use the wrong string length:

https://dxr.mozilla.org/comm-central/source/comm/mailnews/local/src/nsPop3Protocol.cpp#2230
https://dxr.mozilla.org/comm-central/source/comm/mailnews/local/src/nsPop3Protocol.cpp#2264
(Reporter)

Comment 7

7 months ago
Use of utf-8 password on AUTH LOGIN and plain LOGIN
Use of correct password length on APOP and AUTH CRAM-MD5
Attachment #9011304 - Flags: review?(jorgk)
(Assignee)

Updated

7 months ago
Attachment #9011302 - Attachment is obsolete: true
Attachment #9011302 - Flags: review?(infofrommozilla)
(Reporter)

Comment 8

7 months ago
(In reply to Jorg K (GMT+2) from comment #4)

>   else if (m_currentAuthMethod == POP3_HAS_AUTH_USER)

>     cmd += NS_LossyConvertUTF16toASCII(m_passwordResult);
> 
> No base64 encoding here, so this will transmit 8bit characters, regardless
> of whether I convert to ASCII (well ANSI/windows-1252) or UTF-8.

As you say. If the password contains 8-bit characters, it does not matter which encoding we use. Either the 8-bit characters arrive or not. Therefore, to stay consistent, we should also use utf-8.
(Assignee)

Comment 9

7 months ago
Comment on attachment 9011304 [details] [diff] [review]
Fix for incorrect utf-8 passwords at POP3 AUTH

Review of attachment 9011304 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.

Only problem I have is that porting this to TB 60 is awkward since two bugs got in the way. But are scheduled for uplift but haven't been uplifted yet:
https://hg.mozilla.org/comm-central/rev/1519c2b5f8c4 (bug 1000851) and
https://hg.mozilla.org/comm-central/rev/271825efcfad (bug 1486913).

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +2316,5 @@
>    }
>    else if (m_currentAuthMethod == POP3_HAS_AUTH_LOGIN)
>    {
>      MOZ_LOG(POP3LOGMODULE, LogLevel::Debug, (POP3LOG("LOGIN password")));
> +    char* base64Str = PL_Base64Encode(passwordUTF8.get(),

I prefer the * on the other side, which is the "old school" style predominantly used in the file. I can fix that when landing it.

@@ +2324,5 @@
>    else if (m_currentAuthMethod == POP3_HAS_AUTH_USER)
>    {
>      MOZ_LOG(POP3LOGMODULE, LogLevel::Debug, (POP3LOG("PASS password")));
>      cmd = "PASS ";
> +    cmd += passwordUTF8.get();

So here too, right. OK.
Attachment #9011304 - Flags: review?(jorgk) → review+
(Reporter)

Comment 10

7 months ago
I'm unsure what's right at AUTH NTLM:

https://dxr.mozilla.org/comm-central/source/comm/mailnews/local/src/nsPop3Protocol.cpp#2160

Unfortunately I can not test that either.
(Reporter)

Comment 11

7 months ago
(In reply to Jorg K (GMT+2) from comment #9)
> > +    char* base64Str = PL_Base64Encode(passwordUTF8.get(),
> 
> I prefer the * on the other side, which is the "old school" style
> predominantly used in the file. I can fix that when landing it.

Sure, just as you like.

<https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations>
😛😉
(Assignee)

Comment 12

7 months ago
(In reply to Alfred Peters from comment #10)
> I'm unsure what's right at AUTH NTLM:
> https://dxr.mozilla.org/comm-central/source/comm/mailnews/local/src/
> nsPop3Protocol.cpp#2160
> Unfortunately I can not test that either.
DXR doesn't work for me today, I mostly get "Service Unavailable".

You're referring to
  DoNtlmStep1(m_username, m_passwordResult, cmd);
m_passwordResult is an nsString (UTF-16) and that gets passed to nsIAuthModule.init which takes that string.

So where is the problem or doubt?
(Reporter)

Comment 13

7 months ago
(In reply to Jorg K (GMT+2) from comment #12)
> (In reply to Alfred Peters from comment #10)
> > I'm unsure what's right at AUTH NTLM:
> > https://dxr.mozilla.org/comm-central/source/comm/mailnews/local/src/
> > nsPop3Protocol.cpp#2160
> > Unfortunately I can not test that either.
> DXR doesn't work for me today, I mostly get "Service Unavailable".
> 
> You're referring to
>   DoNtlmStep1(m_username, m_passwordResult, cmd);
> m_passwordResult is an nsString (UTF-16) and that gets passed to
> nsIAuthModule.init which takes that string.
> 
> So where is the problem or doubt?

<https://dxr.mozilla.org/comm-central/source/comm/mailnews/base/util/nsMsgProtocol.cpp#960>
|     m_authModule->Init(nullptr, 0, nullptr, NS_ConvertUTF8toUTF16(username).get(),
|                        PromiseFlatString(password).get());

nsNTLMAuthModule.cpp:
| nsNTLMAuthModule::Init(const char* /*serviceName*/, uint32_t serviceFlags,
|                        const char16_t* domain, const char16_t* username,
|                        const char16_t* password)

So Init() wants to get username and password as 'char16_t*'.
username is casted from utf-8 utf-16 - password not.
Passing password as utf-8 sounds wrong.

Yes, as it is, it should work. Sorry for the noise.
(Assignee)

Comment 14

7 months ago
No problem, it's better to follow any lead instead of shipping a bug to 25 million people. Even of 0.01% use the feature, that's still > 0 ;-)

The code is a little silly, username is already UTF-8 and needs to be converted, password arrives in the right format but needs to be converted to UTF-8 for sending in the various forms.
(Assignee)

Comment 15

7 months ago
Comment on attachment 9011304 [details] [diff] [review]
Fix for incorrect utf-8 passwords at POP3 AUTH

Since we advertise working UTF-8 passwords in TB 60, they'd better be working.
Attachment #9011304 - Flags: approval-comm-esr60+
Attachment #9011304 - Flags: approval-comm-beta+

Comment 16

7 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e55253fbb27b
Fix for incorrect UTF-8 passwords at POP3 AUTH. r=jorgk
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
(Assignee)

Updated

7 months ago
Target Milestone: --- → Thunderbird 64.0

Comment 19

6 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9e7fc0d137ce
Follow-up: remove needless .get() raw string access. r=me DONTBUILD
You need to log in before you can comment on or make changes to this bug.