1000851 - Crash of Thunderbird when using a very long username/password

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[lenormf]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140319120302

Steps to reproduce:

* Start thunderbird (24.4.0)
* Ignore the first prompt asking for a password
* Write an empty mail to anybody, click the send button
* Enter a very long password (~100000 chars did the trick on my system), `python -c "print 'a'*10000" | xsel -ib`


Actual results:

Thunderbird crashed.

The issue is located in `nsSmtpProtocol::AuthLoginStep1()`, in the scope executed when `m_currentAuthMethod` is set to `SMTP_AUTH_PLAIN_ENABLED`.

Instead of checking the return of `PR_snprintf()`, the length of the concatenated username and password is set by looking at what's in the fields of the same names. This results in a very large `len`, which will then make `encode3to4()` (base64.c) read data at an invalid address, resulting in a segmentation fault.


Expected results:

Thunderbird should have prompted for the password again, after the authentication failed.

Comment 1

[Ludovic Hirlimann [:Usul]]

(In reply to lenormf from comment #0)

> Instead of checking the return of `PR_snprintf()`, the length of the
> concatenated username and password is set by looking at what's in the fields
> of the same names. This results in a very large `len`, which will then make
> `encode3to4()` (base64.c) read data at an invalid address, resulting in a
> segmentation fault.

Thanks for the analysis, would you have the time to create a patch ?

Comment 2

[lenormf]

Attachment: Patch that fixes the crash

Comment 3

[lenormf]

This should do the trick, it also makes the code simpler.

Comment 4

[Ludovic Hirlimann [:Usul]]

(In reply to lenormf from comment #3)
> This should do the trick, it also makes the code simpler.

Thank you.

Comment 5

[[e-mail]]

Comment on attachment 8412423 [details] [diff] [review]
Patch that fixes the crash

(Wow, we "truncate" the username and password to 511 bytes, then base64 encode it to 684 bytes, then truncate it down to 256 bytes?)

Comment 6

[lenormf]

(In reply to [e-mail] from comment #5)
> (Wow, we "truncate" the username and password to 511 bytes, then base64
> encode it to 684 bytes, then truncate it down to 256 bytes?)

Yes, I'm not sure why this was implemented this way, considering that strlen(base64Str) + strlen("AUTH PLAIN ") < sizeof(buffer). Maybe users are not expected to have STMP passwords this long (a ~500B long base64 string would result from a password *way* too long to be used casually), and most SMTP servers would reject credentials this long (although that sounds unlikely) ?

Or it might just be a simple copy/paste overlook, the other authentication methods also limit the strings sent to the server to 256 bytes… which is weird too, a lot of space seems to be wasted in the `buffer`: have a look at mailnews/compose/src/nsSmtpProtocol.cpp (lines 1328, 1309), you can see that the format "%.256" is being used for two other authentication methods.

Comment 7

[[e-mail]]

Comment on attachment 8412423 [details] [diff] [review]
Patch that fixes the crash

>+    int len;
> 
>+    len = PR_snprintf(plain_string, sizeof(plain_string), "\0%s\0%s", username.get(), password.get());

The diff confused me, but I just noticed that int len = PR_snprintf (etc.) would have worked.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Pushed with comment 7's change (and a little line wrapping).

https://hg.mozilla.org/comm-central/rev/0239a98f60ab

Thanks for the patch, Frank! One request, for future patches, can you please make sure that proper commit information is included? Makes life much easier for those landing on your behalf :)
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 9

[lenormf]

Oops. I'll make sure to follow those guidelines next time!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/comm-central/rev/c7a964c0288b

Everything except the UTF-7 one at the bottom of the list.
https://tbpl.mozilla.org/php/getParsedLog.php?id=38500134&tree=Thunderbird-Trunk

Comment 11

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 12

[Wayne Mery (:wsmwk)]

lenormf, can you put up a revised patch?  If not, perhaps jorgk.


bp-dca9a8f0-561f-4c99-b4cb-cc1100171229 may be an example
 0 	libnss3.dylib	PL_Base64Encode	nsprpub/lib/libc/src/base64.c:27
1 	XUL	nsSmtpProtocol::AuthLoginStep1()	/builds/slave/tb-rel-c-esr52-m64_bld-0000000/build/mailnews/compose/src/nsSmtpProtocol.cpp:1377
2 	XUL	nsSmtpProtocol::ProcessProtocolState(nsIURI*, nsIInputStream*, unsigned long long, unsigned int)	/builds/slave/tb-rel-c-esr52-m64_bld-0000000/build/mailnews/compose/src/nsSmtpProtocol.cpp:2021
3 	XUL	nsMsgProtocol::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long long, unsigned int)	/builds/slave/tb-rel-c-esr52-m64_bld-0000000/build/mailnews/base/util/nsMsgProtocol.cpp:297

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: thunderbird-24.4.0-smtplogincrash.patch

Refreshed patch.

Refreshing the patch wasn't a problem but now we need to work out why it was backed out and which test failed. Comment #10 sadly doesn't give any details.

Here's a try run that will tell us:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=060fdef4ba9863c8fbb9a184551d5c41d43de0a0

Comment 14

[Jorg K (CEST = GMT+2)]

Oops, that didn't compile due to another patch of mine, so again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=46679cbb6d3b64949ce78a6acc03de5e2f729e7c

Comment 15

[Jorg K (CEST = GMT+2)]

Hmm, plenty of test failures:

TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_bug155172.js | - "EHLO test,AUTH PLAIN ,AUTH LOGIN,AUTH PLAIN ,AUTH LOGIN,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=155,RCPT TO:<[e-mail] == "EHLO test,AUTH PLAIN AHRlc3Quc210cEBmYWtlc2VydmVyAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3Quc210cEBmYWtlc2VydmVyAHdyb25n,MAIL F [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_sendMailMessage.js | do_check_transaction - [do_check_transaction : 71] "EHLO test,AUTH PLAIN ,AUTH LOGIN,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=155,RCPT TO:<[e-mail]>,DATA" == "EHLO test,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=155,RCPT TO:<[e-mail]>,DATA [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpAuthMethods.js | do_check_transaction - [do_check_transaction : 71] "EHLO test,AUTH PLAIN ,AUTH LOGIN,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=155,RCPT TO:<[e-mail]>,DATA" == "EHLO test,AUTH PLAIN AGZyZWQAd2lsbWE=,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=155,RCPT TO:<[e-mail]>,DATA" [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPassword.js | - "EHLO test,AUTH PLAIN ,AUTH LOGIN,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=155,RCPT TO:<[e-mail]>,DATA" == "EHLO test,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=155,RCPT TO:<[e-mail]>,DATA [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure2.js | - "EHLO test,AUTH PLAIN ,AUTH LOGIN,AUTH PLAIN ,AUTH LOGIN,AUTH PLAIN ,AUTH LOGIN,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE= == "EHLO test,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3RzbXRwAHNtdHB0ZXN0,AUTH LOGIN,AUTH PLAIN AHRlc3RzbXRw [log…] 

I guess, this doesn't work:
+    len = PR_snprintf(plain_string, sizeof(plain_string), "\0%s\0%s",
+                      username.get(), uniPassword.get());
If you put a \0 onto a C-string, the consumer will stop there. So this string here is like an empty string.

So back to the original author.

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: bug1000851_smtp_long_auth_crash.patch

Yes we can't print extra null-containing string with PR_snprintf.

Do it with string classes instead.
I changed the test file some, but didn't add a test atm. Without the cpp change it does crash if I manually set kPasswordWrong = "wrong".repeat(10000000);
But somehow the test ends up in an endless loop if I do that. Hmm. 

Fixed the uniPassword/password confusion. The variables were named the wrong in this place.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 9004254 [details] [diff] [review]
bug1000851_smtp_long_auth_crash.patch

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

It's great this this got picked up again! I had a go, but left it.

(In reply to Magnus Melin from comment #16)
> Fixed the uniPassword/password confusion. The variables were named the wrong
> in this place.
I disagree. There are two (main) string classes nsString and nsCString. Both hold strings encoded in unicode, the first one in UTF-16, the second one in UTF-8 (unless it's a string in some other encoding).

We don't really want to prefix our nsString-s with 'uni'. So please revert the variable rename. Of course I can see the dilemma. In cases like this where we carry the same thing in two different versions around, I suggest to suffix the UTF8 version, so instead of
  NS_ConvertUTF16toUTF8 uniPassword(password);
I suggest
  NS_ConvertUTF16toUTF8 passwordUTF8(password);

I'm sure the original code used uniPassword to express the fact that the nsCString variable was holding unicode (UTF-8) and not some other encoding.

Please wait for my proposal which I will attach once I fixed the replacement error you made :-( - r- for that.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +1474,2 @@
>      m_usernamePrompted = true;
> +    if (uniPassword.IsEmpty() || uniPassword.IsEmpty())

Ouch!!

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK)

Here's my proposal. I did some boy-scout clean-up by renaming the variable in some unrelated function as well.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 9004292 [details] [diff] [review]
bug1000851_smtp_long_auth_crash.patch (JK)

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

This can do with some more clean-up while we're here :-(

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +1456,5 @@
>  }
>  
>  nsresult nsSmtpProtocol::AuthLoginStep1()
>  {
> +  char buffer[512];

Note buffer is 512 bytes long.

@@ +1523,1 @@
>      PR_snprintf(buffer, sizeof(buffer), "AUTH PLAIN %.256s" CRLF, base64Str);

So here we print a base64 string which is the base64 encoding of two 255 byte/octet strings. How long do you think the sum can get and how long will it be if encoded into base64.

Anyway, we print only 265 bytes of it together with a CRLF. So why have a 512 byte buffer?

@@ +1565,2 @@
>    {
>      char buffer[512];

512!

@@ +1620,1 @@
>        PR_snprintf(buffer, sizeof(buffer), "%.256s" CRLF, base64Str);

Not sure with length to expect here, but why only print 256+2 bytes into a 512 byte buffer?

In any case, if the password were 256 bytes/octets long, its base64 encoding would be longer. Not sure what the ratio is.

Comment 20

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #19)
> Comment on attachment 9004292 [details] [diff] [review]
> 
> @@ +1523,1 @@
> >      PR_snprintf(buffer, sizeof(buffer), "AUTH PLAIN %.256s" CRLF, base64Str);
> 
> So here we print a base64 string which is the base64 encoding of two 255
> byte/octet strings. How long do you think the sum can get and how long will
> it be if encoded into base64.
> 
> Anyway, we print only 265 bytes of it together with a CRLF. So why have a
> 512 byte buffer?

Well, that was there already. Two octets of each up to 255+1. I guess it's just room to do the "string" manipulations they needed. 

> @@ +1565,2 @@
> >    {
> >      char buffer[512];
> 
> 512!
> 
> @@ +1620,1 @@
> >        PR_snprintf(buffer, sizeof(buffer), "%.256s" CRLF, base64Str);
> Not sure with length to expect here, but why only print 256+2 bytes into a
> 512 byte buffer?
> 
> In any case, if the password were 256 bytes/octets long, its base64 encoding
> would be longer. Not sure what the ratio is.

Yep, it's not turning out well if the total length is too long. But we're just doing what the standard says. The AUTH PLAIN rfc doesn't say anything about base64 encoding...

Comment 21

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #17)
> (In reply to Magnus Melin from comment #16)
> > Fixed the uniPassword/password confusion. The variables were named the wrong
> > in this place.
> I disagree. 

Well yes both are unicode, but the naming was still the wrong way here. It's quite common to have the wide version be called unicode.

Comment 22

[Jorg K (CEST = GMT+2)]

I'm not sure what to make of comment #20. My second block was *not* about AUTH PLAIN, it's about "PLAIN/LOGIN auth" as you can see from the context.

As for comment #21: I've seen some "utf16" prefixes:
https://searchfox.org/comm-central/rev/5862032a956262bfdda5a7903d24e1cb0fc93ae7/mailnews/base/search/src/nsMsgSearchTerm.cpp#1128

Common or not, it's not useful. Yes, there are some:
https://searchfox.org/comm-central/rev/5862032a956262bfdda5a7903d24e1cb0fc93ae7/mailnews/addrbook/src/nsAddrDatabase.cpp#1335

Comment 23

[Jorg K (CEST = GMT+2)]

Looking at this again. We can actually do it without the modern strings. The original code was:

-    int len = 1; /* first <NUL> char */
-    memset(plain_string, 0, 512);
-    PR_snprintf(&plain_string[1], 510, "%s", username.get());
-    len += username.Length();
-    len++; /* second <NUL> char */
-    PR_snprintf(&plain_string[len], 511-len, "%s", password.get());
-    len += password.Length();
     base64Str = PL_Base64Encode(plain_string, len, nullptr);

It's careful not to overwrite the buffer, but of course the 'len' calculation doesn't take this into account.

So we can leave that code as long as you do the truncation you proposed. Or change PR_snprintf() to memcpy().

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v2)

I put the old code back. It will work with the truncation. We just need to think about which buffers we want to send.

Comment 25

[Jorg K (CEST = GMT+2)]

Sorry, another comment, this is really a can of worms. |passwordUTF8.Truncate(255);| will potentially destroy an UTF-8 character. If anything, we should only encode a certain number of characters from the original UTF-16 string.

Also, for an UTF-16 character you can get up to 4 bytes in UTF-8.

I think it comes down to the question whether we only want to stop the crash, or do something useful.

Something useful would be to say
a) Thunderbird only supports usernames and passwords of up to 64 "real" characters.
   So some Chinese user can enter 64 characters and we will transmit them correctly
or
b) If UTF-8 string exceeds 255 bytes, return an error. It doesn't help the user to enter their
   - say - 64 Chinese characters and then they get an authentication failure since we did actually
   not transmit what there entered, but at worst a truncated version with a defective UTF-8 character.

Maybe we should just stop the crash ;-) - Since we only have a transmit buffer of 256 bytes, that gives Chinese users usernames/passwords of 256 / 2 / 8 = 32 "real" characters.

Comment 26

[Jorg K (CEST = GMT+2)]

(In reply to [e-mail] from comment #5)
> (Wow, we "truncate" the username and password to 511 bytes, then base64
> encode it to 684 bytes, then truncate it down to 256 bytes?)
Ha, ha, ha. Neil knew it already! Also see reply in comment #6.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v3)

OK, I counted the bytes that we need and made the buffers and the formats larger.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v3b)

Miscounted.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v3c)

Now it should be right.

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 9004403 [details] [diff] [review]
bug1000851_smtp_long_auth_crash.patch (JK, v3c)

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

> Looking at this again. We can actually do it without the modern strings. 

Can. But I don't see why we wouldn't take the opportunity => no need to carefully think about indexes, and more readable code etc.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +1511,5 @@
>    }
>    else if (m_currentAuthMethod == SMTP_AUTH_PLAIN_ENABLED)
>    {
>      MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Debug, ("PLAIN auth"));
> +    // Up to 255 octets. Note that truncation can destroy UTF-8 characters.

Please remove this comment.

I think you're mixing up concepts here. We don't really know how many octets will be used for the characters we put in there, until the UTF-8 encoding happens. We're cheating on the assumption (one bit per char) a bit (hah!) but for practical usage that doesn't matter (since usernames/pwds this long are not usable)

@@ +1529,4 @@
>      len += password.Length();
>  
>      base64Str = PL_Base64Encode(plain_string, len, nullptr);
> +    PR_snprintf(buffer, sizeof(buffer), "AUTH PLAIN %.684s" CRLF, base64Str);

I read up on rfc4954:

          Note that these [BASE64] strings can be much longer than
          normal SMTP commands.  Clients and servers MUST be able to
          handle the maximum encoded size of challenges and responses
          generated by their supported authentication mechanisms.

So I think the truncating to 684 (or anything other) should just go away.

Comment 31

[Magnus Melin [:mkmelin]]

That should have read one byte (or octet) per char, of course.

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 9004403 [details] [diff] [review]
bug1000851_smtp_long_auth_crash.patch (JK, v3c)

Sorry, I don't agree on anything here, so please reconsider.

(In reply to Magnus Melin from comment #30)
> > Looking at this again. We can actually do it without the modern strings. 
> Can. But I don't see why we wouldn't take the opportunity => no need to
> carefully think about indexes, and more readable code etc.
The code is full of raw character manipulation and thinking, or not thinking, about indices. Throwing in a "modern" bit
+    nsAutoCString plain = NS_LITERAL_CSTRING("\0") + username +
+                          NS_LITERAL_CSTRING("\0") + passwordUTF8;
doesn't actually improve the overall situation. Since those strings are also null terminated, creating a string with a null and then another null to terminate makes me wonder when that will go wrong next. Having a nulled buffer and writing to specific locations is quite a permissible solution. Sadly the length calculation was wrong leading to crashes.

> > +    // Up to 255 octets. Note that truncation can destroy UTF-8 characters.
> Please remove this comment.
Why? Truncating a UTF-8 string at a random location makes no sense and will certainly destroy information.
I agree that in practise usernames and passwords won't be that long.

> > +    PR_snprintf(buffer, sizeof(buffer), "AUTH PLAIN %.684s" CRLF, base64Str);
> I read up on rfc4954:
>           Note that these [BASE64] strings can be much longer than
>           normal SMTP commands.  Clients and servers MUST be able to
>           handle the maximum encoded size of challenges and responses
>           generated by their supported authentication mechanisms.
> So I think the truncating to 684 (or anything other) should just go away.
Thanks for the research. It shows that I was right to widen the buffer to 698 characters. Since we base64-encode 512 bytes at most, the 'base64Str' will be at most 684 bytes long, so having the truncation in the code makes no difference. I can take it out if you insist, but functionally, it makes no difference.

Conservatively one could argue that if for some coding error, the base64 string became wider than expected, at least the buffer would still be CRLF terminated.

Comment 33

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v3d)

OK, now that we truncate, some hard to read code can be simplified ;-)

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v3e)

