Closed Bug 321598 Opened 19 years ago Closed 19 years ago

Double memory free in nsIX509::getRawDER when called from JavaScript

Categories

(Core :: Security: PSM, defect)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: fernando, Assigned: KaiE)

References

Details

(Keywords: fixed1.8.1, verified1.8.0.4, Whiteboard: [sg:moderate])

Attachments

(3 files)

User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051224 Debian/1.5.dfsg-3 Firefox/1.5 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051224 Debian/1.5.dfsg-3 Firefox/1.5 While i call the getRawDER function of nsIX509Cert (security/manager/ssl/public) it freeze in all mozilla browser and firefox (except 1.5 [windows/linux] and mozilla 1.8b1 [linux] no extension installed success, opening other bug request for these); I'm call the function in a javascript and native mode, but both freeze the browser. ---------------- JavaScript ---------------- try{ netscape.security.PrivilegeManager.enablePrivilege( "UniversalXPConnect"); ... if(cert instanceof Components.interfaces.nsIX509Cert){ var length = {value:0}; var data=[]; data = cert.getRawDER(length);// <-- travamento } }catch(err){ alert("Erro: " + err); } ----------------- C++ ----------------- /* void getCertBase64 (in nsIX509Cert cert, [retval] out string textbase64); */ NS_IMETHODIMP BRTIWebSignatureVerifier::GetCertBase64(nsIX509Cert *cert, char **textbase64) { nsresult rv; PRUint32 length = 0; PRUint8 *data = nsnull; //verifica parametro if(cert == nsnull){ rv = NS_ERROR_INVALID_ARG; goto fim; } //GetRawDER(PRUint32 *length, PRUint8 **data) rv = cert->GetRawDER(&length, &data);// <-- travamento if (rv != NS_OK){ goto fim; } //codifica base64 rv = encode(data, length, textbase64); if (rv != NS_OK){ goto fim; } fim: //if (data) nsMemory::Free(data); return rv; } Reproducible: Always Steps to Reproduce: 1. Open http://www.nerdgroup.org/staff/fernando/html/ns/index-env.html 2. Install the extension in botton page with root user 3. Write any text message in the 'text' area. 4. Click in 'Enviar' button. Actual Results: It freeze the browser. Expected Results: show encrypted message in other text area.
Assignee: nobody → kengert
Component: Security → Security: PSM
Product: Firefox → Core
QA Contact: firefox
Version: unspecified → Trunk
The call getRawDER() of a nsIX509Cert(security/manager/ssl/public) instance freeze Linux machines, on Windows everything works. See javascript line: data = cert.getRawDER(length);// <-- freeze to workaround this problem I have written a code in C++: rv = cert->GetRawDER(&length, &data);// <-- freeze but the error persist. :( Looking in the C++ implementation of nsIX509Cert file nsNSSCertificate.cpp (src/security/manager/ssl/src): NS_IMETHODIMP nsNSSCertificate::GetRawDER(PRUint32 *aLength, PRUint8 **aArray) { nsNSSShutDownPreventionLock locker; <-- freeze probably here if (isAlreadyShutDown()) <-- it cannot be here return NS_ERROR_NOT_AVAILABLE; if (mCert) { *aArray = (PRUint8 *)mCert->derCert.data; *aLength = mCert->derCert.len; return NS_OK; } *aLength = 0; return NS_ERROR_FAILURE; } file nsNSSShutDown.h (src/security/manager/ssl/src) PRBool isAlreadyShutDown() { return mAlreadyShutDown; } file nsNSSShutDown.cpp (src/security/manager/ssl/src) nsNSSShutDownPreventionLock::nsNSSShutDownPreventionLock() { nsNSSActivityState *state = nsNSSShutDownList::getActivityState(); <-- it cannot be here if (!state) return; state->enter(); <-- probably here } file nsNSSShutDown.h (src/security/manager/ssl/src) static nsNSSActivityState *getActivityState() { return singleton ? &singleton->mActivityState : nsnull; } file nsNSSShutDown.cpp (src/security/manager/ssl/src) void nsNSSActivityState::enter() { PR_Lock(mNSSActivityStateLock); <-- probably here, other security component have been not release the lock while (mNSSRestrictedThread && mNSSRestrictedThread != PR_GetCurrentThread()) { PR_WaitCondVar(mNSSActivityChanged, PR_INTERVAL_NO_TIMEOUT); <-- can be here too } ++mNSSActivityCounter; PR_Unlock(mNSSActivityStateLock); } In my opinion some another component got the lock for the Certificate but it didn't release it in the Linux implementation... =/ some idea?
Fernando, I've never seen a deadlock in this area. You say you are able to reproduce it using JavaScript. Are you able to produce a simple test case, a web page that uses JavaScript to call getRawDER, and make your test case available on a web site, so I can see the problem myself? Thanks
To use this sites to reproduce this freeze, follow the steps: 1) Install (as root) the component. 2) Restart the browser to apply new component. 3) Open the index-env.html in your browser. 4) Write any text in 'text' area. 5) Click in 'Post' button. 6) It freeze.
I have uploaded new files to reproduce the freeze. Running the strace in freeze time it return: read(3, "\5\1\234d\362\37c\1L\0\0\0\364\t \3\0\0\0\0I\0\243\2F\0"..., 64) = 64$ gettimeofday({1136222007, 41718}, NULL) = 0$ write(6, "\372", 1) = 1$ open("/dev/tty", O_RDWR|O_NONBLOCK|O_NOCTTY) = 43$ writev(43, [{"*** glibc detected *** ", 23}, {"free(): invalid pointer", 23}, {": 0x", 4}, {"094cb558", 8}, {" ***\n", 5}], 5*** glibc detected *** free(): invalid pointer: 0x094cb558 ***$ ) = 63$ rt_sigprocmask(SIG_UNBLOCK, [ABRT], NULL, 8) = 0$ tgkill(12295, 12295, SIGABRT) = 0$ --- SIGABRT (Aborted) @ 0 (0) ---$ unlink("/home/fernando/.mozilla/firefox/u5oxo5g9.default/lock") = 0$ futex(0xb74b1860, FUTEX_WAIT, 2, NULL$ Which information you need? How to obtain good informations to you debug? Thank you a million.
Fernando, I don't want to install your binary component. I hope you don't mind that I say, the bug could be in your own code, and I don't have the time to debug it. Please demonstrate that you can reproduce the bug without your extension, but with a simple HTML/JS page that calls getRawDER, and I'll be glad to have a look.
I'm not sure that testing this from JS is useful. As that function doesn't copy its out arguments, calls from JS will corrupt the heap due to a double-free.
Attached file test freeze
This javascript example with getrawDER freeze the browser. It run only on local. In remote site (http://www.nerdgroup.org/staff/fernando/test/) it return "was denied UniversalXPConnect privileges". Then, if you can to save the index.html in your machine and open it, your browser freeze and can you to debug. Thank you a million.
<html> <body> <script type="text/javascript"> var emailTreeView; var certSelIndex = 1; var certcache; var cert; var length; var data; try{ netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect"); } catch(err) { alert("Unable to initialize: " + err); } emailTreeView = Components.classes["@mozilla.org/security/nsCertTree;1"].createInstance(Components.interfaces.nsICertTree); certcache = Components.classes["@mozilla.org/security/nsscertcache;1"].createInstance(Components.interfaces.nsINSSCertCache); certcache.cacheAllCerts(); emailTreeView.loadCertsFromCache(certcache, Components.interfaces.nsIX509Cert.USER_CERT); Components.classes["@mozilla.org/security/nsCertTree;1"].createInstance(Components.interfaces.nsICertTree); emailTreeView.loadCertsFromCache(certcache, Components.interfaces.nsIX509Cert.USER_CERT); cert = emailTreeView.getCert(certSelIndex); length = {value:0}; data=[]; //data = cert.getRawDER(length); </script> </body> </html> You need to have a pkcs11 certificate installed.
> This javascript example with getrawDER freeze the browser. Thanks Fernando, I'm able to reproduce!
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
> I'm not sure that testing this from JS is useful. As that function doesn't copy > its out arguments, calls from JS will corrupt the heap due to a double-free. Biesi, doesn't that mean you already found the bug? Good catch!
comment 0 claims this happens from C++ as well...
Attached patch Patch v1Splinter Review
This patch changes getRawDER to copy the out-data, so JS may destroy it.
Attachment #207413 - Flags: review?(cbiesinger)
> comment 0 claims this happens from C++ as well... Fernando, are you able to use my patch, build Mozilla yourself, and test whether you still see the problem?
looks like that was wontfixed in bug 220816...
> looks like that was wontfixed in bug 220816... Thanks Biesi, see my new comment in bug 220816.
Hi Kai, I have build the cvs+patch with --enable-debug and work fine here, without any error and/or warning. It don´t freeze in getRawDER function. Congratulations! Thank you a million.
Attachment #207413 - Flags: review?(cbiesinger) → review?(rrelyea)
Comment on attachment 207413 [details] [diff] [review] Patch v1 r=rrelyea With the following caveat... you get approval for a change to a frozen API. The API does not declare the array as const, so one can argue that this was just a bug, but C++ code may actually leak now. Be sure to let the Camino folks know about this change, I believe they may use GetRawDER to get access to the real certificates. bob
Attachment #207413 - Flags: review?(rrelyea) → review+
> The API does not declare the array as const, so one can argue that this was > just a bug, but C++ code may actually leak now. Does that mean, somebody could be using the reference to the raw DER cert as a way to modify it directly, thus bypassing any other NSS APIs?
Somebody have any new about this bug request? It will be fixed in which release? Thank you a million.
The Cert Viewer Plus extension for Firefox and Thunderbird I just released would also benefit from this fix, actually (I'm calling getRawDER() from JS): https://addons.mozilla.org/extensions/moreinfo.php?application=firefox&id=1964 Currently, I'm advising Linux users to relax glibc's malloc checking by setting MALLOC_CHECK_ to 0 before starting the application (but would be glad if I could remove this warning as soon as possible, of course). BTW, libc implementations on other OSes (FreeBSD, Mac OS X) will also lead to warnings ("modified (page-) pointer", "Deallocation of a pointer not malloced"), but on these platforms the application will not freeze - this seems to be limited to glibc in its default configuration.
(In reply to comment #20) > Currently, I'm advising Linux users to relax glibc's malloc checking by setting > MALLOC_CHECK_ to 0 before starting the application (but would be glad if I > could remove this warning as soon as possible, of course). That seems like an insane idea to me, to be honest...
(In reply to comment #21) From what I understand when reading malloc(3), setting MALLOC_CHECK_ to 0 doesn't disable malloc checking completely - it switches to a somewhat more tolerant implementation (0 means "no diagnostic output", it's also possible to run Firefox with MALLOC_CHECK_=1): When MALLOC_CHECK_ is set, a special (less efficient) implementation is used which is designed to be tolerant against simple errors, such as double calls of free() with the same argument, or overruns of a single byte (off-by-one bugs). Not all such errors can be protected against, however, and memory leaks can result. If MALLOC_CHECK_ is set to 0, any detected heap corruption is silently ignored; if set to 1, a diagnostic is printed on stderr; if set to 2, abort() is called immedi- ately. I agree that setting MALLOC_CHECK_ is an unpleasant workaround and am hoping that this bug is fixed as soon as possible.
Summary: Firefox/Mozilla freeze while call getRawDER → Double memory free in nsIX509::getRawDER
Attachment #207413 - Flags: approval-branch-1.8.1+
Bob, we discussed this change on the Drivers mailing list. Nobody objected to this change. I think I fixed all callers, I did not see any calls in Camino code. Fernando, to answer your question, the fix will be contained in Firefox 2 and 3. I don't know yet if we will be allowed to fix 1.5.x
fix checked in to trunk and 1_8 branch
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
*** Bug 220816 has been marked as a duplicate of this bug. ***
Attachment #207413 - Flags: approval1.8.0.3?
Comment on attachment 207413 [details] [diff] [review] Patch v1 approved for 1.8.0 branch, a=dveditz for drivers
Attachment #207413 - Flags: approval1.8.0.3? → approval1.8.0.3+
Keywords: fixed1.8.0.3
Fernando, could you retest this with the latest release candidate? Or maybe you can point me to some dummy certificates I can use to recreate the problem before I test it myself on the fixed version.
Hi Juan, My tests results: seamonkey/nightly/candidates-1.0.2/seamonkey-1.0.2.en-US.linux-i686 Status: PASS seamonkey/nightly/latest-mozilla1.8.0/seamonkey-1.0.2.en-US.linux-i686 Status: PASS seamonkey/nightly/latest-trunk/seamonkey-1.5a.en-US.linux-i686 Status: PASS CVS trunk Status: PASS All tests passed without errors or warnings (included cvs trunk with --enable-debug). If you to need a certificate for tests, i can to send my person test certificate by mail for you. Thanks a million by attention.
Hi again, Firefox tests: firefox-1.5.0.3.en-US.linux-i686 Status: FAIL firefox-1.5.0.4.en-US.linux-i686 Status: PASS firefox-2.0a2.en-US.linux-i686 Status: PASS firefox-3.0a1.en-US.linux-i686 Status: PASS firefox cvs trunk Status: PASS The 1.5.0.4 have already the patch applied? Thank you a million by attention.
Verified on a Linux machine running FF 1.5.0.4, BuildID = "2006050817"
Status: RESOLVED → VERIFIED
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
This now looks like it leaks from the getRawDER callers in nsNSSCertificateDB (part of the 220816 patch was dealing with these leaks, although there appears to be another caller now. at least on trunk).
Summary: Double memory free in nsIX509::getRawDER → Double memory free in nsIX509::getRawDER when called from JavaScript
Whiteboard: [sg:moderate]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: