Closed Bug 536023 Opened 12 years ago Closed 12 years ago

DER_UTCTimeToTime and DER_GeneralizedTimeToTime ignore all bytes after an embedded null

Categories

(NSS :: Libraries, defect, P2)

3.12
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.6

People

(Reporter: wtc, Assigned: wtc)

Details

Attachments

(4 files, 2 obsolete files)

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)
Attachment #418538 - Flags: review?(nelson) → review+
Attachment #418537 - Flags: review?(nelson) → review+
Priority: -- → P2
Version: unspecified → 3.12
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)
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)
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
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 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.
(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.
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.
This new patch leaves DER_AsciiToTime unchanged.
Attachment #418724 - Attachment is obsolete: true
Attachment #418780 - Flags: review?(nelson)
Attachment #418724 - Flags: review?(nelson)
Attachment #418780 - Attachment is obsolete: true
Attachment #418783 - Flags: review?(nelson)
Attachment #418780 - Flags: review?(nelson)
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+
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)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 422841 [details] [diff] [review]
Fix a compiler warning (checked in)

r=nelson
Attachment #422841 - Flags: review?(nelson) → review+
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)
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.