Closed
Bug 536023
Opened 14 years ago
Closed 14 years ago
DER_UTCTimeToTime and DER_GeneralizedTimeToTime ignore all bytes after an embedded null
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.6
People
(Reporter: wtc, Assigned: wtc)
Details
Attachments
(4 files, 2 obsolete files)
3.46 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
3.28 KB,
text/plain
|
nelson
:
review+
|
Details |
8.12 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
2.77 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
DER_UTCTimeToTime and DER_GeneralizedTimeToTime ignore all bytes after an embedded null. An embedded null allows an invalid UTCTime or GeneralizedTime string to be parsed successfully, for example, 20091219000000Z\0junkjunkjunkjunkjunkjunkjunk The junk bytes can be arbitrarily long because these two functions do not impose a maximum length on inputs.
Attachment #418537 -
Flags: review?(nelson)
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #418538 -
Flags: review?(nelson)
Updated•14 years ago
|
Attachment #418538 -
Flags: review?(nelson) → review+
Updated•14 years ago
|
Attachment #418537 -
Flags: review?(nelson) → review+
Updated•14 years ago
|
Priority: -- → P2
Version: unspecified → 3.12
Assignee | ||
Comment 2•14 years ago
|
||
Comment on attachment 418537 [details] [diff] [review] Proposed patch (checked in) Checked in on the NSS trunk (NSS 3.12.6). Checking in cmd/tests/manifest.mn; /cvsroot/mozilla/security/nss/cmd/tests/manifest.mn,v <-- manifest.mn new revision: 1.9; previous revision: 1.8 done Checking in lib/util/dertime.c; /cvsroot/mozilla/security/nss/lib/util/dertime.c,v <-- dertime.c new revision: 1.12; previous revision: 1.11 done
Attachment #418537 -
Attachment description: Proposed patch → Proposed patch (checked in)
Assignee | ||
Comment 3•14 years ago
|
||
Comment on attachment 418538 [details]
Add a test program mozilla/security/nss/cmd/tests/dertimetest.c (checked in)
Checked in on the NSS trunk (NSS 3.12.6).
RCS file: /cvsroot/mozilla/security/nss/cmd/tests/dertimetest.c,v
done
Checking in cmd/tests/dertimetest.c;
/cvsroot/mozilla/security/nss/cmd/tests/dertimetest.c,v <-- dertimetest.c
initial revision: 1.1
done
Attachment #418538 -
Attachment description: Add a test program mozilla/security/nss/cmd/tests/dertimetest.c → Add a test program mozilla/security/nss/cmd/tests/dertimetest.c (checked in)
Assignee | ||
Comment 4•14 years ago
|
||
Comment on attachment 418538 [details]
Add a test program mozilla/security/nss/cmd/tests/dertimetest.c (checked in)
I removed the HTML that was inadvertently added to
dertimetest.c in the previous checkin.
Checking in dertimetest.c;
/cvsroot/mozilla/security/nss/cmd/tests/dertimetest.c,v <-- dertimetest.c
new revision: 1.2; previous revision: 1.1
done
Assignee | ||
Comment 5•14 years ago
|
||
I found that these two functions also stop parsing when it has parsed a valid UTCTime/GeneralizedTime string, so they allow junk after that point. This patch fixes that.
Attachment #418724 -
Flags: review?(nelson)
Comment 6•14 years ago
|
||
Comment on attachment 418724 [details] [diff] [review] Disallow junk after a valid UTCTime or GeneralizedTime string Wan-Teh, I am inclined to give this patch r-. Perhaps I am mistaken, but I believe it would seriously break the function DER_AsciiToTime. See the definition of that function as documented at: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/dertime.c&rev=1.11&mark=120-124,126,131#114 If you believe I am mistaken, please explain why you think so.
Comment 7•14 years ago
|
||
(In reply to comment #5) > I found that these two functions also stop parsing when it has parsed a > valid UTCTime/GeneralizedTime string, so they allow junk after that point. > This patch fixes that. I don't consider that behavior a bug, necessarily. It allows a UTCTime or GeneralizedTime string to be parsed in place in the middle of a DER encoded certificate. I believe NSS relies on that behavior.
Assignee | ||
Comment 8•14 years ago
|
||
I was also worried about NSS using these functions to parse a UTCTime or GeneralizedTime string in the middle of a long DER. But such a function needs to have an output parameter that returns the end of the valid time string, such as the second parameter of PR_strtod: http://mxr.mozilla.org/nspr/ident?i=PR_strtod DER_AsciiToTime is another issue. I can redo the patch to not change the behavior of DER_AsciiToTime. In any case, with the maximum length checks I added in the first patch, the amount of junk bytes is limited to just a few bytes, and is extremely unlikely enough for forging a signature or creating a hash collision.
Assignee | ||
Comment 9•14 years ago
|
||
This new patch leaves DER_AsciiToTime unchanged.
Attachment #418724 -
Attachment is obsolete: true
Attachment #418780 -
Flags: review?(nelson)
Attachment #418724 -
Flags: review?(nelson)
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #418780 -
Attachment is obsolete: true
Attachment #418783 -
Flags: review?(nelson)
Attachment #418780 -
Flags: review?(nelson)
Comment 11•14 years ago
|
||
Comment on attachment 418783 [details] [diff] [review] Disallow junk after a valid UTCTime or GeneralizedTime string, v2 (corrected for interdiff; checked in) This looks right to me. r=nelson
Attachment #418783 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 12•14 years ago
|
||
Comment on attachment 418783 [details] [diff] [review] Disallow junk after a valid UTCTime or GeneralizedTime string, v2 (corrected for interdiff; checked in) I checked in this patch on the NSS trunk (NSS 3.12.6). Checking in cmd/tests/dertimetest.c; /cvsroot/mozilla/security/nss/cmd/tests/dertimetest.c,v <-- dertimetest.c new revision: 1.4; previous revision: 1.3 done Checking in lib/util/dertime.c; /cvsroot/mozilla/security/nss/lib/util/dertime.c,v <-- dertime.c new revision: 1.13; previous revision: 1.12 done
Attachment #418783 -
Attachment description: Disallow junk after a valid UTCTime or GeneralizedTime string, v2 (corrected for interdiff) → Disallow junk after a valid UTCTime or GeneralizedTime string, v2 (corrected for interdiff; checked in)
Assignee | ||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•14 years ago
|
||
The endptr argument to der_TimeStringToTime should be declared as const char ** rather than char **: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/dertime.c&rev=1.13&mark=264-265#263 otherwise this assignment generates a compiler warning: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/dertime.c&rev=1.13&mark=330#329 Note: I copied the char ** type from the strtod function: http://www.opengroup.org/onlinepubs/000095399/functions/strtod.html Indeed, our PR_strtod function in prdtoa.c uses a typecast to suppress the warning: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/misc/prdtoa.c&rev=4.14&mark=2505#2503
Attachment #422841 -
Flags: review?(nelson)
Assignee | ||
Updated•14 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 14•14 years ago
|
||
Comment on attachment 422841 [details] [diff] [review] Fix a compiler warning (checked in) r=nelson
Attachment #422841 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 15•14 years ago
|
||
Comment on attachment 422841 [details] [diff] [review] Fix a compiler warning (checked in) I checked in the patch on the NSS trunk (NSS 3.12.6). Checking in dertime.c; /cvsroot/mozilla/security/nss/lib/util/dertime.c,v <-- dertime.c new revision: 1.14; previous revision: 1.13 done
Attachment #422841 -
Attachment description: Fix a compiler warning → Fix a compiler warning (checked in)
Assignee | ||
Updated•14 years ago
|
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•