536023 - DER_UTCTimeToTime and DER_GeneralizedTimeToTime ignore all bytes after an embedded null

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Proposed patch (checked in)

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.

Comment 1

[Wan-Teh Chang]

Attachment: Add a test program mozilla/security/nss/cmd/tests/dertimetest.c (checked in)

Comment 2

[Wan-Teh Chang]

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

Comment 3

[Wan-Teh Chang]

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

Comment 4

[Wan-Teh Chang]

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

Comment 5

[Wan-Teh Chang]

Attachment: Disallow junk after a valid UTCTime or GeneralizedTime string

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.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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

[Nelson Bolyard (seldom reads bugmail)]

(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.

Comment 8

[Wan-Teh Chang]

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.

Comment 9

[Wan-Teh Chang]

Attachment: Disallow junk after a valid UTCTime or GeneralizedTime string, v2

This new patch leaves DER_AsciiToTime unchanged.

Comment 10

[Wan-Teh Chang]

Attachment: Disallow junk after a valid UTCTime or GeneralizedTime string, v2 (corrected for interdiff; checked in)

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 12

[Wan-Teh Chang]

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

Comment 13

[Wan-Teh Chang]

Attachment: Fix a compiler warning (checked in)

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

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 422841 [details] [diff] [review]
Fix a compiler warning (checked in)

r=nelson

Comment 15

[Wan-Teh Chang]

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