443284 - Firefox 3 crashes when trying to login to an ActivIdentity/ActivClient smartcard using acpkcs211.dll

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Dani Church]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

Firefox 3 crashes whenever I try to use a CAC smartcard with the ActivClient 6.1 drivers acpkcs211.dll or acpkcs201.dll.  There is no trouble with the drivers acpkcs201-ns.dll or acpkcs201-en6.dll, and there was no trouble using Firefox 2 with acpkcs201.dll and acpkcs211.dll.  It should be noted that the non-working drivers always defer to the builtin ActivClient PIN entry (Fx2 would pop up an irrelevant password-entry dialog, then proceed to AC PIN entry no matter what was entered) while the working drivers pop up a password-entry dialog and actually use the PIN entered there to log into the smartcard, bypassing the AC PIN entry dialog.  Under Fx3, the non-working drivers pop up a brief dialog box saying something to the effect of "Now login to your security device, the method may differ depending on the device" (I don't know the exact wording, it flashes by too quickly to tell) before it crashes.

Reproducible: Always

Steps to Reproduce:
1. Load acpkcs211.dll (usually found in %WINDIR%\system32) as a security module.
2. Insert a smartcard into the reader.
3. Click Log In in the security manager, for that smartcard.  Firefox crashes.

Comment 1

[Matthias Versen [:Matti]]

Please provide a crash ID from about:crashes (enter it as url) if you submitted a crash report with the Mozilla crash reporter.

Comment 2

[Dani Church]

Unfortunately the crash reporter doesn't seem to work, it just says "There was a problem submitting your report".  I checked with Wireshark and it doesn't even send out any network traffic.  Is there any way to retrieve the crashdump manually?

Some corrections and additions to the initial report:

Firefox actually does initiate the AC PIN entry dialog, and if AC has not already authenticated itself to the smartcard, it will display.  The Firefox dialog that I was missing is titled "Protected Token Authentication" and reads "Please authenticate to the token. Authentication method depends on the type of your token."  Firefox will not crash until the AC PIN entry dialog is closed, but it will crash whether PIN entry is successful or unsuccessful (i.e. Cancel is clicked).

Comment 3

[Matthias Versen [:Matti]]

There should be a submit log under profiles\crashreporter\submit.log, is there an error listed ?
( http://kb.mozillazine.org/Profile_folder)

You can also create a stack trace using this instructions :
http://developer.mozilla.org/en/docs/How_to_get_a_stacktrace_with_WinDbg

Comment 4

[timeless]

I sent an email to	[e-mail]

Could you please coordinate with the reporter of this bug (preferably in the bug report, but privately is probably OK). If you'd like to coordinate via me, this is possible....

Comment 5

[Peter Koch]

Hi,

I have developed a PKCS#11-library for a german signature card. The library did work well with Firefox 2.0.0.15. It now crashes with Firefox 3.0 immediately after C_Login() returns.

Since I can change the sourcecode of my library I can confirm that the this crash is caused by the CKF_PROTECTED_AUTHENTICATION_PATH flag which I set in C_GetTokenInfo().

Without the CKF_PROTECTED_AUTHENTICATION_PATH flag Firefox 2 and 3 ask for a PIN an feed this PIN into the pPin parameter of C_Login(). With the CKF_PROTECTED_AUTHENTICATION_PATH flag Firefox 2 still asks for a PIN but calls C_Login() with pPin==NULL. Firefox 3 does not asks for the PIN but tells the user use the token authentication mechanism. While the dialog is displayed C_Login() is called with pPin==NULL. Immediately after C_Login() returns with CKR_OK Firefox crashs. I'm not sure wether the crash happens while the dialog is still displayed or after the display was removed from the screen.

I reported a crash with ID ba33a7e1-4b91-11dd-b098-001a4bd43ed6.

If I can help in any way - let me know.

Peter Koch

Comment 6

[timeless]

For now, I'm going to assume that the NSS team can help. Note that the report is not yet available from our crash reporter, it should be soon.

peter: it'd be great if you could use http://developer.mozilla.org/en/docs/How_to_get_a_stacktrace_with_WinDbg as it'd enable you to have symbols for our side (as well as symbols for your side).

you can also use:
http://developer.mozilla.org/en/docs/Using_the_Mozilla_source_server
to get matching sources....

Right now, I really want a stack trace :), your explanation as to what's happening is greatly appreciated.

(For me, it'd be helpful to know what the expected behaviors of these things [function calls/arguments] are, I could read the docs or api or sources, but I'm not dedicated to working on pkcs11, whereas probably the people who are already know those things, and I have other things to do.)

Comment 7

[Kai Engert [:KaiE:]]

Support for CKF_PROTECTED_AUTHENTICATION_PATH was added for Firefox 3, using a contributed patch.

Not prompting for a password with such tokens was the intended behavior.

I'm not an expert of the pkcs#11 interface, so I don't know whether the call to C_Login with pPin==NULL is right or wrong.

I'm adding the contributor of the feature to the cc list.

Comment 8

[Peter Koch]

Kai, calling C_Login witn pPin==NULL is OK. That's the way a caller of C_Login() signals that he doesn't have a PIN and the token should do it's authentication procedure on its own.

Firefox 3 always asks for a pin BEFORE C_Login() is called. The pin-dialog happens with Firefox 2 even if the token supports a protected authentication path an C_Logon() is therfore called without a PIN. In this case Firefox just ignores the PIN that a user entered and calls C_Login() with pPin==NULL, which confuses most users.

Firefos 3 only asks for a PIN if the token does NOT support a protected authentication path. If the token DOES supports a protected authentication path then Firefox 3 will NOT ask for a pin BEFORE C_Login() is called but displays a message WHILE C_Login() is called with pPin==NULL. That's perfect behaviour if only Firefox 3 would not crash after C_Login() returns.

My guess is that Firefox 3 reates a seperate thread in order to display the message and call C_Login() at the same time. And this might cause the problems.

I will try WinDbg as soon as possible and create a stack trace.

Comment 9

[Dani Church]

Sorry it took so long to get back to this.  After getting WinDbg and some symbols and sources, I finally managed to track down what looks like a usable stack trace:

0:021> kp
ChildEBP RetAddr  
04fff9b4 7c90e89a ntdll!KiFastSystemCallRet
04fff9b8 7c81cd96 ntdll!ZwTerminateProcess+0xc
04fffab4 7c81cdee kernel32!_ExitProcess+0x62
04fffac8 6000179e kernel32!ExitProcess+0x14
04fffad0 6065b07e MOZCRT19!__crtExitProcess(int status = 1610619918)+0x2e [e:\fx19rel\winnt_5.2_depend\mozilla\obj-fx-trunk\memory\jemalloc\src\crt0dat.c @ 683]
04fffb0c 60001c0e xul!PrepareAndDispatch(class nsXPTCStubBase * self = 0x00000003, unsigned int methodIndex = 1, unsigned int * args = 0x00000000, unsigned int * stackBytesToPop = 0x6000e5b1)+0xe7 [e:\fx19rel\winnt_5.2_depend\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp @ 114]
04fffb3c 6065b07e MOZCRT19!_exit(int code = 1617277157)+0xe [e:\fx19rel\winnt_5.2_depend\mozilla\obj-fx-trunk\memory\jemalloc\src\crt0dat.c @ 406]
04ffff04 6065b0e5 xul!PrepareAndDispatch(class nsXPTCStubBase * self = 0x056286c0, unsigned int methodIndex = 3, unsigned int * args = 0x04ffff2c, unsigned int * stackBytesToPop = 0x04ffff1c)+0xe7 [e:\fx19rel\winnt_5.2_depend\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp @ 114]
04ffff20 6080b938 xul!SharedStub(void)+0x16 [e:\fx19rel\winnt_5.2_depend\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp @ 142]
04ffff44 6080b980 xul!nsProtectedAuthThread::Run(void)+0x6e [e:\fx19rel\winnt_5.2_depend\mozilla\security\manager\ssl\src\nsprotectedauththread.cpp @ 186]
04ffff4c 600cbba9 xul!nsProtectedAuthThreadRunner(void * arg = 0x600cde1d)+0x9 [e:\fx19rel\winnt_5.2_depend\mozilla\security\manager\ssl\src\nsprotectedauththread.cpp @ 52]
04ffff6c 600cde1d nspr4!_PR_NativeRunThread(void * arg = 0x042996c0)+0x169 [e:\fx19rel\winnt_5.2_depend\mozilla\nsprpub\pr\src\threads\combined\pruthr.c @ 458]
04ffff74 60002c28 nspr4!pr_root(void * arg = 0x60002cb6)+0xd [e:\fx19rel\winnt_5.2_depend\mozilla\nsprpub\pr\src\md\windows\w95thred.c @ 122]
04ffffac 60002cb6 MOZCRT19!_callthreadstartex(void)+0x48 [e:\fx19rel\winnt_5.2_depend\mozilla\obj-fx-trunk\memory\jemalloc\src\threadex.c @ 348]
04ffffb4 7c80b683 MOZCRT19!_threadstartex(void * ptd = 0x00000000)+0x66 [e:\fx19rel\winnt_5.2_depend\mozilla\obj-fx-trunk\memory\jemalloc\src\threadex.c @ 326]
04ffffec 00000000 kernel32!BaseThreadStart+0x37

Based on the single-stepping I was doing through various parts of the process, it looks like the crash happens after the return from C_Login(), but before the authentication thread finishes.

And then, after going through all that, I tried using the Crash Reporter again, just for the heck of it, and what do you know, it worked.  Here's the URL to the report: http://crash-stats.mozilla.com/report/index/fd0f824f-4df3-11dd-9177-001cc45a2c28

Comment 10

[timeless]

crash reporter is currently hosed, 

http://blog.mozilla.com/webdev/2008/07/07/socorro-delays/

so please for the time being just track this down using windbg, it sounds like you've gotten that all working, which is great :).

anyway, here's my guess:
161     nsIObserver *observer = nsnull;
51     nsCOMPtr<nsIObserver> mStatusObserver;
177         observer = mStatusObserver;
180     mStatusObserver = nsnull;
186         observer->Observe(nsnull, "operation-completed", nsnull);

Comment 11

[timeless]

Attachment: don't kill object before using it...

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 328829 [details] [diff] [review]
don't kill object before using it...

r=kaie

Thanks timeless!

Comment 13

[Kai Engert [:KaiE:]]

Comment on attachment 328829 [details] [diff] [review]
don't kill object before using it...

Dan, this is a one-line patch, obvious correctness fix, could you please review? Thanks!

Comment 14

[Dani Church]

This sounds right; it's around line 186 that the not-a-crash seems to happen, and I get a bunch of "memory inaccessible" errors in WinDbg's Locals window that weren't there before.  If I skip line 180 with its implicit unref/destroy, it gets a lot further before the inevitable crash, and actually manages to close the Protected Auth window first.  If someone can build a copy of xul.dll with this patch against either release or nightly, I can test it for correctness.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Is this relevant?
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11slot.c&rev=1.93#1108

Comment 16

[Dani Church]

(In reply to comment #15)
> Is this relevant?
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11slot.c&rev=1.93#1108
> 

I don't think so; this version of ActivClient actually does have a protected authentication path.  The company that makes it is called ActivIdentity now, not ActivCard, so the test at line 1320 is false and slot->protectedAuthPath does NOT get disabled at 1111.

Comment 18

[Dani Church]

Attachment: Don't try to free string literals

PK11_DoPassword in pk11auth.c expects that pk11_GetPassword (which eventually calls ShowProtectedAuthPrompt in nsNSSCallbacks.cpp) returns a newly-allocated string that it can free later, but ShowProtectedAuthPrompt assigns a string literal to the return value.  Perhaps this should be using something besides strdup(), like maybe PORT_Strdup() or something, but I don't know the source well enough.

Comment 19

[Dani Church]

I finally managed to build a working copy from the Fx3.0 source tarball (I can't access CVS through the firewall, I'm afraid), and I can confirm that the two patches here are enough to fix the bug.

Comment 20

[Peter Koch]

Sorry if this is a stupid question - I would like to test wether my PKCS#11 library still crashes Firefox 3 when CKF_PROTECTED_AUTHENTICATION_PATH is set or not. So I tried the latest nightly Firefox build (firefox-3.1a1pre.en-US.win32.installer.exe). The problem is still there.

How can I find out wether the latest nightly build contains these patches or not?

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Peter, 
I suggest you wait until this bug is marked "fixed", or until some comment
reports that a patch has been committed, before expecting that nightly 
builds will contain any patches.

Comment 22

[Samuel Sidler (old account; do not CC)]

Comment on attachment 328829 [details] [diff] [review]
don't kill object before using it...

Please request approval after this gets sr and lands on mozilla-central.

Comment 23

[Dani Church]

Can we get some action on this?  This is a three-line fix and has been open for more than a month.  When can I expect that someone will look at these patches?

Comment 24

[Samuel Sidler (old account; do not CC)]

We won't block 1.9.0.2 on this bug.

Dan, have you had time to look at this patch?

Comment 25

[Kai Engert [:KaiE:]]

Comment on attachment 328829 [details] [diff] [review]
don't kill object before using it...

Maybe Bob could have a look at this one line patch, so we don't need to wait for Dan.

Comment 26

[Robert Relyea]

Comment on attachment 328829 [details] [diff] [review]
don't kill object before using it...

r+ assuming nsCOMPtr always initializes the pointer to NULL in it's constructure.

bob

Comment 27

[Dani Church]

Just as a note, applying the patch in attachment 328829 [details] [diff] [review] without the one in
attachment 329708 [details] [diff] [review] still causes an unavoidable crash when attempting a protected
authentication path (but at a different place in the code).

Comment 28

[Peter Koch]

Dani Church sent me a version of FireFox3 with BOTH patches applied. I tested them with my PKCS#15-library with all kind of combinations of CKF_PROTECTED_AUTHENTICATION_PATH, CKF_LOGIN_REQUIRED and CKF_WRITE_PROTECTED. In all cases FireFox3 behaves as expected. In particular with CKF_PROTECTED_AUTHENTICATION_PATH and CKF_LOGIN_REQUIRED FireFox3 does NOT ask for a PIN before C_Login is called but displays a message in a seperate thread and calls C_Login with pPIN==NULL. Immediately after C_login returnes (successfull or not) the message is removed without crashing FireFox.

Comment 29

[Kai Engert [:KaiE:]]

Comment on attachment 328829 [details] [diff] [review]
don't kill object before using it...

clearing unnecessary review request

Comment 30

[Kai Engert [:KaiE:]]

Comment on attachment 329708 [details] [diff] [review]
Don't try to free string literals

Dani, thank you very much for finding this issue and proposing the patch?

You're on the right track, I've only one change request.

We should make sure our memory allocation is consistent. I can see that other code path use our memory helper functions ToNewUTF8String. Here we should use ToNewCString.

Comment 31

[Kai Engert [:KaiE:]]

Attachment: Patch v3

Comment 32

[Robert Relyea]

Comment on attachment 333390 [details] [diff] [review]
Patch v3

r+

Comment 33

[Kai Engert [:KaiE:]]

Thanks again to timeless and Dani Church.

I've commited the fix to mozilla-central.
5450ca25f3aa

marking fixed.

Comment 34

[Kai Engert [:KaiE:]]

Note for 1.9.0.2 drivers:
- both patches are needed
- I haven't tested myself

Comment 35

[Marcia Knous [:marcia]]

Dani: Can you please verify the fix using the latest nightly from ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.9.0/? Thanks.

Comment 36

[Samuel Sidler (old account; do not CC)]

Actually, this needs to be verified with ftp://ftp.mozilla.org/pub/firefox/nightly/latest-trunk builds.

This hasn't yet landed on 1.9.0.x. I'd like to get verification before we take this in 1.9.0.x.

Comment 37

[Samuel Sidler (old account; do not CC)]

Any update on getting this verified?

Comment 38

[Samuel Sidler (old account; do not CC)]

Comment on attachment 333390 [details] [diff] [review]
Patch v3

I'm pushing these approval requests out to 1.9.0.3. This needs to get verified before we'll take it on a stable branch.

Comment 40

[Daniel Veditz [:dveditz]]

Comment on attachment 328829 [details] [diff] [review]
don't kill object before using it...

clearing approval requests until questions are answered.

Comment 41

[Kai Engert [:KaiE:]]

Dani Church, a clarification for you:

This bug has not yet been fixed in Firefox 3.
It has only been fixed in the development versions towards the future version Firefox 3.1

The people controlling the patches for Firefox 3.0.x update releases are conservative and careful.

In order to get this patch into Firefox 3.0.x, they require that someone uses a Firefox 3.1 test release and tests whether this bug is now indeed fixed.

Dani, would you be able to perform that test in your environment, and report back here whether the bug is fixed for you?

Once have done confirmed it worked (verified) then we can ask the drivers again for Firefox 3.0.x inclusion...

Comment 42

[Kai Engert [:KaiE:]]

Usually running through this code depends on special hardware, which I don't have access to.

I think I've found a way to verify that we no longer crash, although this still requires a developer build.

In function PK11PasswordPrompt I've changed 

  if (PK11_ProtectedAuthenticationPath(slot))
    return ShowProtectedAuthPrompt(slot, ir);

to always go through the protected auth path

    return ShowProtectedAuthPrompt(slot, ir);


With the latest trunk (mozilla-central) code, I do not crash.

If I back out the patch, Firefox crashes with signal 11 (SIGSEV) while trying to free memory.


In theory we could add a new preference to the code, a testing.security.force-protected-auth that QA could use, that would have the same effect as my code change.

Comment 43

[Kai Engert [:KaiE:]]

I hope that Dani will be able to help us and give feedback.

Should he be unable to, I propose that my comment 42 is used as sufficient verification to approve the patch for Firefox 3.0.x

Comment 44

[Kai Engert [:KaiE:]]

I filed bug 459971 and provided a patch for the idea from comment 42.

Comment 45

[Dani Church]

Unfortunately, I'm not working in the same environment anymore, and I no longer have access to the smartcard reader.  You'll need to verify the patch some other way.

Comment 46

[Brandon Meyer]

Hello, I am able to provide the verification needed. I installed 3.1 Beta 1 and was successfully able to load in my certificates using C:\WINDOWS\system32\acpkcs211.dll without the crash listed in this bug.

However, when attempting to access a page that requires a certificate I am not prompted with a choice. This could be another bug, but maybe its related.

Comment 47

[Mike Connor [:mconnor]]

Comment on attachment 328829 [details] [diff] [review]
don't kill object before using it...

approved for 1.9.0.5, its a weird edgecase, but the patches seem quite straightforward and its been baking for quite some time.

Comment 48

[Kai Engert [:KaiE:]]

both patches checked in to cvs for 1.9.0.5

Checking in src/nsNSSCallbacks.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp,v  <--  nsNSSCallbacks.cpp
new revision: 1.71; previous revision: 1.70
done
Checking in src/nsProtectedAuthThread.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsProtectedAuthThread.cpp,v  <--  nsProtectedAuthThread.cpp
new revision: 1.3; previous revision: 1.2
done

Comment 50

[Al Billings [:abillings - ex-MoCo]]

Brandon Meyer, can you verify that this is fixed in the Firefox 3.0.5 release as well?

You can use the official builds (if Windows builds are available when you see this) at ftp://ftp.mozilla.org/pub/firefox/nightly/3.0.5-candidates/build1/. Otherwise, you can use the nightly from 12/1 at ftp://ftp.mozilla.org/pub/firefox/nightly/2008-12-01-05-mozilla1.9.0/.

Comment 51

[Brandon Meyer]

Yes, I can verify. I installed 3.0.5 as was able to load my certificates using C:\WINDOWS\system32\acpkcs211.dll

Comment 52

[timeless]

recording verified based on comment 46 and comment 51

Comment 53

[Al Billings [:abillings - ex-MoCo]]

Thanks, Brandon!