[PATCH] X.509: Remove certificate date checks

[PATCH] X.509: Remove certificate date checks

[David Howells]

Remove the certificate date checks that are performed when a certificate is
parsed.  There are two checks: a valid from and a valid to.  The first check is
causing a lot of problems with system clocks that don't keep good time and the
second places an implicit expiry date upon the kernel when used for module
signing, so do we really need them?

Signed-off-by: David Howells <dhowells@redhat.com>
cc: David Woodhouse <dwmw2@infradead.org>
cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Josh Boyer <jwboyer@redhat.com>
cc: Alexander Holler <holler@ahsoftware.de>
cc: stable@vger.kernel.org
---

 crypto/asymmetric_keys/x509.asn1          |    4 +-
 crypto/asymmetric_keys/x509_cert_parser.c |   64 -----------------------------
 crypto/asymmetric_keys/x509_parser.h      |    2 -
 crypto/asymmetric_keys/x509_public_key.c  |   46 ---------------------
 4 files changed, 2 insertions(+), 114 deletions(-)

diff --git a/crypto/asymmetric_keys/x509.asn1 b/crypto/asymmetric_keys/x509.asn1
index bf32b3d..b448099 100644
--- a/crypto/asymmetric_keys/x509.asn1
+++ b/crypto/asymmetric_keys/x509.asn1
@@ -35,8 +35,8 @@ AttributeValueAssertion ::= SEQUENCE {
 	}
 
 Validity ::= SEQUENCE {
-	notBefore		Time ({ x509_note_not_before }),
-	notAfter		Time ({ x509_note_not_after })
+	notBefore		Time,
+	notAfter		Time
 	}
 
 Time ::= CHOICE {
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 7fabc4c..3987508 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -430,67 +430,3 @@ int x509_process_extension(void *context, size_t hdrlen,
 
 	return 0;
 }
-
-/*
- * Record a certificate time.
- */
-static int x509_note_time(struct tm *tm,  size_t hdrlen,
-			  unsigned char tag,
-			  const unsigned char *value, size_t vlen)
-{
-	const unsigned char *p = value;
-
-#define dec2bin(X) ((X) - '0')
-#define DD2bin(P) ({ unsigned x = dec2bin(P[0]) * 10 + dec2bin(P[1]); P += 2; x; })
-
-	if (tag == ASN1_UNITIM) {
-		/* UTCTime: YYMMDDHHMMSSZ */
-		if (vlen != 13)
-			goto unsupported_time;
-		tm->tm_year = DD2bin(p);
-		if (tm->tm_year >= 50)
-			tm->tm_year += 1900;
-		else
-			tm->tm_year += 2000;
-	} else if (tag == ASN1_GENTIM) {
-		/* GenTime: YYYYMMDDHHMMSSZ */
-		if (vlen != 15)
-			goto unsupported_time;
-		tm->tm_year = DD2bin(p) * 100 + DD2bin(p);
-	} else {
-		goto unsupported_time;
-	}
-
-	tm->tm_year -= 1900;
-	tm->tm_mon  = DD2bin(p) - 1;
-	tm->tm_mday = DD2bin(p);
-	tm->tm_hour = DD2bin(p);
-	tm->tm_min  = DD2bin(p);
-	tm->tm_sec  = DD2bin(p);
-
-	if (*p != 'Z')
-		goto unsupported_time;
-
-	return 0;
-
-unsupported_time:
-	pr_debug("Got unsupported time [tag %02x]: '%*.*s'\n",
-		 tag, (int)vlen, (int)vlen, value);
-	return -EBADMSG;
-}
-
-int x509_note_not_before(void *context, size_t hdrlen,
-			 unsigned char tag,
-			 const void *value, size_t vlen)
-{
-	struct x509_parse_context *ctx = context;
-	return x509_note_time(&ctx->cert->valid_from, hdrlen, tag, value, vlen);
-}
-
-int x509_note_not_after(void *context, size_t hdrlen,
-			unsigned char tag,
-			const void *value, size_t vlen)
-{
-	struct x509_parse_context *ctx = context;
-	return x509_note_time(&ctx->cert->valid_to, hdrlen, tag, value, vlen);
-}
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index f86dc5f..8ee6f8e 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -18,8 +18,6 @@ struct x509_certificate {
 	char		*subject;		/* Name of certificate subject */
 	char		*fingerprint;		/* Key fingerprint as hex */
 	char		*authority;		/* Authority key fingerprint as hex */
-	struct tm	valid_from;
-	struct tm	valid_to;
 	enum pkey_algo	pkey_algo : 8;		/* Public key algorithm */
 	enum pkey_algo	sig_pkey_algo : 8;	/* Signature public key algorithm */
 	enum pkey_hash_algo sig_hash_algo : 8;	/* Signature hash algorithm */
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 06007f0..0d1fcfb 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -106,7 +106,6 @@ error_no_sig:
 static int x509_key_preparse(struct key_preparsed_payload *prep)
 {
 	struct x509_certificate *cert;
-	struct tm now;
 	size_t srlen, sulen;
 	char *desc = NULL;
 	int ret;
@@ -118,14 +117,6 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 	pr_devel("Cert Issuer: %s\n", cert->issuer);
 	pr_devel("Cert Subject: %s\n", cert->subject);
 	pr_devel("Cert Key Algo: %s\n", pkey_algo[cert->pkey_algo]);
-	pr_devel("Cert Valid From: %04ld-%02d-%02d %02d:%02d:%02d\n",
-		 cert->valid_from.tm_year + 1900, cert->valid_from.tm_mon + 1,
-		 cert->valid_from.tm_mday, cert->valid_from.tm_hour,
-		 cert->valid_from.tm_min,  cert->valid_from.tm_sec);
-	pr_devel("Cert Valid To: %04ld-%02d-%02d %02d:%02d:%02d\n",
-		 cert->valid_to.tm_year + 1900, cert->valid_to.tm_mon + 1,
-		 cert->valid_to.tm_mday, cert->valid_to.tm_hour,
-		 cert->valid_to.tm_min,  cert->valid_to.tm_sec);
 	pr_devel("Cert Signature: %s + %s\n",
 		 pkey_algo[cert->sig_pkey_algo],
 		 pkey_hash_algo[cert->sig_hash_algo]);
@@ -137,43 +128,6 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 		goto error_free_cert;
 	}
 
-	time_to_tm(CURRENT_TIME.tv_sec, 0, &now);
-	pr_devel("Now: %04ld-%02d-%02d %02d:%02d:%02d\n",
-		 now.tm_year + 1900, now.tm_mon + 1, now.tm_mday,
-		 now.tm_hour, now.tm_min,  now.tm_sec);
-	if (now.tm_year < cert->valid_from.tm_year ||
-	    (now.tm_year == cert->valid_from.tm_year &&
-	     (now.tm_mon < cert->valid_from.tm_mon ||
-	      (now.tm_mon == cert->valid_from.tm_mon &&
-	       (now.tm_mday < cert->valid_from.tm_mday ||
-		(now.tm_mday == cert->valid_from.tm_mday &&
-		 (now.tm_hour < cert->valid_from.tm_hour ||
-		  (now.tm_hour == cert->valid_from.tm_hour &&
-		   (now.tm_min < cert->valid_from.tm_min ||
-		    (now.tm_min == cert->valid_from.tm_min &&
-		     (now.tm_sec < cert->valid_from.tm_sec
-		      ))))))))))) {
-		pr_warn("Cert %s is not yet valid\n", cert->fingerprint);
-		ret = -EKEYREJECTED;
-		goto error_free_cert;
-	}
-	if (now.tm_year > cert->valid_to.tm_year ||
-	    (now.tm_year == cert->valid_to.tm_year &&
-	     (now.tm_mon > cert->valid_to.tm_mon ||
-	      (now.tm_mon == cert->valid_to.tm_mon &&
-	       (now.tm_mday > cert->valid_to.tm_mday ||
-		(now.tm_mday == cert->valid_to.tm_mday &&
-		 (now.tm_hour > cert->valid_to.tm_hour ||
-		  (now.tm_hour == cert->valid_to.tm_hour &&
-		   (now.tm_min > cert->valid_to.tm_min ||
-		    (now.tm_min == cert->valid_to.tm_min &&
-		     (now.tm_sec > cert->valid_to.tm_sec
-		      ))))))))))) {
-		pr_warn("Cert %s has expired\n", cert->fingerprint);
-		ret = -EKEYEXPIRED;
-		goto error_free_cert;
-	}
-
 	cert->pub->algo = x509_public_key_algorithms[cert->pkey_algo];
 	cert->pub->id_type = PKEY_ID_X509;
 

Re: [PATCH] X.509: Remove certificate date checks

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

On Thu, 2013-03-14 at 12:34 +0000, David Howells wrote:
> Remove the certificate date checks that are performed when a certificate is
> parsed.  There are two checks: a valid from and a valid to.  The first check is
> causing a lot of problems with system clocks that don't keep good time and the
> second places an implicit expiry date upon the kernel when used for module
> signing, so do we really need them?

While the date check is entirely bogus for the specific case of module
signing, I don't think we necessarily ought to rip it out of our generic
X.509 support entirely.

Some use cases *might* want to check the dates, and should be permitted
to do so. Just don't refuse to even *parse* the key outside its valid
date range... :)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] X.509: Remove certificate date checks

