328187 - crash at IMAP login with GSSAPI authentication using SSPI library

Status: VERIFIED FIXED

(Thunderbird :: General, defect)

Description

[Thorsten Schlichting]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1
Build Identifier: Thunderbird version 1.6a1 (20060221)

When trying to log into a cyrus imap server using gssapi, TB crashes if network.auth.use-sspi is set to true. If set to false and using gssapi32.dll from Kerberos for windows 3.0 from MIT, login works fine!

Reproducible: Always

Steps to Reproduce:
1. Create imap Account, select "use SSL connection", check "secure authentication".
2. Login on at a kerberized XP workstation
3. Start TB
4. click on Inbox of account.
Actual Results:  
crash

Expected Results:  
show mails

Talkback crashid ID for nightly build 2006-02-21: TB15469549Y
TB 1.5 also crashes: TB15290938Y

Comment 1

[Thorsten Schlichting]

Attachment: NSPR_LOG_MODULES=negotiateauth:5,IMAP:5

Comment 2

[David :Bienvenu]

these stacks look really familiar - I think we fixed similar problems earlier when landing this feature.

Comment 3

[Thorsten Schlichting]

If IMAP uses the same code as SMTP it might be a dup of 319210... I did not find this bug earlier because neither GSSAPI nor SSPI in the summary, sorry. If so, can I download a version that contains the fix and test it? I already tried the latest trunk...

Comment 4

[David :Bienvenu]

that does look very similar, and yes, imap does use the same code. But the fix for bug 319210 has been on the trunk and the 1.8.1 and 1.8.0.1 branches for a couple weeks now.

Comment 5

[Simon Wilkinson]

This problem does look identical to 319210. The crash is at the same point in the 
SASL handshake, and appears to come from the same SSPI code. There are three entries in Talkback that look like they're this crash:

TB15469549 (Build 2006-02-21)  	
TB15442751 (Build 2006-02-12)  	
TB15394025 (Build 2006-02-19)

Comment 6

[Thorsten Schlichting]

Attachment: fix in nsAuthSSPI::Wrap(), nsAuthSSPI::GetNextToken()

This crash is due to stack corruption in nsAuthSSPI::Wrap().
These 3 memcpy()'s are wrong:

 *outToken = nsMemory::Alloc(len)
 ...
 memcpy(outToken, ... );

The following should be done:

 memcpy(*outToken, ... );

I've created a small patch. Pointer arithmetic
for (void *) is not possible, that's the reason why
I added *p;

The other part of the patch is because
I'm worried about something in nsAuthSSPI::GetNextToken():

I see InitializeSecurityContext and thus
GetNextToken() returning a zero length token
the second time it is called. I'm neither an
experienced C++ nor Mozilla programm but maybe
GetNextToken is called from DoGSSAPIStep1/2 in
nsMsgProtocol.cpp???
That code only checks outBuf for NULL,
not OutBufLen for 0. PL_Base64Encode() is
called with len=0, but it is clever and will
call strlen() in such a case... this works because
the buffer is memset() to zero in nsAuthSSPI::GetNextToken(),
but I think it's not the way it should work...
Returning outBuf=NULL and OutBufLen=0 is more like
nsAuthGSSAPI::GetNextToken().

I successfully did some POP3/IMAP/SMTP GSSAPI-logins with this patch
from a kerberized WinXP Box on a Cyrus-Imap/sendmail server.
Without patch TB always crashes...

Comment 7

[David :Bienvenu]

Comment on attachment 214178 [details] [diff] [review]
fix in nsAuthSSPI::Wrap(), nsAuthSSPI::GetNextToken()

this looks good to me. There's an outside chance we could get this into 1.5.0.2 if we get this reviewed and checked in today.

Comment 8

[Christopher Nebergall]

Comment on attachment 214178 [details] [diff] [review]
fix in nsAuthSSPI::Wrap(), nsAuthSSPI::GetNextToken()

Looks good! Thanks

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 214178 [details] [diff] [review]
fix in nsAuthSSPI::Wrap(), nsAuthSSPI::GetNextToken()

approved for 1.8.0 branch, a=dveditz

Comment 10

[Scott MacGregor]

fixed on the 1.8.1 branch, the trunk and 1.8.0.2.

Comment 11

[Scott MacGregor]

Thorsten, care to verify that you are no longer crashing by testing out one of the 1.5.0.2 builds? Thanks!

ftp://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-mozilla1.8.0

Comment 12

[Thorsten Schlichting]

(In reply to comment #11)

I've tried 1.5.0.2 (20060307). No crash, POP, IMAP & SMTP login works very well now!
Thanks for the lightning-fast checkin! 

BTW, while looking for this bug I've seen a memory leak in nsMsgProtocol::DoGSSAPIStep2() and a nsMemory::Free() of a buffer which has been allocated by the SSPI Library, probably by malloc(), in nsAuthSSPI::Unwrap(). Both not really harmfull I think and not related to this crash but next in the code.
Is it OK to drop patches here for a later checkin or do you need a separate bug for this?

Comment 13

[David :Bienvenu]

thx for the fix. A new bug for the memory leak would be good.

Comment 14

[timeless]

feel free to mention the bug number here so people can find it. note that mixing allocators on windows is a real problem, one would hope that if sspi is doing an alloc and giving out the pointer for someone else to hold, then it provides an api (not "free") through which the memory can be released.

Comment 15

[Thorsten Schlichting]

Attachment: undo changes in nsAuthSSPI::GetNextToken()

I'm very sorry about that, the patch seems to crash firefox at NTLM Authentication.

The fast solution is to undo the change in nsAuthSSPI::GetNextToken(). This brings back the old behavior of GetNextToken() to return a pointer to a
buffer of zeros and a bufferlen of zero.

A better solution should make all GetNextToken()'s behave the same when returning a zero token and make all callers of GetNextToken() able to handle this new behavior.

Comment 16

[Thorsten Schlichting]

This is the Bug #330044 of the NTLM crash....

Comment 17

[David :Bienvenu]

Comment on attachment 214703 [details] [diff] [review]
undo changes in nsAuthSSPI::GetNextToken()

I'll try to land this on the trunk tomorrow.

Comment 18

[Marcia Knous [:marcia]]

Doesn't seem to be an easy way for QA to verify this, but based on Comment 12 by Thorsten, the fix looks good in the 20060307 build.

Comment 19

[Tracy Walker [:tracy]]

verified per reporters comment #12