1493542 - POP3 AUTH Login uses incorrect utf-8 Password

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, enhancement)

Description

[Alfred Peters [:infofrommozilla]]

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

Comment 1

[Jorg K (CEST = GMT+2)]

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?

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1493542-pop3-auto-utf8.patch

Trivial fix, but is it correct??

Comment 3

[Jorg K (CEST = GMT+2)]

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

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1493542-pop3-auto-utf8.patch (v1b)

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?

Comment 5

[Alfred Peters [:infofrommozilla]]

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

Comment 6

[Alfred Peters [:infofrommozilla]]

(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

Comment 7

[Alfred Peters [:infofrommozilla]]

Attachment: Fix for incorrect utf-8 passwords at POP3 AUTH

Use of utf-8 password on AUTH LOGIN and plain LOGIN
Use of correct password length on APOP and AUTH CRAM-MD5

Comment 8

[Alfred Peters [:infofrommozilla]]

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

Comment 9

[Jorg K (CEST = GMT+2)]

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.

Comment 10

[Alfred Peters [:infofrommozilla]]

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.

Comment 11

[Alfred Peters [:infofrommozilla]]

(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>
😛😉

Comment 12

[Jorg K (CEST = GMT+2)]

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

Comment 13

[Alfred Peters [:infofrommozilla]]

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

Comment 14

[Jorg K (CEST = GMT+2)]

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.

Comment 15

[Jorg K (CEST = GMT+2)]

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e55253fbb27b
Fix for incorrect UTF-8 passwords at POP3 AUTH. r=jorgk

Comment 17

[Jorg K (CEST = GMT+2)]

TB 60.1/60.2:
https://hg.mozilla.org/releases/comm-esr60/rev/03c944b003941b961cd49ecaca42302fcbdbd678

Comment 18

[Jorg K (CEST = GMT+2)]

Beta (TB 63):
https://hg.mozilla.org/releases/comm-beta/rev/1a50352a569a9c033cc604ccde415cca07f08cb4

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9e7fc0d137ce
Follow-up: remove needless .get() raw string access. r=me DONTBUILD