PSM includes the private NSS header file crmfi.h

VERIFIED FIXED in psm2.4

Status

Core Graveyard
Security: UI
P1
normal
VERIFIED FIXED
16 years ago
10 months ago

People

(Reporter: Wan-Teh Chang, Assigned: Stephane Saux)

Tracking

1.0 Branch
psm2.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

16 years ago
I'll try building without that include and we'll see what happens.
Priority: -- → P1
Target Milestone: --- → 2.2

Comment 2

16 years ago
nsbeta1
Component: Daemon → Client Library
Keywords: nsbeta1
(Assignee)

Updated

16 years ago
Keywords: nsbeta1 → nsbeta1-

Comment 3

15 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

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

Updated

15 years ago
Keywords: nsbeta1

Updated

15 years ago
Keywords: nsbeta1-
(Reporter)

Comment 5

15 years ago
Created attachment 112357 [details] [diff] [review]
Proposed patch
(Reporter)

Comment 6

15 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

15 years ago
I forgot to mention that this patch is against Mozilla 1.3a.

Comment 8

15 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

15 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

15 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

15 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

15 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

15 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

15 years ago
Kai, could you test this patch for me?  Thanks.

Comment 15

15 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

15 years ago
John, do you know which of your test pages access crypto.generateCRMFRequest ?
(Reporter)

Comment 17

15 years ago
Kai, I'd like you to test the functionality.  Thanks.

Comment 18

15 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

15 years ago
http://testca.netscape.com/ will also test crypto.generateCRMFRequest

Comment 20

15 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

15 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)
Attachment #112357 - Flags: superreview?(bryner) → superreview+
(Reporter)

Comment 22

15 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

15 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

15 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
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 25

15 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

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

Comment 27

15 years ago
Verified per wtc's comment.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 28

15 years ago
Created attachment 116404 [details] [diff] [review]
Do not use static_cast to cast a pointer to an incomplete type to a void * pointer

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

15 years ago
Attachment #116404 - Flags: review?(kaie)

Comment 29

15 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

15 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

15 years ago
VERIFIED on hpux... thanks wtc

Updated

13 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

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