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)
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 321598
psm2.4
People
(Reporter: Bienvenu, Assigned: Bienvenu)
Details
Attachments
(1 file)
1.66 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•21 years ago
|
||
copy the data; free it in the callers.
Assignee | ||
Updated•21 years ago
|
Attachment #132424 -
Flags: review?(rrelyea0264)
Comment 3•21 years ago
|
||
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
Assignee | ||
Comment 4•21 years ago
|
||
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
Assignee | ||
Comment 5•21 years ago
|
||
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);
Assignee | ||
Comment 6•21 years ago
|
||
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 7•21 years ago
|
||
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+
Comment 8•21 years ago
|
||
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
Assignee | ||
Comment 9•21 years ago
|
||
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
Comment 10•19 years ago
|
||
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 → ---
Comment 11•18 years ago
|
||
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 ago → 18 years ago
Resolution: --- → DUPLICATE
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•