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)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.7
People
(Reporter: julien.pierre, Assigned: julien.pierre)
Details
Attachments
(2 files, 3 obsolete files)
512 bytes,
application/octet-stream
|
Details | |
7.34 KB,
patch
|
nelson
:
review+
wtc
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•17 years ago
|
||
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)
Assignee | ||
Comment 2•17 years ago
|
||
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.
Assignee | ||
Comment 3•17 years ago
|
||
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 4•17 years ago
|
||
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+
Comment 5•17 years ago
|
||
The ideal error code is one that indicates the input is out of range. You can try PR_RANGE_ERROR.
Comment 6•17 years ago
|
||
Unfortunately, NSS's error string for PR_RANGE ERROR is: "Unused." Of course, we could fix that, too. :)
Assignee | ||
Comment 7•17 years ago
|
||
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
Assignee | ||
Comment 8•17 years ago
|
||
Attachment #262827 -
Attachment is obsolete: true
Attachment #262842 -
Flags: superreview?(wtc)
Attachment #262842 -
Flags: review?(nelson)
Attachment #262827 -
Flags: superreview?(alexei.volkov.bugs)
Assignee | ||
Comment 9•17 years ago
|
||
The header comment should read less than 10000, of course ...
Assignee | ||
Comment 10•17 years ago
|
||
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 11•17 years ago
|
||
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].
Assignee | ||
Comment 12•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Priority: -- → P3
Assignee | ||
Comment 13•17 years ago
|
||
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)
Assignee | ||
Comment 14•17 years ago
|
||
The January 1, 2050 constant was already present, I just double-checked it.
Assignee | ||
Updated•17 years ago
|
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 15•17 years ago
|
||
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 16•17 years ago
|
||
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 17•17 years ago
|
||
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.
Comment 18•17 years ago
|
||
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);
Assignee | ||
Comment 19•17 years ago
|
||
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.
Assignee | ||
Comment 20•17 years ago
|
||
Attachment #262971 -
Flags: superreview?(wtc)
Attachment #262971 -
Flags: review?(nelson)
Assignee | ||
Updated•17 years ago
|
Target Milestone: --- → 3.11.7
Comment 21•17 years ago
|
||
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 22•17 years ago
|
||
Comment on attachment 262971 [details] [diff] [review] Fix test expression, use LL_INIT, relocate constants r=nelson
Attachment #262971 -
Flags: review?(nelson) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #262858 -
Attachment is obsolete: true
Assignee | ||
Comment 23•17 years ago
|
||
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.
Description
•