Closed Bug 511806 Opened 15 years ago Closed 15 years ago

Crash when using SSPI and joined to AD under Vista or later [@ strcat - nsImapFlagAndUidState::AddUidCustomFlagPair]

Categories

(MailNews Core :: Networking: IMAP, defect)

1.9.1 Branch
x86
Windows Vista
defect
Not set
critical

Tracking

(status1.9.2 beta1-fixed, status1.9.1 .4-fixed)

VERIFIED FIXED
Thunderbird 3.0b4
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- .4-fixed

People

(Reporter: pnfisher, Assigned: Bienvenu)

References

Details

(Keywords: crash, verified1.8.1.24, verified1.9.1, Whiteboard: [sg:critical][no l10n impact][needs 1.9.0 landing])

Crash Data

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.2) Gecko/20090803 Fedora/3.5.2-2.fc11 Firefox/3.5.2
Build Identifier: 3.0b3 / 20090715140311

The UC Berkeley central email system recently enabled GSSAPI support for SMTP, IMAP, and POP.  Thunderbird 2.0.0.22 on Windows XP when joined to AD works fine; however, under Vista or Windows 7 (and joined to AD), Thunderbird crashes when attempting to use SSPI.  Thunderbird 2, by default, will always attempt to use SSPI during SMTP, and we've received widespread reports across campus of Thunderbird crashing when attempting to send email.  Setting network.auth-use-sspi to false returns Thunderbird to a stable state.

We have been unable to get Talkback data for Thunderbird 2, so we've replicated the problem with Thunderbird 3.0b3 and Breakpad, which appears to also crash under SSPI.

This bug may be a duplicate of bug 505971.  However, since this behaviour is widespread in our environment, and Thunderbird immediately crashes with SMTP or IMAP when joined to AD on Vista or later and SSPI is enabled it seems worthwhile to file a separate bug report.

Reproducible: Always

Steps to Reproduce:
1. Join Windows Vista (or later) machine to CAMPUS.BERKELEY.EDU AD
2. Configure Thunderbird client to use calmail.berkeley.edu (which will advertise AUTH=GSSAPI for SSL/TLS connections).
3. Attempt to send email with Thunderbird 2 (or under Thunderbird 3, check "Use secure authentication" for SMTP setup) or retrieve email with "Use secure authentication" checked for IMAP/POP
Actual Results:  
Thunderbird crashes.

Expected Results:  
Thunderbird performs GSSAPI SASL negotiation and sends email or retrieves email.

http://crash-stats.mozilla.com/report/index/40d6d04c-6423-47b1-898d-6a8a62090820 is a crash of 3.0b3 under Vista using SSPI to login to IMAP.
so, your description indicates it should be that bug, but your crash says it's totally unrelated.

