125377 - CERT_ImportCRL always fails

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

This function always returns NULL. Even if the CRL given to it as input is
successfully checked, it does not actually store it into the db/softoken and
falls through the return as below :

	rv = CERT_VerifySignedData(&newCrl->signatureWrap, caCert,
				   PR_Now(), wincx);
	if (rv != SECSuccess) {
	    if (type == SEC_CRL_TYPE) {
		PORT_SetError(SEC_ERROR_CRL_BAD_SIGNATURE);
	    } else {
		PORT_SetError(SEC_ERROR_KRL_BAD_SIGNATURE);
	    }
	    break;
	}

#ifdef FIXME
	/* Do CRL validation and add to the dbase if this crl is more present then the one
	   in the dbase, if one exists.
	 */
	crl = cert_DBInsertCRL (handle, url, newCrl, derCRL, type);
#endif

    } while (0);

    SEC_DestroyCrl (newCrl);
    return (crl);
}

This is something that did not get done when the softoken changes occurred for
3.4 . As a result, crlutil is completely broken because it calls CERT_ImportCRL
. PSM and the web server use different code for CRLs, so they aren't broken.

Comment 1

[Julien Pierre]

FYI, the above code fragment is from mozilla/security/nss/lib/certhigh/certhigh.c .

Comment 2

[Julien Pierre]

This is a regression in 3.4 . The only known user of that function at the moment
is the crlutil tool. We should check if other products are using it and possibly
target this bug for 3.4.1 if not.

The fix to store the crl into the token may not be that complicated though.
SEC_NewCrl is doing it and we might just be able to call that or take some code
from there to do it.

Comment 3

[Robert Relyea]

If CERT_ImportCRL is on the nss.def export list, we should fix it to call
SEC_NewCrl(). If not, then it's OK to fix crlutil and deprecate CERT_ImportCRL.

bob

Comment 4

[Wan-Teh Chang]

CERT_ImportCRL is on the nss.def export list, so we should
fix it to call SEC_NewCrl().

Bob, this is your unfinished work when you landed
BOB_WORK_BRANCH onto the tip.

Comment 5

[Robert Relyea]

Attachment: Implement code to allow CRL Import to work.

Comment 6

[Wan-Teh Chang]

Comment on attachment 71396 [details] [diff] [review]
Implement code to allow CRL Import to work.

In file mozilla/security/nss/lib/certhigh/certhigh.c

>+CERTSignedCrl * crl_storeCRL (PK11SlotInfo *slot,char *url,
>+                  CERTSignedCrl *newCrl, SECItem *derCrl, int type);

Should we add this function declaration to some internal
header file?

>+	PK11_FReeSlot(slot);

This is misspelled :-)

Comment 7

[Robert Relyea]

I didn't see an appropriate private header file to use (sigh). I really didn't want
to put it in a public header. I could put it in certxutl.h (it really belongs in
certi.h, but we don't have one.)

PK11_FReeSlot := linker just caught it (good thing I was waiting to run all.sh
before checking it in:).

Comment 8

[Julien Pierre]

Wan-Teh : we should rename all our files to .cpp and extern "C" in the headers,
then we would never run into those dumb undeclared symbols errors that the C
compiler allows ... Just my 2 cents. Even for C code a C++ compiler is better.

Comment 9

[Robert Relyea]

Ah, no. We run through the C compiler so that we get C linkage and name
processing. That way we can link with programs written in C. You can link C++
programs with C libraries, but you can't (easily) go the other way around. Nice
try, though Julian;).

bob

Comment 10

[Julien Pierre]

Bob,

If you declare the functions extern "C", you get C linkage and name processing,
even though the code is in a .cpp and compiled with a C++ compiler. I really
think it would be cleaner that way and we would pick up errors earlier. It might
require some code clean up to do this for all of NSS though.

Comment 11

[Julien Pierre]

Bob,

The code still crashes with your patch if the URL extension is not present.

t@1 (l@1) terminated by signal SEGV (no mapping at the fault address)
Current function is PK11_PutCrl
 3792       PK11_SETATTRS(attrs, CKA_NETSCAPE_URL, url, PORT_Strlen(url)+1);
attrs++;
(dbx) where
current thread: t@1
  [1] strlen(0x0, 0x0, 0x7c9d4, 0x7efefeff, 0x81010100, 0x0), at 0xfeeb29ec
