378104 - certutil crashes creating certs with very long validity

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Julien Pierre]

This was reported internally

- create a self signed certificate using the following command:
certutil -S -k rsa -g 1024 -t u,u,u -s CN=foo -n me -v 2112822 -d <dbdir> -z <noise_file> -x -1

Observe that the certutil command will crash because of the high validity value. If the value of validity is less than say 2000, then the command work as expected.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I don't think we should waste a minute more on this.  

We could ensure that the value provided yields a date that can be 
represented in a GeneralizedTime, but we've got so MANY truly 
important bugs to fix that I would advise against working on this.

There is a way to mark bugs as being good projects for first time 
mozilla contributors to undertake, but I don't remember what it is.  
I think this would be a good candidate. 

Comment 2

[Julien Pierre]

Nelson, I agree this is not a valid use case, but our tools should still not crash under invalid input. I looked into this and found several errors in the code of library functions.

Here is the relevant certutil code :

 1421       PRExplodedTime printableTime;
 1422       PRTime now, after;
 1423
 1424       if ( !selfsign ) {
 1425           issuerCert = CERT_FindCertByNicknameOrEmailAddr(handle,
issuerNickName);
 1426           if (!issuerCert) {
 1427               SECU_PrintError(progName, "could not find certificate named
\"%s\"",
 1428                               issuerNickName);
 1429               return NULL;
 1430           }
 1431       }
 1432
 1433       now = PR_Now();
 1434       PR_ExplodeTime (now, PR_GMTParameters, &printableTime);
 1435       if ( warpmonths ) {
(dbx) l
 1436           printableTime.tm_month += warpmonths;
 1437           now = PR_ImplodeTime (&printableTime);
 1438           PR_ExplodeTime (now, PR_GMTParameters, &printableTime);
 1439       }
 1440       printableTime.tm_month += validityMonths;
 1441       after = PR_ImplodeTime (&printableTime);
 1442
 1443       /* note that the time is now in micro-second unit */
 1444       validity = CERT_CreateValidity (now, after);
 1445
 1446       cert = CERT_CreateCertificate(serialNumber,
 1447                                     (selfsign ? &req->subject
 1448                                               : &issuerCert->subject),
 1449                                     validity, req);

I spent some time debugging, and there are several problems :

1) In this case, on line 1441, PR_ImplodeTime produces an incorrect negative
value for "after". That's because it calls PR_NormalizeTime as an intermediate
step, creating another PRExplodedTime structure, which stores years as a 16-bit
signed number, so the value wraps around (2112822 / 12 = 176068 years) from the
year 178075 to the year -18533 .
There is no easy way to fix this, since PR_NormalizeTime returns void and
cannot report an error, and PR_ImplodeTime doesn't have a way to report the out
of bounds error to the application either, as it just returns a PRTime value.

2) Subsequently, CERT_CreateValidity fails to encode the notAfter value,
because it calls DER_TimeToUTCTimeArena, which contains a check that the year
must be no less than 1950 . Thus, CERT_CreateValidity returns this NULL
pointer.

I think we could add a check in CERT_CreateValidity to make sure notAfter is >=
notBefore, to save ourselves tripping on this lower-level check.

3) Finally, certutil passes that NULL pointer gets to CERT_CreateCertificate,
which happily dereferences it and crashes. This is clearly a bug in certutil.
certutil needs to check the return value of CERT_CreateValidity.

Comment 3

[Julien Pierre]

Attachment: Check validity pointer in certutil

Comment 4

[Julien Pierre]

Attachment: Check that notBefore is not later than notAfter

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

The certutil command quoted in comment 0 was trying to create a cert 
valid for 2,112,822 months. 2,112,822 months is 176,068.5 years.  
176 millennia from now.  This is 2007.3 AD.  So, that command was trying 
to create a cert to expire late in the year 178,075 AD. 

Clearly this was not being used to accomplish any real world purpose.
Someone just thought "I wonder what will happen if I put this ridiculously
large number in there."  

Let me suggest working on bug 341371.  Until that bug is fixed, certutil 
cannot request or issue "bridge" (cross-certified) CA certs, which will be 
necessary to test libPKIX.

Comment 6

[Julien Pierre]

Nelson, right, I'm not disputing that it was not a valid use case - it definitely wasn't realistic. Nevertheless, our tools are shipped with many products, and should not crash regardless of user input. My short investigation uncovered a couple of library issues which may be more serious - the NSPR one in particular disturbs me, and I may yet file a separate bug about it,  although NSPR already has some ancient history in the time area (see bug 350). Anyway, the very small NSS fixes are done now and only await review before checkin. I have moved on to something else.

Comment 7

[Wan-Teh Chang]

Comment on attachment 262190 [details] [diff] [review]
Check validity pointer in certutil

>-    cert = CERT_CreateCertificate(serialNumber, 
>+    if (validity) {
>+        cert = CERT_CreateCertificate(serialNumber, 
> 				  (selfsign ? &req->subject 
> 				            : &issuerCert->subject), 
> 	                          validity, req);

The last three lines should also be indented four spaces
to the right.

Comment 8

[Julien Pierre]

Thanks for the reviews, Alexei. I fixed this on the trunk.

Checking in cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.108; previous revision: 1.107
done
Checking in lib/util/sectime.c;
/cvsroot/mozilla/security/nss/lib/util/sectime.c,v  <--  sectime.c
new revision: 1.6; previous revision: 1.5
done

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Julien,  I have some questions about this bug and the fix for it.
Q1) Do we want this in NSS 3.11.7?
Q2) What happens, now with the patch applied, if one tries to generate a
cert or CSR with -v 95928.  This will generate a date in the year 10001.
That date cannot be represented in a GeneralizedTime.  Do we 
- issue a cert with in improper encoding?
- issue a cert encoded for the year 0001?
- crash?
- fail gracefully?  and if so, 
  - where does the failure take place, and how is it manifest to the user?  
  - What error message, if any, does the user get?  

Comment 10

[Julien Pierre]

Nelson,

Q1, it could be useful. If you want to give a 2nd review to the patches I attached, I will check them in to the NSS_3_11_BRANCH . 

Q2, we produce a cert that's incorrectly encoded for the year :0001 (not sure where the colon comes from). There is no crash or graceful failure. This is a different bug than this one, however, and I think it should be in a separate bugzilla and is less serious since there is no crash.
This bug is about a crash with -v 2112822, and the user does get an error for that case and no longer a crash - the error is "invalid arguments".

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 262190 [details] [diff] [review]
Check validity pointer in certutil

SR=nelson for 3.11 branch

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 262192 [details] [diff] [review]
Check that notBefore is not later than notAfter

SR=nelson for 3.11 branch

Comment 13

[Julien Pierre]

I checked this in to the NSS_3_11_BRANCH for 3.11.7 .

Checking in cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.97.2.9; previous revision: 1.97.2.8
done
Checking in lib/util/sectime.c;
/cvsroot/mozilla/security/nss/lib/util/sectime.c,v  <--  sectime.c
new revision: 1.5.28.1; previous revision: 1.5
done

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #10)

> Q2, we produce a cert that's incorrectly encoded for the year :0001 

Actually, it was for the year :001.  
The ':' is the next ASCII character after '9'.  
It was an attempt to encode the millenium "10" in a single character.

Comment 15

[Julien Pierre]

Right. That separate issue is now being tracked in bugzilla 378815 .

Comment 16

[Wan-Teh Chang]

Comment on attachment 262192 [details] [diff] [review]
Check that notBefore is not later than notAfter

This patch is in the FIPS crypto boundary (lib/util).
Please reconsider whether you want this patch on the
NSS_3_11_BRANCH with this in light.  I am neutral.

Comment 17

[Julien Pierre]

Wan-Teh,

Re: comment 16 and attachment 262192 [details] [diff] [review], the fix was already checked in on the 3.11 branch. The function changed is CERT_CreateValidity, which is linked into softoken, but not actually called by any code under nss/lib - the only callers are applications, and they call the exported function from libnss3.
Hopefully, that doesn't jeopardize the FIPS validation.
Perhaps this function should be moved out of util and into certdb - it is declared in certdb/cert.h after all.