Signature	strcat
UUID	40d6d04c-6423-47b1-898d-6a8a62090820
Time 	2009-08-20 16:55:07.232911
Uptime	3
Product	Thunderbird
Version	3.0b3
Build ID	20090715140311
Branch	1.9.1
OS	Windows NT
OS Version	6.0.6002 Service Pack 2
CPU	x86
CPU Info	GenuineIntel family 15 model 6 stepping 4
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x0
User Comments	using sspi to log into IMAP
Processor Notes 	
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	mozcrt19.dll 	strcat 	strcat.asm:178
1 	thunderbird.exe 	nsImapFlagAndUidState::AddUidCustomFlagPair 	mailnews/imap/src/nsImapFlagAndUidState.cpp:365
2 	thunderbird.exe 	nsImapServerResponseParser::msg_fetch 	mailnews/imap/src/nsImapServerResponseParser.cpp:1385
3 	thunderbird.exe 	nsImapServerResponseParser::response_data 	mailnews/imap/src/nsImapServerResponseParser.cpp:756
4 	thunderbird.exe 	nsImapServerResponseParser::ParseIMAPServerResponse 	mailnews/imap/src/nsImapServerResponseParser.cpp:243
5 	thunderbird.exe 	nsImapProtocol::ParseIMAPandCheckForNewMail 	mailnews/imap/src/nsImapProtocol.cpp:1853
6 	thunderbird.exe 	nsImapProtocol::FetchMessage 	mailnews/imap/src/nsImapProtocol.cpp:3416
7 	thunderbird.exe 	nsImapProtocol::ProcessMailboxUpdate 	mailnews/imap/src/nsImapProtocol.cpp:3917
8 	thunderbird.exe 	nsImapProtocol::ProcessSelectedStateURL 	mailnews/imap/src/nsImapProtocol.cpp:2819
9 	thunderbird.exe 	nsImapProtocol::ProcessCurrentURL 	mailnews/imap/src/nsImapProtocol.cpp:1709
10 	thunderbird.exe 	nsImapProtocol::ImapThreadMainLoop 	mailnews/imap/src/nsImapProtocol.cpp:1360
11 	thunderbird.exe 	nsImapProtocol::Run 	mailnews/imap/src/nsImapProtocol.cpp:1059
12 	xpcom_core.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
13 	xpcom_core.dll 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:227
14 	xpcom_core.dll 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:254
15 	nspr4.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:426
16 	nspr4.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:122
17 	mozcrt19.dll 	_callthreadstartex 	threadex.c:348
18 	mozcrt19.dll 	_threadstartex 	threadex.c:326
19 	kernel32.dll 	BaseThreadInitThunk 	
20 	ntdll.dll 	__RtlUserThreadStart 	
21 	ntdll.dll 	_RtlUserThreadStart
Component: General → Networking: IMAP
Keywords: crash
Product: Thunderbird → MailNews Core
QA Contact: general → networking.imap
Summary: Crash when using SSPI and joined to AD under Vista or later → Crash when using SSPI and joined to AD under Vista or later [@ strcat - nsImapFlagAndUidState::AddUidCustomFlagPair]
Version: unspecified → 1.9.1 Branch
Yes, I agree that the backtrace doesn't match the bug description; however if the bug is the same as bug 505971, I would assume the use of SSPI, in and of itself, could cause Thunderbird to become unstable and blow up at some later point.

I don't have immediate access to a Vista machine, but I'll try to get some additional crash reports submitted in the next few days.
I got a crash report generated under Windows Server 2008 R2 (equivalent to Windows 7) when trying to use SMTP with SSPI enabled and while logged into an AD account:

http://crash-stats.mozilla.com:80/report/index/e148a46f-4282-42fb-8604-6424f2090821
This crash occurred when using TB 3b3 under Win Server 2008 R2 when gssapi-enabled imap server was being accessed while logged into an AD account with network.auth-use-sspi=true:

