Closed Bug 378815 Opened 17 years ago Closed 17 years ago

DER_TimeToGeneralizedTimeArena and DER_TimeToUTCTime don't check for valid range and may leak

Categories

(NSS :: Libraries, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.7

People

(Reporter: julien.pierre, Assigned: julien.pierre)

Details

Attachments

(2 files, 3 obsolete files)

This bug can be triggered with the following test case :

certutil -S -k rsa -g 1024 -t u,u,u -s CN=foo -n me -v 95928 -d <dbdir> -z
<noise_file> -x -1

DER_TimeToGeneralizedTimeArena will return an improperly encoded value of ":0010224235450Z" for the notAfter . The colon is invalid. Here is the stack :

=>[1] DER_TimeToGeneralizedTimeArena(arenaOpt = 0x80c3fb8, dst = 0x80c7330, gmttime = 253443858890259185LL), line 292 in "dertime.c"
  [2] DER_EncodeTimeChoice(arena = 0x80c3fb8, output = 0x80c7330, input = 253443858890259185LL), line 260 in "sectime.c"
  [3] CERT_CreateValidity(notBefore = 1177545290259185LL, notAfter = 253443858890259185LL), line 134 in "sectime.c"
  [4] MakeV1Cert(handle = 0x80baab0, req = 0x80c6b00, issuerNickName = (nil), selfsign = 1, serialNumber = 2245989387U, warpmonths = 0, validityMonths = 95928), line 1444 in "certutil.c"
  [5] CreateCert(handle = 0x80baab0, issuerNickName = (nil), inFile = 0x808c2c0, outFile = 0x808c580, selfsignprivkey = 0x80c4a80, pwarg = 0x804660c, hashAlgTag = SEC_OID_UNKNOWN, serialNumber = 2245989387U, warpmonths = 0, validityMonths = 95928, emailAddrs = (nil), dnsNames = (nil), ascii = 0, selfsign = 1, keyUsage = 1, extKeyUsage = 0, basicConstraint = 0, authKeyID = 0, crlDistPoints = 0, nscpCertType = 0), line 2250 in "certutil.c"
  [6] certutil_main(argc = 18, argv = 0x80466bc, initialize = 1), line 3032 in "certutil.c"
  [7] main(argc = 18, argv = 0x80466bc), line 3194 in "certutil.c"
(dbx) l
  291       d[0] = (printableTime.tm_year /1000) + '0';
  292       d[1] = ((printableTime.tm_year % 1000) / 100) + '0';
  293       d[2] = ((printableTime.tm_year % 100) / 10) + '0';
  294       d[3] = (printableTime.tm_year % 10) + '0';

This code doesn't take into account the case where printableTime.tm_year /1000 is greater than 9 . This is where the colon comes from. The function should fail in this case as there is no proper GeneralizedTime encoding for this date.
Attached patch Validate input year range (obsolete) — Splinter Review
I moved the PR_ExplodeTime call earlier in the function in order to prevent an unncessary allocation if the range is invalid (which would cause memory growth/leak in the arena).

Negative years are also disallowed in GeneralizedTime, so I added a check for that also. We can't get to that point with certutil, since it checks for a positive validityMonths; and also DER_EncodeTimeChoice will call DER_UTCTimeToTime for dates prior to January 1, 2050. But there may be other call stacks which allow negative years to be passed in, and we have just made DER_TimeToGeneralizedTimeArena a public function, so somebody could try to feed it a PRTime from a negative year, and thus it is worthwile to check for negative years here too.
Attachment #262827 - Flags: superreview?(alexei.volkov.bugs)
Attachment #262827 - Flags: review?(nelson)
BTW, I sent email to Wan-Teh aobut the comment referring to NSPR 2.0 before the PR_ExplodeTime call. I am not aware of an alternate NSPR function to use.
Wan-Teh just let me know that the comment is invalid. Please ignore it in your review . I will delete it for the checkin.
Comment on attachment 262827 [details] [diff] [review]
Validate input year range

r=nelson.  I wonder about one thing.

>+       PORT_SetError(SEC_ERROR_INVALID_ARGS);

Maybe it should be SEC_ERROR_BAD_DATA ?
Attachment #262827 - Flags: review?(nelson) → review+
The ideal error code is one that indicates the input is out
of range.  You can try PR_RANGE_ERROR.
Unfortunately, NSS's error string for PR_RANGE ERROR is: "Unused." 
Of course, we could fix that, too.  :)
Wan-Teh, Nelson,

I don't thnk the PR_RANGE_ERROR is very good, since most programs don't have a useful string for it compiled in, even we add it later. Also, the function here isn't even an NSPR function, and the time argument we are talking about is unfortunately declared as an int64 in the prototype, not even as a PRTime.