=>[2] PK11_PutCrl(slot = 0x7abf0, crl = 0xffbef9d8, name = 0x7c9d4, url = (nil),
type = 1), line 3792 in "pk11cert.c"
  [3] crl_storeCRL(slot = 0x7abf0, url = (nil), newCrl = 0x7c9a8, derCrl =
0xffbef9d8, type = 1), line 488 in "crl.c"
  [4] CERT_ImportCRL(handle = 0x730e8, derCRL = 0xffbef9d8, url = (nil), type =
1, wincx = (nil)), line 788 in "certhigh.c"
  [5] ImportCRL(certHandle = 0x730e8, url = (nil), type = 1, inFile = 0x50f48),
line 193 in "crlutil.c"
  [6] main(argc = 6, argv = 0xffbefb0c), line 356 in "crlutil.c"
(dbx) p attrs
attrs = 0xffbef844
(dbx) p url
url = (nil)
(dbx) 

Comment 12

[Robert Relyea]

Attachment: Don't set the URL attribute is no URL parameter has been passed in

Julien, try this patch..

Comment 13

[Julien Pierre]

Bob,

I applied the patch manually.
I'm still crashing, but in a different place.

(dbx) where
current thread: t@1
  [1] __sigprocmask(0x0, 0xffbef570, 0x0, 0x0, 0x0, 0x0), at 0xff159bf0
  [2] _resetsig(0xff15c510, 0x0, 0x0, 0x50b48, 0xff16e000, 0x0), at 0xff14e620
  [3] _sigon(0x50b48, 0xff175990, 0x6, 0xffbef644, 0x50b48, 0xffbef688), at
0xff14dd10
  [4] _thrp_kill(0x0, 0x1, 0x6, 0xff16e000, 0x1, 0xfef3a428), at 0xff150e84
  [5] raise(0x6, 0x0, 0x0, 0xffffffff, 0xfef3a394, 0xff0eb6dc), at 0xfeec9b08
  [6] abort(0xfef36000, 0x5b, 0xfef3d99c, 0xfef39c78, 0xed5, 0x0), at 0xfeeb5124
=>[7] PR_Assert(s = 0xff22bbc4 "tsize >=
sizeof(theTemplate)/sizeof(theTemplate[0])", file = 0xff22bbf8 "pk11cert.c", ln
= 3797), line 495 in "prlog.c"
  [8] PK11_PutCrl(slot = 0x7abf0, crl = 0xffbef9d8, name = 0x7c9d4, url = (nil),
type = 1), line 3797 in "pk11cert.c"
  [9] crl_storeCRL(slot = 0x7abf0, url = (nil), newCrl = 0x7c9a8, derCrl =
0xffbef9d8, type = 1), line 488 in "crl.c"
  [10] CERT_ImportCRL(handle = 0x730e8, derCRL = 0xffbef9d8, url = (nil), type =
1, wincx = (nil)), line 788 in "certhigh.c"
  [11] ImportCRL(certHandle = 0x730e8, url = (nil), type = 1, inFile = 0x50f48),
line 193 in "crlutil.c"
  [12] main(argc = 6, argv = 0xffbefb0c), line 356 in "crlutil.c"
(dbx) 

I am attaching the DER CRL and necessary root CA cert to this defect so you can
try them.

Comment 14

[Julien Pierre]

Attachment: root CA matched to the CRL

Comment 15

[Julien Pierre]

Attachment: test CRL I'm using

Comment 16

[Robert Relyea]

Attachment: Don't include the URL is one isn't supplied. Fix the Assert to have the correct inequality check

Julien, thanks for the test CRL's. I also have another patch I will attach to
this bug with fixed the problem where crlutil won't list the CRLs in the
database as well.

Comment 17

[Robert Relyea]

Attachment: KRL handles are unique, and should be different from a primordial CRL handle

Make PK11_LookupCrls() work correctly.

Comment 18

[Wan-Teh Chang]

Comment on attachment 71682 [details] [diff] [review]
KRL handles are unique, and should be different from a primordial CRL handle

r=wtc.

Comment 19

[Wan-Teh Chang]

Comment on attachment 71681 [details] [diff] [review]
Don't include the URL is one isn't supplied. Fix the Assert to have the correct inequality check

Bob, did you attach the wrong patch?  This patch
does not match the description, and seems to be
the same as the patch for bug 126834. :-)

Comment 20

[Robert Relyea]

Attachment: Don't crash if url is not specified.

Grab patch from the correct directory;).

bob

Comment 21

[Julien Pierre]

This works now.

Comment 22

[Julien Pierre]

Ooops. forgot to mark fixed.

Comment 23

[Julien Pierre]

Marking verified.

Comment 24

[Wan-Teh Chang]

Comment on attachment 71731 [details] [diff] [review]
Don't crash if url is not specified.

r=wtc.