Closed Bug 220816 Opened 21 years ago Closed 18 years ago

nsNSSCertificate::GetRawDER needs to copy data it returns

Categories

(Core Graveyard :: Security: UI, defect)

1.0 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 321598
psm2.4

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Details

Attachments

(1 file)

nsNSSCertificate::GetRawDER needs to copy the data it returns, to match the idl
and in order for it to be called from js : 

 void getRawDER(out unsigned long length,
	               [retval, array, size_is(length)] out octet data);

patch upcoming
Attached patch proposed fixSplinter Review
copy the data; free it in the callers.
cc'ing Nelson
Status: NEW → ASSIGNED
Attachment #132424 - Flags: review?(rrelyea0264)
Changed product to PSM.  Adding relyea to CC list.

David, can you provide a little background for this bug?
Component: Libraries → Client Library
OS: Windows 2000 → All
Product: NSS → PSM
Hardware: PC → All
Target Milestone: --- → 2.4
Version: unspecified → 2.4
In order to call this method from JS (which is needed for some auto config
stuff), GetRawDER needs to make a copy of the data, because that's what
JS/XPConnect requires from the IDL

Here's more context - maybe you can help:


const nsIX509CertDB = Components.interfaces.nsIX509CertDB;
const nsX509CertDBContractID = "@mozilla.org/security/x509certdb;1";

var certDB = Components.classes[nsX509CertDBContractID].getService(nsIX509CertDB);

	xxxca2 = "base64 encoded cert"

	// Add xxxCA2 for email, server, and code signing

var certlength = new Object;
var data = new Object;
		var cert = certDB.constructX509FromBase64(xxxca2);
    data = cert.getRawDER(certlength);
    certDB.importUserCertificate(data, certlength.value, null);
any problems with this patch? Also, when I try to import a cert that contains a
public key, nsNSSCertificateDB::ImportUserCertificate fails on this line:
   slot = PK11_KeyForCertExists(cert, NULL, ctx);

Is this just a valid error (i.e., is it possible that the cert is such that I
need some sort of private key in my cert db that I don't have?) or have I done
something wrong in the code?
Comment on attachment 132424 [details] [diff] [review]
proposed fix

The only worry is that all the places you call GetRawDER are freeing the
results. It might be better to create a new method that returns the allocated
version.

There is a performance hit in those cases where the call to GetRawDER() would
have used and 'discarded' before the destruction of the underlying cert. If the
only case that this happens is the one in your patch, then it's not an issue
(CA download is pretty rare).

patch approved pending verification of the calls to GetRawDER.

bob
Attachment #132424 - Flags: review?(rrelyea0264) → review+
re comment 6 :
PK11_KeyForCertExists looks for a private key. Inside NSS the term "User Cert"
implies certs owned by the user... that is certificates which the user holds the
private key for. Certificates from other users are considered 'Peer' certs.

So the short answer is yes, by definition the cert needs a private key. If you
want to import a different cert, you will need to use a different import call.

bob
Hey Bob, thx, but I don't need this anymore, because the interfaces were frozen
and I had to use a different approach. If we had embedders using this call from
c++, this change would introduce a memory leak anyway :-( 
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → WONTFIX
Product: PSM → Core
Unfortunately function getRawDER became part of the frozen interface.
It's possible to call it from JS.
People try to do it, see bug 321598.
It seems, as getRawDER gives out a reference, JS keeps a dangling pointer.
I was able to reproduce a crash with the test case attached to bug 321598.

As the interface is frozen, IMHO we should make our code correct, and if somebody indeed requires the additional performance (by avoding to copy), we should come up with a new, non-scriptable interface.

I'm reopening and request that we fix this bug with the patch in bug 321598.
What do you think?
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
The fix has landed.
Now that we always copy the out array: If you think the performance penalty is not acceptable, let's add a non-scriptable function to a new interface.


*** This bug has been marked as a duplicate of 321598 ***
Status: REOPENED → RESOLVED
Closed: 21 years ago18 years ago
Resolution: --- → DUPLICATE
Version: psm2.4 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: