118832 - PSM includes the private NSS header file crmfi.h

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Wan-Teh Chang]

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.

Comment 1

[Stephane Saux]

I'll try building without that include and we'll see what happens.

Comment 2

[John Unruh]

nsbeta1

Comment 3

[Kai Engert [:KaiE:]]

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

Comment 4

[Wan-Teh Chang]

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.

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch

Comment 6

[Wan-Teh Chang]

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?

Comment 7

[Wan-Teh Chang]

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

Comment 8

[Kai Engert [:KaiE:]]

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

[Kai Engert [:KaiE:]]

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?

Comment 10

[Wan-Teh Chang]

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?

Comment 11

[Wan-Teh Chang]

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

[Kai Engert [:KaiE:]]

Oops, indeed. While the interface is defined in nsIDOMCrypto.idl, I can't find
any callers either, neither in JS or C++.

Comment 13

[Javier Delgadillo]

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.

Comment 14

[Wan-Teh Chang]

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

Comment 15

[Kai Engert [:KaiE:]]

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

[Kai Engert [:KaiE:]]

John, do you know which of your test pages access crypto.generateCRMFRequest ?

Comment 17

[Wan-Teh Chang]

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

Comment 18

[John Unruh]

Getting a dual key uses crypto.generateCRMFRequest
https://pki.mcom.com:1025/ca/DirUserEnrollDual1024.html

View the source code at that page.

Comment 19

[Javier Delgadillo]

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

Comment 20

[Kai Engert [:KaiE:]]

The patch seems to work for me. With the patch applied, I was successfully able
to obtain certificates from both sites.

Comment 21

[Wan-Teh Chang]

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

Comment 22

[Wan-Teh Chang]

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.

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 112357 [details] [diff] [review]
Proposed patch

a=asa (on behalf of drivers) for checkin to 1.3

Comment 24

[Wan-Teh Chang]

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

Comment 25

[Jim Dunn]

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.

Comment 26

[Wan-Teh Chang]

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

[John Unruh]

Verified per wtc's comment.

Comment 28

[Wan-Teh Chang]

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

Comment 29

[Kai Engert [:KaiE:]]

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

Comment 30

[Wan-Teh Chang]

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

[Jim Dunn]

VERIFIED on hpux... thanks wtc