SEC_ERROR_BAD_DATA is somewhat vague.

SEC_ERROR_INVALID_TIME would be my second choice, but the existing string for it compiled in many programs already is "Improperly formatted time string", which is not the problem here.

Thus, I think SEC_ERROR_INVALID_ARGS is appropriate. With the addition of a comment in secder.h about the valid range of years for GeneralizedTime, I think it should suffice.
Summary: DER_GeneralizedTimeToTimeArena should fail if time value cannot be properly encoded → DER_TimeToGeneralizedTimeArena should fail if time value cannot be properly encoded
Attachment #262827 - Attachment is obsolete: true
Attachment #262842 - Flags: superreview?(wtc)
Attachment #262842 - Flags: review?(nelson)
Attachment #262827 - Flags: superreview?(alexei.volkov.bugs)
The header comment should read less than 10000, of course ...
Comment on attachment 262842 [details] [diff] [review]
remove NSPR 2.0 comment, add year range comment in header file

I found some other issues in closely related code.
Attachment #262842 - Attachment is obsolete: true
Attachment #262842 - Flags: superreview?(wtc)
Attachment #262842 - Flags: review?(nelson)
Comment on attachment 262827 [details] [diff] [review]
Validate input year range

>+    if ( (printableTime.tm_year < 0) || (printableTime.tm_year / 1000 > 9)) {

It is more clear to say "printableTime.tm_year > 9999" so that one
can easily see the legal range of printableTime.tm_year is [0, 9999].
I'm working on a new patch that uses PRTime constants early on to validate the gmttime argument without even calling PR_ExplodeTime, analog to what is already being done in DER_EncodeTimeChoice.

The other related issue I found is in DER_TimetoUTCTimeArena. It only checks for the year to be after 1950, leaking memory if it is not; and it does not check if the year is below 2050. Also, it wasn't setting any NSPR code for the range error.
Priority: -- → P3
I took a different approach to use PRTime constants to compare against. This is more efficient at runtime than calling PR_ExplodeTime to figure out that the year is not within the range.

In DER_TimetoUTCTimeArena, there was a check for before 1950, but it was late, which would most likely result in a leak for the NULL arena case (or DER_TimeToUTCTimecall ) since the caller might forget to free the memory if the function returned SECFailure. This early check prevents it.
Attachment #262858 - Flags: superreview?(wtc)
Attachment #262858 - Flags: review?(nelson)
The January 1, 2050 constant was already present, I just double-checked it.
Summary: DER_TimeToGeneralizedTimeArena should fail if time value cannot be properly encoded → DER_TimeToGeneralizedTimeArena and DER_TimeToUTCTime don't check for valid range and may leak
Comment on attachment 262858 [details] [diff] [review]
Rewrite, also fix DER_TimeToUTCTimeArena

Julien, I see two sets of problems with this patch.  I think I must 
give this patch r-, and will do so, but you might be able to persuade me
to change my review with contrary empirical evidence from all platforms.

#1. Operator precedence. The following two new blocks of code each
have an expression of the form (!x<y).

>+    if ( (gmttime<January1st1950) || (!gmttime<January1st2050) ) {
>+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
>+        return SECFailure;
>+    }

>+    if ( (gmttime<January1st1) || (!gmttime<January1st10000) ) {
>+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
>+        return SECFailure;
>+    }

As I understand c operator precedence, the expression (!x<y) is equivalent to 
((!x)<y).  The expression (!x) always evaluates precisely to one of these two
values: 0, 1.  So, I think that both of the above expressions of the form 
(!x<y) will always be true, given that y > 1.  See the c operator precedence 
table at http://www.difranco.net/cop2220/op-prec.htm

Does your testing reveal otherwise?  Do these functions ever do other than
returning SECFailure with error SEC_ERROR_INVALID_ARGS?

I suggest that you either add explicit parentheses, e.g. (!(x<y)) 
or just change the comparison operator (x>=y).

>+/* These constants are used internally for UTCTime/GeneralizedTime encoding */
>+#define January1st1     (PRTime) 0xff23400100d44000
>+#define January1st1950  (PRTime) 0xfffdc1f8793da000
>+#define January1st2050  (PRTime) 0x0008f81e1b098000
>+#define January1st10000 (PRTime) 0x0384440ccc736000

I think those defines will not work as expected on several platforms, maybe 
all platforms where int is only 32 bits (which is basically all platforms).  
The problem is that without a suffix of ULL or UL or ia64 (or windows), and 
without a dot to make it a floating point constant, a decimal or hex 
constant is an int, not a long or long long, and its value will be truncated 
to fit in sizeof int.  Casting it to be a 64-bit type doesn't solve this 
problem.  The int size truncation is applied BEFORE the cast occurs.  

The LL_INIT macro solves this problem.  It knows the right kind of suffix to 
apply for each platform to get a genuine 64-bit constant value or expression.  
See the implementation of LL_INIT at 
http://lxr.mozilla.org/nspr/source/nsprpub/pr/include/prlong.h#80
It will do the right thing on every platform.  That's why the existing code
used LL_INIT.
Attachment #262858 - Flags: review?(nelson) → review-
Comment on attachment 262858 [details] [diff] [review]
Rewrite, also fix DER_TimeToUTCTimeArena

>+    if ( (gmttime<January1st1950) || (!gmttime<January1st2050) ) {

This should be formatted like this:

      if ((gmttime < January1st1950) || (gmttime >= January1st2050)) {

Many style guidelines discourage the use of negation in conditional
expressions.  In this case the negation can be easily avoided.

It's true that some compilers will truncate your 64-bit constants because
the constants don't have the correct suffix.  Unfortunately older versions
of Visual C++ only support the nonstandard i64 suffix.  So it's better
to define these constants as 'static const' and use the LL_INIT macro
to initialize them.  Or you can use either i64 or LL depending whether
the macro _MSC_VER is defined.
Attachment #262858 - Flags: superreview?(wtc)
Comment on attachment 262858 [details] [diff] [review]
Rewrite, also fix DER_TimeToUTCTimeArena

>+/* These constants are used internally for UTCTime/GeneralizedTime encoding */
>+#define January1st1     (PRTime) 0xff23400100d44000
>+#define January1st1950  (PRTime) 0xfffdc1f8793da000
>+#define January1st2050  (PRTime) 0x0008f81e1b098000
>+#define January1st10000 (PRTime) 0x0384440ccc736000

You should avoid defining these constants in the exported header secder.h.
I know you need to share them between dertime.c and sectime.c.  You can
duplicate the definitions or add a new local header.
Re: comment 17, I agree that you're better off with declaring 
const external variables in the .h file, and statically initializing 
them in one of the .c files that references them.  e.g.

in foo.h:
const extern PRTime January1st2050;
in bar,c:
const PRTime January1st2050 = LL_INIT(0x0008f81e,0x1b098000);
Nelson,

Re: comment 15, you may be right about the operator precedence. My testing showed a failure, but it could have been due to the constant truncation also. I will change the test expressions as you and Wan-Teh suggested to use >= .

Re: constant truncation, I thought we had determined that the LL macros were no longer necessary for current compilers. I will use them in the forthcoming patch.

Re: constant location, there is actually only one constant that is shared, January1st2050 . I will just duplicate it as a static const in the C files. Somebody just filed an internal bug at Sun about another static const in keythi that I put in, and lint complained about because it was unused in their application. So I guess I will not increase their lint log anymore with the same construct here.
Attachment #262971 - Flags: superreview?(wtc)
Attachment #262971 - Flags: review?(nelson)
Target Milestone: --- → 3.11.7
Comment on attachment 262971 [details] [diff] [review]
Fix test expression, use LL_INIT, relocate constants

r=wtc.

>+    if ( (gmttime<January1st1950) || (gmttime>=January1st2050) ) {

You should add spaces before and after '<' and '>='.
Attachment #262971 - Flags: superreview?(wtc) → superreview+
Comment on attachment 262971 [details] [diff] [review]
Fix test expression, use LL_INIT, relocate constants

r=nelson
Attachment #262971 - Flags: review?(nelson) → review+
Attachment #262858 - Attachment is obsolete: true
Checked in to NSS_3_11_BRANCH :

Checking in dertime.c;
/cvsroot/mozilla/security/nss/lib/util/dertime.c,v  <--  dertime.c
new revision: 1.7.28.1; previous revision: 1.7
done
Checking in secder.h;
/cvsroot/mozilla/security/nss/lib/util/secder.h,v  <--  secder.h
new revision: 1.7.28.1; previous revision: 1.7
done
Checking in sectime.c;
/cvsroot/mozilla/security/nss/lib/util/sectime.c,v  <--  sectime.c
new revision: 1.5.28.2; previous revision: 1.5.28.1
done

And to the trunk :

Checking in dertime.c;
/cvsroot/mozilla/security/nss/lib/util/dertime.c,v  <--  dertime.c
new revision: 1.8; previous revision: 1.7
done
Checking in secder.h;
/cvsroot/mozilla/security/nss/lib/util/secder.h,v  <--  secder.h
new revision: 1.8; previous revision: 1.7
done
Checking in sectime.c;
/cvsroot/mozilla/security/nss/lib/util/sectime.c,v  <--  sectime.c
new revision: 1.7; previous revision: 1.6
done


I verified that none of this code is being called from softoken, even though it links to util.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.