Closed
Bug 118832
Opened 23 years ago
Closed 22 years ago
PSM includes the private NSS header file crmfi.h
Categories
(Core Graveyard :: Security: UI, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
psm2.4
People
(Reporter: wtc, Assigned: ssaux)
Details
Attachments
(2 files)
3.59 KB,
patch
|
KaiE
:
review+
bryner
:
superreview+
asa
:
approval1.3+
|
Details | Diff | Splinter Review |
779 bytes,
patch
|
KaiE
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
The PSM file mozilla/security/manager/ssl/src/nsCrypto.cpp includes the private NSS header file crmfi.h. We should find out whether PSM really needs to include crmfi.h. If so, we should eliminate that need, which may require fixing NSS.
Assignee | ||
Comment 1•23 years ago
|
||
I'll try building without that include and we'll see what happens.
Priority: -- → P1
Target Milestone: --- → 2.2
Assignee | ||
Updated•22 years ago
|
Comment 3•22 years ago
|
||
I tried. When you remove the include, compilcation fails with an error: /mozilla/security/manager/ssl/src/nsCrypto.cpp: In function `char * nsCreateReqFromKeyPairs(struct nsKeyPairInfo *, PRInt32, char *, char *, char *, class nsNSSCertificate *)': /mozilla/security/manager/ssl/src/nsCrypto.cpp:1347: aggregate `struct CRMFCertReqMessages messages' has incomplete type and cannot be initialized
Reporter | ||
Comment 4•22 years ago
|
||
You should be able to use the CRMF_EncodeCertReqMessages function to avoid the need for 'CRMFCertReqMessages messages'. You should be able to replace the SEC_ASN1EncodeItem call on 'messages' by two CRMF_EncodeCertReqMessages calls on 'certReqMsgs'. You would use the first call to get the length of the encoded output, allocate a SECItem with a buffer that size, and use the second call to write the encoded output in the SECItem. There may be other solutions.
Reporter | ||
Comment 5•22 years ago
|
||
Reporter | ||
Comment 6•22 years ago
|
||
Comment on attachment 112357 [details] [diff] [review] Proposed patch Bob, Kai, could you review this patch? Kai, could you test the code that's affected?
Attachment #112357 -
Flags: superreview?(kaie)
Attachment #112357 -
Flags: review?(relyea)
Reporter | ||
Comment 7•22 years ago
|
||
I forgot to mention that this patch is against Mozilla 1.3a.
Comment 8•22 years ago
|
||
Thanks for the patch. I will review and test it, but note that I'm not a superreviewer, we will need to ask one of the general superreviewers.
Comment 9•22 years ago
|
||
Comment on attachment 112357 [details] [diff] [review] Proposed patch r=kaie Patch looks fine to me. I've tested it and it compiles on Linux. However, I'm not sure which functionality I'd have to access that uses this portion of the code. Wan-Teh, do you think compiling on Linux is sufficient, or should I test on Windows and Mac, too?
Attachment #112357 -
Flags: superreview?(kaie)
Attachment #112357 -
Flags: review?(relyea)
Attachment #112357 -
Flags: review+
Reporter | ||
Comment 10•22 years ago
|
||
Javi, I have a patch that modifies the nsCreateReqFromKeyPairs function in nsCrypto.cpp (http://lxr.mozilla.org/security/ident?i=nsCreateReqFromKeyPairs), which is used by nsCrypto::GenerateCRMFRequest. Do you know what tests we can perform to exercise that code path?
Reporter | ||
Comment 11•22 years ago
|
||
My LXR searches showed that nsCrypto::GenerateCRMFRequest is not being used by the Mozilla client. Kai, could you confirm this? This would mean we can't test the modified code using the Mozilla client.
Comment 12•22 years ago
|
||
Oops, indeed. While the interface is defined in nsIDOMCrypto.idl, I can't find any callers either, neither in JS or C++.
Comment 13•22 years ago
|
||
To test this, any page that calls crypto.generateCRMFRequest should do. I believe John Unruh's web page has such sites set up. Failing that, you could just get another cert from the Corporate CA.
Reporter | ||
Comment 14•22 years ago
|
||
Kai, could you test this patch for me? Thanks.
Comment 15•22 years ago
|
||
Wan-Teh, did you see my comment 9, where I say I have already tested that your patch compiles? Or did you mean that I should test the functionality?
Comment 16•22 years ago
|
||
John, do you know which of your test pages access crypto.generateCRMFRequest ?
Reporter | ||
Comment 17•22 years ago
|
||
Kai, I'd like you to test the functionality. Thanks.
Comment 18•22 years ago
|
||
Getting a dual key uses crypto.generateCRMFRequest https://pki.mcom.com:1025/ca/DirUserEnrollDual1024.html View the source code at that page.
Target Milestone: 2.2 → 2.4
Version: unspecified → 2.4
Comment 19•22 years ago
|
||
http://testca.netscape.com/ will also test crypto.generateCRMFRequest
Comment 20•22 years ago
|
||
The patch seems to work for me. With the patch applied, I was successfully able to obtain certificates from both sites.
Reporter | ||
Comment 21•22 years ago
|
||
Comment on attachment 112357 [details] [diff] [review] Proposed patch Kai, thank you for testing this patch. Brian, could you review this patch? PSM is including two private NSS header files. One of them (genname.h) is just unnecessary. The other (crmfi.h) is for the definition of an opaque structure (CRMFCertReqMessages). I found that the need for the definition for that opaque type can be eliminated by using a public function (CRMF_EncodeCertReqMessages).
Attachment #112357 -
Flags: superreview?(bryner)
Updated•22 years ago
|
Attachment #112357 -
Flags: superreview?(bryner) → superreview+
Reporter | ||
Comment 22•22 years ago
|
||
Comment on attachment 112357 [details] [diff] [review] Proposed patch Requesting mozilla 1.3 approval. Asa, please see comment 21 for a description of the patch. The risk of this patch is low and is confined to certificate requests. Kai has tested the patch against our test certificate authority.
Attachment #112357 -
Flags: approval1.3?
Comment 23•22 years ago
|
||
Comment on attachment 112357 [details] [diff] [review] Proposed patch a=asa (on behalf of drivers) for checkin to 1.3
Attachment #112357 -
Flags: approval1.3? → approval1.3+
Reporter | ||
Comment 24•22 years ago
|
||
Patch checked in. Checking in Makefile.in; /cvsroot/mozilla/security/manager/ssl/src/Makefile.in,v <-- Makefile.in new revision: 1.54; previous revision: 1.53 done Checking in nsCrypto.cpp; /cvsroot/mozilla/security/manager/ssl/src/nsCrypto.cpp,v <-- nsCrypto.cpp new revision: 1.31; previous revision: 1.30 done Checking in nsNSSIOLayer.cpp; /cvsroot/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp,v <-- nsNSSIOLayer. cpp new revision: 1.87; previous revision: 1.86 done
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 25•21 years ago
|
||
Have you guys tried this on an HP-UX build? I ran into problems on hp 10.20 using aCC A.01.21 (I am trying on 11 with a newer aCC as I speak). When I build security/managers/ssl/src/nsCrypto.cpp I get the following: Error 744: "nsCrypto.cpp", line 884 # The operand of a static_cast must be a pointer to or lvalue of a complete class; the actual type was CRMFEncryptedKeyStr *. static_cast< void* >(encrKey) ); ^^^^^^^ If I go back and include crmfi.h in nsCrypto.cpp and return the -I for the nss private header directory it builds. What I think is happening is that aCC needs to know exactly what CRMFEncryptedKey is. Right now all it knows is that it is a "struct CRMFEncryptedKeyStr" but the definition of such is not included.
Reporter | ||
Comment 26•21 years ago
|
||
Jim, Could you ask on HP's cxx-dev mailing list what's the right way to cast a pointer to an incomplete C struct to a void * pointer in C++? I think we can replace the static_cast by either a C-style cast or reinterpret_cast, but I'd like to hear from a C++ expert what's the right way to do this cast in C++.
Reporter | ||
Comment 28•21 years ago
|
||
aCC on HP-UX does not allow using static_cast to cast a pointer to an incomplete type to a void * pointer. With the removal of the crmfi.h inclusion, CRMFEncryptedKey becomes an incomplete type. Turns out that any data pointer can be implicitly converted to a void * pointer by the compiler, so no cast is needed.
Attachment #116404 -
Flags: superreview+
Reporter | ||
Updated•21 years ago
|
Attachment #116404 -
Flags: review?(kaie)
Comment 29•21 years ago
|
||
Comment on attachment 116404 [details] [diff] [review] Do not use static_cast to cast a pointer to an incomplete type to a void * pointer r=kaie
Attachment #116404 -
Flags: review?(kaie) → review+
Reporter | ||
Comment 30•21 years ago
|
||
Comment on attachment 116404 [details] [diff] [review] Do not use static_cast to cast a pointer to an incomplete type to a void * pointer Patch checked into the trunk (mozilla 1.4alpha). Jim, could you verify it on HP-UX?
Comment 31•21 years ago
|
||
VERIFIED on hpux... thanks wtc
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
•