Even simpler.

Comment 35

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #32)
> Comment on attachment 9004403 [details] [diff] [review]
> bug1000851_smtp_long_auth_crash.patch (JK, v3c)
> 
> Sorry, I don't agree on anything here, so please reconsider.
> 
> (In reply to Magnus Melin from comment #30)
> > > Looking at this again. We can actually do it without the modern strings. 
> > Can. But I don't see why we wouldn't take the opportunity => no need to
> > carefully think about indexes, and more readable code etc.
> The code is full of raw character manipulation and thinking, or not
> thinking, about indices. Throwing in a "modern" bit
> +    nsAutoCString plain = NS_LITERAL_CSTRING("\0") + username +
> +                          NS_LITERAL_CSTRING("\0") + passwordUTF8;
> doesn't actually improve the overall situation. Since those strings are also
> null terminated, creating a string with a null and then another null to
> terminate makes me wonder when that will go wrong next. Having a nulled

Hmm, maybe it is indeed a bit risky.

> 
> > > +    // Up to 255 octets. Note that truncation can destroy UTF-8 characters.
> > Please remove this comment.
> Why? Truncating a UTF-8 string at a random location makes no sense and will
> certainly destroy information.

Because it's not relevant. We truncate anyway == we always destroy information.

> > So I think the truncating to 684 (or anything other) should just go away.
> Thanks for the research. It shows that I was right to widen the buffer to
> 698 characters. Since we base64-encode 512 bytes at most, the 'base64Str'
> will be at most 684 bytes long, so having the truncation in the code makes
> no difference. I can take it out if you insist, but functionally, it makes
> no difference.

Yes, please remove it.

Comment 36

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v3f)

