[PATCH 0/4] X.509: Fix time handling

[PATCH 0/4] X.509: Fix time handling

[David Howells]

Here's a set of patches that fix X.509 time handling in three ways:

 (1) Fix leap year handling.

 (2) Add leap second handling (where you get a time of 23:59:60).

 (3) Add end-of-day midnight encoding (where you get a time of 24:00:00).

David
---
David Howells (4):
      X.509: Fix leap year handling again
      Handle ISO 8601 leap seconds and encodings of midnight in mktime64()
      X.509: Support leap seconds
      X.509: Handle midnight alternative notation in GeneralizedTime


 crypto/asymmetric_keys/x509_cert_parser.c |   12 ++++++------
 kernel/time/time.c                        |    9 ++++++++-
 2 files changed, 14 insertions(+), 7 deletions(-)

[RFC PATCH 1/4] X.509: Fix leap year handling again

[David Howells]

There are still a couple of minor issues in the X.509 leap year handling:

 (1) To avoid doing a modulus-by-400 in addition to a modulus-by-100 when
     determining whether the year is a leap year or not, I divided the year
     by 100 after doing the modulus-by-100, thereby letting the compiler do
     one instruction for both, and then did a modulus-by-4.

     Unfortunately, I then passed the now-modified year value to mktime64()
     to construct a time value.

     Since this isn't a fast path and since mktime64() does a bunch of
     divisions, just condense down to "% 400".  It's also easier to read.

 (2) The default month length for any February where the year doesn't
     divide by four exactly is obtained from the month_length[] array where
     the value is 29, not 28.

     This is fixed by altering the table.

Reported-by: Rudolf Polzer <rpolzer@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-By: David Woodhouse <David.Woodhouse@intel.com>
cc: stable@vger.kernel.org
---

 crypto/asymmetric_keys/x509_cert_parser.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 021d39c0ba75..13c4e5a5fe8c 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -494,7 +494,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
 		     unsigned char tag,
 		     const unsigned char *value, size_t vlen)
 {
-	static const unsigned char month_lengths[] = { 31, 29, 31, 30, 31, 30,
+	static const unsigned char month_lengths[] = { 31, 28, 31, 30, 31, 30,
 						       31, 31, 30, 31, 30, 31 };
 	const unsigned char *p = value;
 	unsigned year, mon, day, hour, min, sec, mon_len;
@@ -540,9 +540,9 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
 		if (year % 4 == 0) {
 			mon_len = 29;
 			if (year % 100 == 0) {
-				year /= 100;
-				if (year % 4 != 0)
-					mon_len = 28;
+				mon_len = 28;
+				if (year % 400 == 0)
+					mon_len = 29;
 			}
 		}
 	}

Re: [RFC PATCH 1/4] X.509: Fix leap year handling again

[Rudolf Polzer]

On Mon, Jan 4, 2016 at 5:17 PM, David Howells <dhowells@redhat.com> wrote:
> There are still a couple of minor issues in the X.509 leap year handling:
>
>  (1) To avoid doing a modulus-by-400 in addition to a modulus-by-100 when
>      determining whether the year is a leap year or not, I divided the year
>      by 100 after doing the modulus-by-100, thereby letting the compiler do
>      one instruction for both, and then did a modulus-by-4.
>
>      Unfortunately, I then passed the now-modified year value to mktime64()
>      to construct a time value.
>
>      Since this isn't a fast path and since mktime64() does a bunch of
>      divisions, just condense down to "% 400".  It's also easier to read.
>
>  (2) The default month length for any February where the year doesn't
>      divide by four exactly is obtained from the month_length[] array where
>      the value is 29, not 28.
>
>      This is fixed by altering the table.
>
> Reported-by: Rudolf Polzer <rpolzer@google.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Acked-By: David Woodhouse <David.Woodhouse@intel.com>
> cc: stable@vger.kernel.org
> ---
>
>  crypto/asymmetric_keys/x509_cert_parser.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 021d39c0ba75..13c4e5a5fe8c 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -494,7 +494,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
>                      unsigned char tag,
>                      const unsigned char *value, size_t vlen)
>  {
> -       static const unsigned char month_lengths[] = { 31, 29, 31, 30, 31, 30,
> +       static const unsigned char month_lengths[] = { 31, 28, 31, 30, 31, 30,
>                                                        31, 31, 30, 31, 30, 31 };
>         const unsigned char *p = value;
>         unsigned year, mon, day, hour, min, sec, mon_len;
> @@ -540,9 +540,9 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
>                 if (year % 4 == 0) {
>                         mon_len = 29;
>                         if (year % 100 == 0) {
> -                               year /= 100;
> -                               if (year % 4 != 0)
> -                                       mon_len = 28;
> +                               mon_len = 28;
> +                               if (year % 400 == 0)
> +                                       mon_len = 29;
>                         }
>                 }
>         }
>

Looks good. In particular this is a lot more legible than
before, as it now reads exactly like the leap year rules everyone
knows.

A small nit: could it be that the month_lengths array also exists
elsewhere in the kernel? Can it possibly be shared? A quick
glance around didn't pop up anything that's not static, so, looks
like it's okay to add yet another one...

Best regards,

Rudolf Polzer

Re: [RFC PATCH 1/4] X.509: Fix leap year handling again

[David Howells]

Rudolf Polzer <rpolzer@google.com> wrote:

> A small nit: could it be that the month_lengths array also exists elsewhere
> in the kernel?  Can it possibly be shared? A quick glance around didn't pop
> up anything that's not static, so, looks like it's okay to add yet another
> one...

Fair point, but outside of the scope of this patch.  The array already exists
that I'm fixing.  It's definitely material for a future patch though.

Note that the following:

	git grep 'month.*[[]'

trawls up a few candidates.

David

[RFC PATCH 2/4] Handle ISO 8601 leap seconds and encodings of midnight in mktime64()

[David Howells]

Handle the following ISO 8601 features in mktime64():

 (1) Leap seconds.

     Leap seconds are indicated by the seconds parameter being the value
     60.  Handle this by treating it the same as 00 of the following
     minute.

 (2) Alternate encodings of midnight.

     Two different encodings of midnight are permitted - 00:00:00 and
     24:00:00 - the first is midnight today and the second is midnight
     tomorrow and is exactly equivalent to the first with tomorrow's date.

As it happens, we don't actually need to change mktime64() to handle either
of these - just comment them as valid parameters.

These facility will be used by the X.509 parser.  Doing it in mktime64()
makes the policy common to the whole kernel and easier to find.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Arnd Bergmann <arnd@arndb.de>
cc: John Stultz <john.stultz@linaro.org>
---

 kernel/time/time.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/time/time.c b/kernel/time/time.c
index 86751c68e08d..be115b020d27 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -322,6 +322,13 @@ EXPORT_SYMBOL(timespec_trunc);
  * -year/100+year/400 terms, and add 10.]
  *
  * This algorithm was first published by Gauss (I think).
