937976 - libssl stores current time in gmt_unix_time field of ClientHello and ServerHello; should use random value

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: stop-using-time-for-gmt_unix_time-field.patch

See http://www.ietf.org/mail-archive/web/tls/current/msg09861.html
and https://github.com/openssl/openssl/pull/23
and https://github.com/openssl/openssl/pull/29

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

http://hg.mozilla.org/projects/nss/rev/99b8adde480e

Comment 2

[Wan-Teh Chang]

Attachment: Remove a stale comment in ssl3_GetNewRandom

Brian, Adam: are you guys sure about not filling gmt_unix_time with the
current time? I know some people are interested in using the gmt_unix_time
in ServerHello as a time source or hint, e.g., to detect a false certificate
expired error due to incorrect system clock on the client computer.

Comment 3

[Adam Langley]

I think this is a good change for clients and should land in NSS.

We do have some cases where the time from a google.com ServerHello is used, so I wouldn't make a similar change there immediately, but maybe we'll switch to using a Date header or something in the future.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8342565 [details] [diff] [review]
Remove a stale comment in ssl3_GetNewRandom

Review of attachment 8342565 [details] [diff] [review]:
-----------------------------------------------------------------

I purposely left that comment in so that it was more clear that we're intentionally not doing anything special with the gmt_unix_time field. But, I don't have strong feelings either way about whether the comment stays or goes.

Comment 5

[Wan-Teh Chang]

Comment on attachment 831265 [details] [diff] [review]
stop-using-time-for-gmt_unix_time-field.patch

Review of attachment 831265 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ -888,2 @@
>      /* first 4 bytes are reserverd for time */
> -    rv = PK11_GenerateRandom(&random->rand[4], SSL3_RANDOM_LENGTH - 4);

Brian: this comment only makes sense when the code uses the magic array index 4.

If you want to keep the comment, you need to update it to explain why the first
4 bytes are not the gmt_unix_time as specified in the TLS protocol.