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

VERIFIED FIXED

Status

()

--
critical
VERIFIED FIXED
13 years ago
13 years ago

People

(Reporter: fernando, Assigned: kaie)

Tracking

({fixed1.8.1, verified1.8.0.4})

Trunk
x86
Linux
fixed1.8.1, verified1.8.0.4
Points:
---
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(3 attachments)

(Reporter)

Description

13 years ago
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

Comment 1

13 years ago
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?
(Assignee)

Comment 2

13 years ago
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
(Reporter)

Comment 3

13 years ago
Created attachment 207353 [details]
site example to reproduce the freeze

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.
(Reporter)

Comment 4

13 years ago
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.
(Assignee)

Comment 5

13 years ago
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.
(Reporter)

Comment 7

13 years ago
Created attachment 207410 [details]
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.
(Reporter)

Comment 8

13 years ago
<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.
(Assignee)

Comment 9

13 years ago
> This javascript example with getrawDER freeze the browser.

Thanks Fernando, I'm able to reproduce!
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 10

13 years ago
> 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...
(Assignee)

Comment 12

13 years ago
Created attachment 207413 [details] [diff] [review]
Patch v1

This patch changes getRawDER to copy the out-data, so JS may destroy it.
Attachment #207413 - Flags: review?(cbiesinger)
(Assignee)

Comment 13

13 years ago
> 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...
(Assignee)

Comment 15

13 years ago
> looks like that was wontfixed in bug 220816...

Thanks Biesi, see my new comment in bug 220816.
(Reporter)

Comment 16

13 years ago
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.
(Assignee)

Updated

13 years ago
Attachment #207413 - Flags: review?(cbiesinger) → review?(rrelyea)

Comment 17

13 years ago
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+
(Assignee)

Comment 18

13 years ago
> 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?

(Reporter)

Comment 19

13 years ago
Somebody have any new about this bug request?
It will be fixed in which release?

Thank you a million.

Comment 20

13 years ago
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...

Comment 22

13 years ago
(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.
(Assignee)

Updated

13 years ago
Summary: Firefox/Mozilla freeze while call getRawDER → Double memory free in nsIX509::getRawDER
(Assignee)

Updated

13 years ago
Attachment #207413 - Flags: approval-branch-1.8.1+
(Assignee)

Comment 23

13 years ago
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
(Assignee)

Comment 24

13 years ago
fix checked in to trunk and 1_8 branch
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(Assignee)

Comment 25

13 years ago
*** Bug 220816 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

13 years ago
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+
(Assignee)

Updated

13 years ago
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.
(Reporter)

Comment 28

13 years ago
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.
(Reporter)

Comment 29

13 years ago
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
Keywords: fixed1.8.0.4 → verified1.8.0.4
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.