http://crash-stats.mozilla.com:80/report/index/7fb38a9e-a2f6-4082-8a2f-df43d2090821?p=1
Karl your problem is described in bug 505971
marking blocker - I think bug 505971 is the same but I'll do the work here for now.
Assignee: nobody → bienvenu
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b4
Whiteboard: [no l10n impact]
With much help from Karl (thx!) I've got an environment to reproduce this bug. The issue seems to be in the Microsoft secur32.dll (which might help explain why this bug seems to be new to Vista/Windows 7). When we call DecryptMessage on a token, we get back a success error code and a valid unwrapped token len,  but trying to free the resulting unwrapped token crashes. According to the comments in the code, we're only unwrapping the token to verify that the signature was correct (we're not doing security layers).

Since we're not using the unwrapped token, and don't care what it is, commenting out the freeing of the memory makes things work fine. I suppose it's possible that we should be freeing the memory differently, but an other possibility is that the arguments we're passing to DecryptMessage might be exposing a bug in secur32.dll. I suppose it's also remotely possible that we shouldn't be using secur32.dll - this code is fairly old. Cc'ing Simon who might just know off the top of his head what's going wrong.


Here's the call to DecryptMessage:

http://mxr.mozilla.org/comm-central/source/mozilla/extensions/auth/nsAuthSSPI.cpp#407

and here's the free call:

http://mxr.mozilla.org/comm-central/source/mozilla/extensions/auth/nsAuthSASL.cpp#135

I'm marking security sensitive for now, just because it's a potential heap corruption, though I don't know if it's exploitable.
Group: core-security
In the MSDN sample code, they don't say anything about having the free the result data, but I don't know if that's just assumed or not.
they do say that the data is decrypted in place, from the input buffer, which implies we wouldn't have to free it.

http://msdn.microsoft.com/en-us/library/aa375215%28VS.85%29.aspx

Our code looks to be straight from the sample code, except that the sample code doesn't free the memory.
(In reply to comment #7)
> With much help from Karl (thx!) I've got an environment to reproduce this bug.
> The issue seems to be in the Microsoft secur32.dll (which might help explain
> why this bug seems to be new to Vista/Windows 7). 
Therefore seems not dupe at all of bug 505971.
why do you say that? Isn't that SSPI GSSAPI as well? Or are you saying you're having the issue with XP?
(In reply to comment #11)
> Or are you saying you're having the issue with XP?
Exactly, there crash reports from Windows XP, tested on Windows 7 and XP.
I have no technical reason to believe that the issue doesn't happen on XP, other than some folks say they haven't seen it on XP. If you can do your own builds, I can tell you what line to comment out and see if the problem goes away on XP.
If you are talking about exactly this bug, I'm not aware any problems such as these described by reporter. Only those reported in bug 505971. Still I can make my own builds (at least give a try)
the general problem is a crash after doing SSPI GSSAPI authentication due to heap corruption. bug 505971 sounds like it matches. Given the nature of heap corruption, and this bug in particular, I would expect the actual crash site to differ between builds and OS's.

I haven't tried this myself, but a simple fix would be to find this code 

414     if (SEC_SUCCESS(rc)) {
415         *outToken = ib[1].pvBuffer;
416         *outTokenLen = ib[1].cbBuffer;

and just change the *outToken = ... line to *outToken = NULL;

which is here:

http://mxr.mozilla.org/comm-central/source/mozilla/extensions/auth/nsAuthSSPI.cpp#407
Attached patch proposed fix (obsolete) — — Splinter Review
I think this affects Firefox, in theory, at least, since the code is shared with Firefox. We very much would like this for the next beta of TB 3, which has code freeze in about 2 weeks. Someone other than me should probably figure out if it affects Firefox in actual fact, but I think this fix should go into whatever versions of gecko Thunderbird ships on, at least. Since I only have the version of gecko that Thunderbird uses, once this is reviewed, it would be helpful if someone else could land it on the trunk...
Attachment #397375 - Flags: superreview?(neil)
Attachment #397375 - Flags: review?(simon)
Attachment #397375 - Flags: approval1.9.1.3?
Attachment #397375 - Flags: approval1.8.1.next?
Comment on attachment 397375 [details] [diff] [review]
proposed fix

1.9.1.3 is frozen.
Attachment #397375 - Flags: approval1.9.1.3? → approval1.9.1.4?
Comment on attachment 397375 [details] [diff] [review]
proposed fix

>
>     else
>         nsMemory::Free(ib[1].pvBuffer);

This should free ib[0].pvBuffer (which we've allocated), not ib[1].pvBuffer, which may not be a pointer to the start of an allocated block.

S.
Attachment #397375 - Flags: review?(simon) → review-
(In reply to comment #15)
David, I've tested this small suggestion and it appears works fine and resolving bug 505971 issue. I've test this only on Windows 7.
And its working just fine on XP too
Attached patch fix addressing comments (obsolete) — — Splinter Review
Attachment #397375 - Attachment is obsolete: true
Attachment #397446 - Flags: superreview?(neil)
Attachment #397446 - Flags: review?(simon)
Attachment #397375 - Flags: superreview?(neil)
Attachment #397375 - Flags: approval1.9.1.4?
Attachment #397375 - Flags: approval1.8.1.next?
(In reply to comment #16)
> I think this affects Firefox, in theory, at least, since the code is shared
> with Firefox. We very much would like this for the next beta of TB 3, which has
> code freeze in about 2 weeks. Someone other than me should probably figure out
> if it affects Firefox in actual fact, but I think this fix should go into
> whatever versions of gecko Thunderbird ships on, at least. Since I only have
> the version of gecko that Thunderbird uses, once this is reviewed, it would be
> helpful if someone else could land it on the trunk...

Well I can't crash Firefox, probably something special need to do but just surfing pages with SSPI authentication enabled does nothing.
(In reply to comment #8)
> In the MSDN sample code
Got a link to that? I'm finding it really hard to work out what Unwrap is trying to achieve and reading that is my last resort.
http://msdn.microsoft.com/en-us/library/aa375215%28VS.85%29.aspx describes what DecryptMessage does. In particular, note that it says it decrypts in place, which means that the outbuffer pointer it returns is a pointer into the input buffer, which is also what we see in the debugger. Simon says he could imagine the pointer not being at the start of the buffer, which is why we have the code that copies the output buffer if the pointer is not at the start of the buffer.
Whiteboard: [no l10n impact] → [no l10n impact][has patch for review]
Comment on attachment 397446 [details] [diff] [review]
fix addressing comments

I'm happy that this is theoretically correct, but I don't have a Windows development system handy to test it on.
Attachment #397446 - Flags: review?(simon) → review+
I've gone to rather great pains to test it, so I'm confident it works. Perhaps Nikolay could be prevailed upon to test this latest patch, instead of the hack I suggested to him earlier...
Works good, no problem so far. Only problem is my builds are with memory leak so can't test if this code leaks anything. Thanks for fix.
Nikolay, I've kicked off a try server build with the patch that you can try, if you want - it shouldn't have any memory leak problems.

http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry

However, I'm not really sure you'd be able to detect any leaks if there were any - there are about 10 bytes of memory allocated in this code, per connection to the imap server.
Thanks David, if that just 10 bytes it really difficult to test manually. So it not worth trying.
btw
Try server actually reports - fail to patch, if it really matters
unable to find 'extensions/auth/nsAuthSSPI.cpp' for patching
1 out of 1 hunks FAILED -- saving rejects to file extensions/auth/nsAuthSSPI.cpp.rej
extensions\auth\nsAuthSSPI.cpp: The system cannot find the file specified
oh, yeah, it's a moz-central, not comm-central patch. But yeah, if it's just for you to try to see if there's any memory leaks, it's not going to help you.
(In reply to comment #24)
> http://msdn.microsoft.com/en-us/library/aa375215%28VS.85%29.aspx describes what
> DecryptMessage does. In particular, note that it says it decrypts in place,
> which means that the outbuffer pointer it returns is a pointer into the input
I don't see anything about inbuffer and outbuffer pointers...
pMessage [in, out]

    A pointer to a SecBufferDesc structure. On input, the structure references one or more SecBuffer structures. One of these may be of type SECBUFFER_DATA. That buffer contains the encrypted message. The encrypted message is decrypted in place, overwriting the original contents of its buffer.

This matches what I see in the debugger as well.
(In reply to comment #33)
> pMessage [in, out]
> 
>     A pointer to a SecBufferDesc structure. On input, the structure references
> one or more SecBuffer structures. One of these may be of type SECBUFFER_DATA.
> That buffer contains the encrypted message. The encrypted message is decrypted
> in place, overwriting the original contents of its buffer.
Right, but what I'm not following is why we pass in two structures, one of type SECBUFFER_DATA, and expect to read the output from the other structure.
If you scroll to the end of this page - http://msdn.microsoft.com/en-us/library/aa380496%28VS.85%29.aspx you'll see that the sample code MS provides does the same thing. We don't actually use the result (the caller throws it away - it just wants to check that it was decrypted successfully - if we try to turn on security levels, then we would not throw the data away, if I understand correctly).
The 1.9.x moz-central branch this needs to go into is freezing in a few days, and we'd really like to get this in before it freezes - let me know if there's anything else Simon or I can do to help explain this (the MS doc is not extremely helpful).
Whiteboard: [no l10n impact][has patch for review] → [no l10n impact][needs sr neil]
Comment on attachment 397446 [details] [diff] [review]
fix addressing comments

>+            memmove(*outToken, ib[1].pvBuffer, ib[1].cbBuffer);
Thanks, that makes sense now. Nit: use memcpy (or even nsMemory::Clone) here.
(I assume a previous revision of the patch memmoved from ib[1] to ib[0].)
Attachment #397446 - Flags: superreview?(neil) → superreview+
OK, thx, I'll attach a new patch in a bit, and request moz-central check-in approval.
Whiteboard: [no l10n impact][needs sr neil] → [no l10n impact]
Attached patch patch addressing Neil's comment — — Splinter Review
carrying forward r/sr, requesting moz-central checkin approval.
Attachment #397446 - Attachment is obsolete: true
Attachment #399064 - Flags: superreview+
Attachment #399064 - Flags: review+
Attachment #399064 - Flags: approval1.9.2?
Attachment #399064 - Flags: approval1.9.1.4?
dveditz, I'm not clear what all moz-central branches this should land in. I believe it's a bug that goes a long way back. It may not affect Firefox but it definitely affects Thunderbird and SeaMonkey.  So I guess I need at minimum to get this into TB 2.0x next and TB 3.0b4...
Attachment #399064 - Flags: approval1.8.1.next?
Needs to land on trunk and 1.9.2 first at least so it get some baking...
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/3a1e9ee72015

Leaving this open whilst we bake and get approvals for branch so that it doesn't get lost from the TB 3 blocking list.
Whiteboard: [no l10n impact] → [no l10n impact][baking on trunk][awaiting branch approvals]
Whiteboard: [no l10n impact][baking on trunk][awaiting branch approvals] → [no l10n impact][baking on trunk][awaiting branch approvals][sg:critical]
Can we get 1.9.2 approval on this?

I know it hasn't baked for long, but we are looking to cut Thunderbird 3b4 on Tuesday so it would be good if we could get it in to 1.9.2 before the weekend so we have some hope of getting it into 1.9.1 by Tuesday...
This isn't in a component that any 1.9.2 drivers likely look at. You should ping one or two individually for approval.
pinging dveditz...
dveditz isn't a 1.9.2 driver. See the following page for a list of drivers for various releases.

  https://wiki.mozilla.org/Releases/Drivers
bsmedberg, ping for 1.9.2 approval. afaik, Firefox doesn't seem to use this code, but Thunderbird needs this fix for our b4 build.
Comment on attachment 399064 [details] [diff] [review]
patch addressing Neil's comment

a192=beltzner
Attachment #399064 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 399064 [details] [diff] [review]
patch addressing Neil's comment

Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #399064 - Flags: approval1.9.1.4? → approval1.9.1.4+
Checked into the 1.9.1 branch:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4aac246ba59f
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [no l10n impact][baking on trunk][awaiting branch approvals][sg:critical] → [no l10n impact][baking on trunk][sg:critical]
Whiteboard: [no l10n impact][baking on trunk][sg:critical] → [no l10n impact][sg:critical]
Can the reporter or someone with a repro environment try this with the nightly 1.9.1 Thunderbird build and see if the issue is actually fixed now?

ftp://ftp.mozilla.org/pub/thunderbird/nightly/2009-09-15-03-comm-1.9.1/
It is fixed
Tested on Windows Server 2008 R2 64-bit, Windows 7 32-bit, Window Vista, and Windows XP (all joined to an MS Active Directory). Thanks to all for working on this.
Status: RESOLVED → VERIFIED
Thanks. Marking as verified for 1.9.1.4.
Keywords: verified1.9.1
who does 1.8.1.next approvals? I'd like to get this fix in the next 2.0x TB build...
Sam, Dan, and I do them.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next?
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Comment on attachment 399064 [details] [diff] [review]
patch addressing Neil's comment

Looks like this never landed on the 1.9.0 branch. When you land on the 1.8 branch in CVS please go ahead and land on CVE HEAD as well.
Attachment #399064 - Flags: approval1.9.0.18+
Attachment #399064 - Flags: approval1.8.1.next?
Attachment #399064 - Flags: approval1.8.1.next+
dveditz:  thanks for the heads up!
I've landed this on the 1.8 branch  but I don't have a CVS head tree pulled right now. I'll try to get one built at some point...
Keywords: fixed1.8.1.24
Whiteboard: [no l10n impact][sg:critical] → [sg:critical][no l10n impact][needs 1.9.0 landing]
Comment on attachment 399064 [details] [diff] [review]
patch addressing Neil's comment

removing 1.9.0.18 approval, time to ship. Please re-request approval if this is still wanted by someone maintaining mailnews on the 1.9.0 branch
Attachment #399064 - Flags: approval1.9.0.18+ → approval1.9.0.18-
This is now checked into 1.8.1 for Thunderbird 2.0.0.24. If possible, we need the reporter or someone else with Berkeley e-mail access to verify that it is fixed in TB 2.0.0.24. This can be done using a nightly build from ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-mozilla1.8/.
I can confirm correct working with SSPI enabled on Win7 Enterprise 32-bit joined to AD and running TB version 2.0.0.24pre (20100211) for both IMAP and SMTP. Thanks for helping to get this addressed for our campus.
Thx, Karl.
Thanks!

Marking as verified for 1.8.1.24.
Group: core-security
Crash Signature: [@ strcat - nsImapFlagAndUidState::AddUidCustomFlagPair]
You need to log in before you can comment on or make changes to this bug.