+ *
+ * A leap second can be indicated by calling this function with sec as
+ * 60 (allowable under ISO 8601).  The leap second is treated the same
+ * as the following second since they don't exist in UNIX time.
+ *
+ * An encoding of midnight at the end of the day as 24:00:00 - ie. midnight
+ * tomorrow - (allowable under ISO 8601) is supported.
  */
 time64_t mktime64(const unsigned int year0, const unsigned int mon0,
 		const unsigned int day, const unsigned int hour,
@@ -338,7 +345,7 @@ time64_t mktime64(const unsigned int year0, const unsigned int mon0,
 	return ((((time64_t)
 		  (year/4 - year/100 + year/400 + 367*mon/12 + day) +
 		  year*365 - 719499
-	    )*24 + hour /* now have hours */
+	    )*24 + hour /* now have hours - midnight tomorrow handled here */
 	  )*60 + min /* now have minutes */
 	)*60 + sec; /* finally seconds */
 }

[RFC PATCH 3/4] X.509: Support leap seconds

[David Howells]

The format of ASN.1 GeneralizedTime seems to be specified by ISO 8601
[X.680 46.3] and this apparently supports leap seconds (ie. the seconds
field is 60).  It's not entirely clear that ASN.1 expects it, but we can
relax the seconds check slightly for GeneralizedTime.

This results in us passing a time with sec as 60 to mktime64(), which
handles it as being a duplicate of the 0th second of the next minute.

We can't really do otherwise without giving the kernel much greater
knowledge of where all the leap seconds are.  Unfortunately, this would
require change the mapping of the kernel's current-time-in-seconds.

UTCTime, however, only supports a seconds value in the range 00-59, but for
the sake of simplicity allow this with UTCTime also.

Without this patch, certain X.509 certificates will be rejected,
potentially making a kernel unbootable.

Reported-by: Rudolf Polzer <rpolzer@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Arnd Bergmann <arnd@arndb.de>
cc: David Woodhouse <David.Woodhouse@intel.com>
cc: John Stultz <john.stultz@linaro.org>
---

 crypto/asymmetric_keys/x509_cert_parser.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 13c4e5a5fe8c..3379c0ba3988 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -550,7 +550,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
 	if (day < 1 || day > mon_len ||
 	    hour > 23 ||
 	    min > 59 ||
-	    sec > 59)
+	    sec > 60) /* ISO 8601 permits leap seconds [X.680 46.3] */
 		goto invalid_time;
 
 	*_t = mktime64(year, mon, day, hour, min, sec);