[Alexander Holler]

Am 14.03.2013 13:48, schrieb David Woodhouse:
> On Thu, 2013-03-14 at 12:34 +0000, David Howells wrote:
>> Remove the certificate date checks that are performed when a certificate is
>> parsed.  There are two checks: a valid from and a valid to.  The first check is
>> causing a lot of problems with system clocks that don't keep good time and the
>> second places an implicit expiry date upon the kernel when used for module
>> signing, so do we really need them?
>
> While the date check is entirely bogus for the specific case of module
> signing, I don't think we necessarily ought to rip it out of our generic
> X.509 support entirely.
>
> Some use cases *might* want to check the dates, and should be permitted
> to do so. Just don't refuse to even *parse* the key outside its valid
> date range... :)

Agreed (thats what my patch did).

I've introduced a new config option because I don't know if something (a 
use case I don't know) relies on the validity check of the dates in the 
parser. If there currently isn't such a user, just removing the validity 
check in the parser might be enough. Offering the parsed dates for later 
usage is still a good idea.

Regards,

Alexander

Re: [PATCH] X.509: Remove certificate date checks

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

On Thu, 2013-03-14 at 17:22 +0100, Alexander Holler wrote:
> 
> Agreed (thats what my patch did).
> 
> I've introduced a new config option because I don't know if something (a 
> use case I don't know) relies on the validity check of the dates in the 
> parser. If there currently isn't such a user, just removing the validity 
> check in the parser might be enough. 

Is there *is* such a user, it's broken already. The key could have been
loaded (and passed the existing check) *months* ago, expired seconds
after it was loaded, and your hypothetical user could still be happily
trusting it.

> Offering the parsed dates for later usage is still a good idea.

Right.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] X.509: Remove certificate date checks

[Alexander Holler]

Am 14.03.2013 18:09, schrieb David Woodhouse:
> On Thu, 2013-03-14 at 17:22 +0100, Alexander Holler wrote:
>>
>> Agreed (thats what my patch did).
>>
>> I've introduced a new config option because I don't know if something (a
>> use case I don't know) relies on the validity check of the dates in the
>> parser. If there currently isn't such a user, just removing the validity
>> check in the parser might be enough.
>
> Is there *is* such a user, it's broken already. The key could have been
> loaded (and passed the existing check) *months* ago, expired seconds
> after it was loaded, and your hypothetical user could still be happily
> trusting it.

As the user (program or whatever) calls the parser, he knows if he can 
trust it to validate dates. So there might be something for which the 
current implementation works (parsing date = using date).

I just don't know, because I've only discovered that glitch while trying 
to use modsign to be sure no unsigned module (I've compiled myself) will 
be become loaded (I compile the kernel and delete the keys right 
afterwards). So I don't know anything if and how the crypto-api to load 
x.509 keys is used besides modsign. ;)

Regards,

Alexander