1486913 - Port changes of bug 1000851 to POP and IMAP protocols

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, enhancement)

Description

[Jorg K (CEST = GMT+2)]

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

In bug 1000851 we fixed some issues with username and password encoding. The same issues exist in POP and IMAP and should be fixed there, too.

https://searchfox.org/comm-central/search?q=PR_snprintf(%26plain&case=false&regexp=false&path=
https://searchfox.org/comm-central/rev/cce392b6a57170eb2139bbf15eaa9a88340f97b2/mailnews/local/src/nsPop3Protocol.cpp#2303
https://searchfox.org/comm-central/rev/cce392b6a57170eb2139bbf15eaa9a88340f97b2/mailnews/imap/src/nsImapProtocol.cpp#5920

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1486913-auth-POP.patch

Déjà vu?

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1486913-auth-IMAP.patch

Another déjà vu. Not truncating in IMAP won't cause crashes apart from PLAIN which has the same faulty code. I now truncate everywhere for consistency. Let me know what you think.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1486913-auth-POP.patch (v1b)

Off by one. If you want to write 255 net bytes, you need to tell it that the buffer is 256 bytes long.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1486913-auth-IMAP.patch (1b)

And here.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9004717 [details] [diff] [review]
1486913-auth-IMAP.patch (1b)

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5942,5 @@
>      ParseIMAPandCheckForNewMail();
>  
>      if (GetServerStateParser().LastCommandSuccessful())
>      {
> +      char *base64Str = PL_Base64Encode(userName, std::min((int)PL_strlen(userName), 255), nullptr);

looks like this would crash if the type casting actually is needed, and the length is suitable to make the number wrap around to the negative side

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1486913-auth-POP.patch (v1c)

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1486913-auth-IMAP.patch (v1c)

Sadly you didn't answer the ...
> Not truncating in IMAP won't cause crashes apart from PLAIN
> which has the same faulty code. I now truncate everywhere for consistency.
> Let me know what you think.

So if you'd rather not truncate everywhere, we need another round.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9004785 [details] [diff] [review]
1486913-auth-POP.patch (v1c)

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

LGTM, r=mkmelin

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #7)
> I now truncate everywhere for consistency.

I think that's fine.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9004787 [details] [diff] [review]
1486913-auth-IMAP.patch (v1c)

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5951,5 @@
>        {
>          ParseIMAPandCheckForNewMail(currentCommand);
>          if (GetServerStateParser().LastCommandSuccessful())
>          {
> +          base64Str = PL_Base64Encode(password.get(), std::min((int)password.Length(), 255), nullptr);

you forgot to change these casts. ^^^, and the one above

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1486913-auth-IMAP.patch (1d)

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9004791 [details] [diff] [review]
1486913-auth-IMAP.patch (1d)

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

Thx! r=mkmelin

Comment 13

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/271825efcfad6aec1857c88b5ef60847e0ccc37a
fix username/password handling in POP. r=mkmelin
https://hg.mozilla.org/comm-central/rev/4aace069f9d1d0952765f8490306726ad5f29c2d
fix username/password handling in IMAP. r=mkmelin

Comment 14

[Jorg K (CEST = GMT+2)]

TB 60.3 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/ffe25f93f1e9ab89de12cbfded77c11108f93003
https://hg.mozilla.org/releases/comm-esr60/rev/18a94aeb864e8c847d4cc096a4e2c301f021bb5e