Re: [RFC PATCH 3/4] X.509: Support leap seconds

[Rudolf Polzer]

On Mon, Jan 4, 2016 at 5:17 PM, David Howells <dhowells@redhat.com> wrote:
> The format of ASN.1 GeneralizedTime seems to be specified by ISO 8601
> [X.680 46.3] and this apparently supports leap seconds (ie. the seconds
> field is 60).  It's not entirely clear that ASN.1 expects it, but we can
> relax the seconds check slightly for GeneralizedTime.
>
> This results in us passing a time with sec as 60 to mktime64(), which
> handles it as being a duplicate of the 0th second of the next minute.
>
> We can't really do otherwise without giving the kernel much greater
> knowledge of where all the leap seconds are.  Unfortunately, this would
> require change the mapping of the kernel's current-time-in-seconds.
>
> UTCTime, however, only supports a seconds value in the range 00-59, but for
> the sake of simplicity allow this with UTCTime also.
>
> Without this patch, certain X.509 certificates will be rejected,
> potentially making a kernel unbootable.
>
> Reported-by: Rudolf Polzer <rpolzer@google.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Arnd Bergmann <arnd@arndb.de>
> cc: David Woodhouse <David.Woodhouse@intel.com>
> cc: John Stultz <john.stultz@linaro.org>
> ---
>
>  crypto/asymmetric_keys/x509_cert_parser.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 13c4e5a5fe8c..3379c0ba3988 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -550,7 +550,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
>         if (day < 1 || day > mon_len ||
>             hour > 23 ||
>             min > 59 ||
> -           sec > 59)
> +           sec > 60) /* ISO 8601 permits leap seconds [X.680 46.3] */
>                 goto invalid_time;
>
>         *_t = mktime64(year, mon, day, hour, min, sec);
>

All good. I find it a bit odd that it only allows for _one_ leap
second, although there can be technically two in a single minute - but
this is the problem of the standard, not of this code.

Best regards,

Rudolf Polzer

[RFC PATCH 4/4] X.509: Handle midnight alternative notation in GeneralizedTime

[David Howells]

The ASN.1 GeneralizedTime object carries an ISO 8601 format date and time.
The time is permitted to show midnight as 00:00 or 24:00 (the latter being
equivalent of 00:00 of the following day).

The permitted value is checked in x509_decode_time() but the actual
handling is left to mktime64().

Without this patch, certain X.509 certificates will be rejected and could
lead to an unbootable kernel.

Note that with this patch we also permit any 24:mm:ss time and extend this
to UTCTime, which whilst not strictly correct don't permit much leeway in
fiddling date strings.

Reported-by: Rudolf Polzer <rpolzer@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: David Woodhouse <David.Woodhouse@intel.com>
cc: John Stultz <john.stultz@linaro.org>
cc: Arnd Bergmann <arnd@arndb.de>
---

 crypto/asymmetric_keys/x509_cert_parser.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 3379c0ba3988..70ed0852fdb2 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -548,7 +548,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
 	}
 
 	if (day < 1 || day > mon_len ||
-	    hour > 23 ||
+	    hour > 24 || /* ISO 8601 permits 24:00:00 as midnight tomorrow */
 	    min > 59 ||
 	    sec > 60) /* ISO 8601 permits leap seconds [X.680 46.3] */
 		goto invalid_time;

Re: [RFC PATCH 4/4] X.509: Handle midnight alternative notation in GeneralizedTime

[Rudolf Polzer]

On Mon, Jan 4, 2016 at 5:17 PM, David Howells <dhowells@redhat.com> wrote:
> The ASN.1 GeneralizedTime object carries an ISO 8601 format date and time.
> The time is permitted to show midnight as 00:00 or 24:00 (the latter being
> equivalent of 00:00 of the following day).
>
> The permitted value is checked in x509_decode_time() but the actual
> handling is left to mktime64().
>
> Without this patch, certain X.509 certificates will be rejected and could
> lead to an unbootable kernel.
>
> Note that with this patch we also permit any 24:mm:ss time and extend this
> to UTCTime, which whilst not strictly correct don't permit much leeway in
> fiddling date strings.
>
> Reported-by: Rudolf Polzer <rpolzer@google.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: David Woodhouse <David.Woodhouse@intel.com>
> cc: John Stultz <john.stultz@linaro.org>
> cc: Arnd Bergmann <arnd@arndb.de>
> ---
>
>  crypto/asymmetric_keys/x509_cert_parser.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 3379c0ba3988..70ed0852fdb2 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -548,7 +548,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
>         }
>
>         if (day < 1 || day > mon_len ||
> -           hour > 23 ||
> +           hour > 24 || /* ISO 8601 permits 24:00:00 as midnight tomorrow */
>             min > 59 ||
>             sec > 60) /* ISO 8601 permits leap seconds [X.680 46.3] */
>                 goto invalid_time;
>