Happy?

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v3g)

Actually, below we truncate a response to 512, so I've changed the truncation above as well.

Comment 38

[Jorg K (CEST = GMT+2)]

This is still wrong, the plaintext buffer needs to be 513 bytes, 2x 255 + 2 nulls + 1. Afk right now, I'll fix it later.

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v3h)

Yet another version. The longer I look at it, the more worms I find.

So the the digest is 16 characters long, written as hex, that's 32 characters. I have no idea why an 8-byte hexVal was needed to encode on byte. I think three are needed, the array will hold "00\0" to "FF\0". The format description says "%.2x" so at most two characters will be written.

I hope this is the last version now.

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v3i)

Nope, wasn't the last version, missing parenthesis in comment.

Comment 41

[Jorg K (CEST = GMT+2)]

Would you be surprised if I told you that we have this same "beautiful" code twice more in the system?

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

So once we're done here we should port the agreed solution to those protocols, too.

Comment 42

[Magnus Melin [:mkmelin]]

Comment on attachment 9004613 [details] [diff] [review]
bug1000851_smtp_long_auth_crash.patch (JK, v3i)

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

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +1592,5 @@
>        PR_Free(decodedChallenge);
>        if (NS_SUCCEEDED(rv))
>        {
> +        // Hexadecimal representation of DIGEST_LENGTH characters,
> +        // so maximal twice that length.

so the encodedDigest will be twice that length.

@@ +1599,1 @@
>  

maybe move this into the loop. it's confusing to have it outside and reusing it

::: mailnews/test/fakeserver/auth.js
@@ +57,5 @@
>    encodeLine : function(username, password)
>    {
> +    username = username.substring(0, 255);
> +    password = password.substring(0, 255);
> +    return btoa("\u0000" + username + "\u0000" + password).substring(0, 256); // base64 encode

the last truncation should go now

Comment 43

[Jorg K (CEST = GMT+2)]

Attachment: bug1000851_smtp_long_auth_crash.patch (JK, v3j)

Wow, finally done. Not onto the other protocols :-(

BTW, I'm using this now:
-        nsAutoCString encodedDigest;
-        char hexVal[8];
+        // The encoded digest is the hexadecimal representation of
+        // DIGEST_LENGTH characters, so it will be twice that length.
+        nsAutoCStringN<2 * DIGEST_LENGTH> encodedDigest;
 
-        for (uint32_t j=0; j<16; j++)
+        for (uint32_t j = 0; j < DIGEST_LENGTH; j++)
         {
-          PR_snprintf (hexVal,8, "%.2x", 0x0ff & (unsigned short)digest[j]);
+          char hexVal[3];
+          PR_snprintf (hexVal, 3, "%.2x", 0x0ff & (unsigned short)digest[j]);
           encodedDigest.Append(hexVal);
         }

You can pre-allocate auto-strings in the correct length, if you know it. Surely the "j<16" wasn't all that useful.

Comment 44

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/1519c2b5f8c4189e3d3b36a358daf12886b1dfe5
Fix crash of Thunderbird when using a very long username/password. r=jorgk

Comment 45

[Jorg K (CEST = GMT+2)]

Attachment: 1000851-follow-up.patch

Comment 46

[Magnus Melin [:mkmelin]]

Comment on attachment 9004718 [details] [diff] [review]
1000851-follow-up.patch

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

Oh what a trap. r=mkmelin

Comment 47

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/092c52d61cba0dc8d4ce8a5ebe688edb3144f0a5
Follow-up: Correct buffer size. r=mkmelin DONTBUILD

Comment 48

[Jorg K (CEST = GMT+2)]

TB 60.3 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/90e2c9058adcff296ebfafd71acd601232211087
https://hg.mozilla.org/releases/comm-esr60/rev/70d7b8ada4ecb939ac39848eb39b22e526d062e0