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

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Fernando Ribeiro]

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.

Comment 1

[Egon Rosa Pereira]

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?

Comment 2

[Kai Engert (:KaiE:)]

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

Comment 3

[Fernando Ribeiro]

Attachment: 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.

Comment 4

[Fernando Ribeiro]

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.

Comment 5

[Kai Engert (:KaiE:)]

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.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 7

[Fernando Ribeiro]

Attachment: 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.

Comment 8

[Fernando Ribeiro]

<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.

Comment 9

[Kai Engert (:KaiE:)]

> This javascript example with getrawDER freeze the browser.

Thanks Fernando, I'm able to reproduce!

Comment 10

[Kai Engert (:KaiE:)]

> 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 11

[Christian :Biesinger (don't email me, ping me on IRC)]

comment 0 claims this happens from C++ as well...

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Patch v1

This patch changes getRawDER to copy the out-data, so JS may destroy it.

Comment 13

[Kai Engert (:KaiE:)]

> 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?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

looks like that was wontfixed in bug 220816...

Comment 15

[Kai Engert (:KaiE:)]

> looks like that was wontfixed in bug 220816...

Thanks Biesi, see my new comment in bug 220816.

Comment 16

[Fernando Ribeiro]

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.

Comment 17

[Robert Relyea]

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

Comment 18

[Kai Engert (:KaiE:)]

> 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?

Comment 19

[Fernando Ribeiro]

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

Thank you a million.

Comment 20

[Kaspar]

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.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

(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

[Kaspar]

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

Comment 23

[Kai Engert (:KaiE:)]

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

Comment 24

[Kai Engert (:KaiE:)]

fix checked in to trunk and 1_8 branch

Comment 25

[Kai Engert (:KaiE:)]

*** Bug 220816 has been marked as a duplicate of this bug. ***

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 207413 [details] [diff] [review]
Patch v1

approved for 1.8.0 branch, a=dveditz for drivers

Comment 27

[juan becerra [:juanb]]

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.

Comment 28

[Fernando Ribeiro]

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.

Comment 29

[Fernando Ribeiro]

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.

Comment 30

[juan becerra [:juanb]]

Verified on a Linux machine running FF 1.5.0.4, BuildID = "2006050817"

Comment 31

[Daniel Veditz [:dveditz]]

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