Looks good.

As for 24:xx:yy times - I'm split about this. This code doesn't
require a bijective decoding anyway (and if it did, 24:00:00 and
00:00:00 mapping to the same time64_t would be problem enough) so this
is sure safe. On the other hand, a cert with a 24:xx:yy time that's
not 24:00:00 probably should be regarded as invalid and not trusted
for that reason alone.

Best regards,

Rudolf Polzer

Re: [RFC PATCH 4/4] X.509: Handle midnight alternative notation in GeneralizedTime

[David Howells]

Rudolf Polzer <rpolzer@google.com> wrote:

> As for 24:xx:yy times - I'm split about this. This code doesn't
> require a bijective decoding anyway (and if it did, 24:00:00 and
> 00:00:00 mapping to the same time64_t would be problem enough) so this
> is sure safe. On the other hand, a cert with a 24:xx:yy time that's
> not 24:00:00 probably should be regarded as invalid and not trusted
> for that reason alone.

Feel free to argue that one with Linus:-/

David

Re: [RFC PATCH 4/4] X.509: Handle midnight alternative notation in GeneralizedTime

[One Thousand Gnomes]

On Wed, 20 Jan 2016 15:20:00 +0000
David Howells <dhowells@redhat.com> wrote:

> Rudolf Polzer <rpolzer@google.com> wrote:
> 
> > As for 24:xx:yy times - I'm split about this. This code doesn't
> > require a bijective decoding anyway (and if it did, 24:00:00 and
> > 00:00:00 mapping to the same time64_t would be problem enough) so this
> > is sure safe. On the other hand, a cert with a 24:xx:yy time that's
> > not 24:00:00 probably should be regarded as invalid and not trusted
> > for that reason alone.
> 
> Feel free to argue that one with Linus:-/

Pedantic hat on 24:00:01 is also potentially valid. The ANSI and ISO
standards allow for double leap seconds even though CCIR-460 doesn't
allow it to actually happen.

Alan

Re: [RFC PATCH 4/4] X.509: Handle midnight alternative notation in GeneralizedTime

[David Howells]

One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:

> Pedantic hat on 24:00:01 is also potentially valid. The ANSI and ISO
> standards allow for double leap seconds even though CCIR-460 doesn't
> allow it to actually happen.

As far as I can tell without actually buying a copy of ISO 8601, that standard
only allows for a single leap second in a day.

I grant that that doesn't mean that other standards may permit multiple leap
seconds.

David

Re: [RFC PATCH 4/4] X.509: Handle midnight alternative notation in GeneralizedTime

[David Howells]

One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:

> > > As for 24:xx:yy times - I'm split about this. This code doesn't
> > > require a bijective decoding anyway (and if it did, 24:00:00 and
> > > 00:00:00 mapping to the same time64_t would be problem enough) so this
> > > is sure safe. On the other hand, a cert with a 24:xx:yy time that's
> > > not 24:00:00 probably should be regarded as invalid and not trusted
> > > for that reason alone.
> > 
> > Feel free to argue that one with Linus:-/
> 
> Pedantic hat on 24:00:01 is also potentially valid. The ANSI and ISO
> standards allow for double leap seconds even though CCIR-460 doesn't
> allow it to actually happen.

Did you mean 24:00:01 or 23:59:61?

David

Re: [RFC PATCH 4/4] X.509: Handle midnight alternative notation in GeneralizedTime

[One Thousand Gnomes]

On Wed, 03 Feb 2016 16:28:41 +0000
David Howells <dhowells@redhat.com> wrote:

> One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> 
> > > > As for 24:xx:yy times - I'm split about this. This code doesn't
> > > > require a bijective decoding anyway (and if it did, 24:00:00 and
> > > > 00:00:00 mapping to the same time64_t would be problem enough) so this
> > > > is sure safe. On the other hand, a cert with a 24:xx:yy time that's
> > > > not 24:00:00 probably should be regarded as invalid and not trusted
> > > > for that reason alone.  
> > > 
> > > Feel free to argue that one with Linus:-/  
> > 
> > Pedantic hat on 24:00:01 is also potentially valid. The ANSI and ISO
> > standards allow for double leap seconds even though CCIR-460 doesn't
> > allow it to actually happen.  
> 
> Did you mean 24:00:01 or 23:59:61?

I'd have to actually go and real all the standards for that one.

Alan