[openssl-commits] [openssl] master update
Emilia Kasper
emilia at openssl.org
Fri Feb 24 17:09:08 UTC 2017
The branch master has been updated
via 80770da39ebba0101079477611b7ce2f426653c5 (commit)
from b169c0ec40408566270fb638bcbfab01a0d2dc60 (commit)
- Log -----------------------------------------------------------------
commit 80770da39ebba0101079477611b7ce2f426653c5
Author: Emilia Kasper <emilia at openssl.org>
Date: Fri Feb 17 19:00:15 2017 +0100
X509 time: tighten validation per RFC 5280
- Reject fractional seconds
- Reject offsets
- Check that the date/time digits are in valid range.
- Add documentation for X509_cmp_time
GH issue 2620
Reviewed-by: Richard Levitte <levitte at openssl.org>
Reviewed-by: Rich Salz <rsalz at openssl.org>
-----------------------------------------------------------------------
Summary of changes:
CHANGES | 5 +
crypto/x509/x509_vfy.c | 147 +++++----------
doc/man3/X509_cmp_time.pod | 39 ++++
test/build.info | 6 +-
.../{90-test_bioprint.t => 60-test_x509_time.t} | 4 +-
test/x509_time_test.c | 201 +++++++++++++++++++++
6 files changed, 300 insertions(+), 102 deletions(-)
create mode 100644 doc/man3/X509_cmp_time.pod
copy test/recipes/{90-test_bioprint.t => 60-test_x509_time.t} (72%)
create mode 100644 test/x509_time_test.c
diff --git a/CHANGES b/CHANGES
index 3e91a08..cda3790 100644
--- a/CHANGES
+++ b/CHANGES
@@ -4,6 +4,11 @@
Changes between 1.1.0e and 1.1.1 [xx XXX xxxx]
+ *) Certificate time validation (X509_cmp_time) enforces stricter
+ compliance with RFC 5280. Fractional seconds and timezone offsets
+ are no longer allowed.
+ [Emilia Käsper]
+
*) Add support for SipHash
[Todd Short]
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index ebc4424..2f1cd1a 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -7,6 +7,7 @@
* https://www.openssl.org/source/license.html
*/
+#include <ctype.h>
#include <stdio.h>
#include <time.h>
#include <errno.h>
@@ -1754,119 +1755,67 @@ int X509_cmp_current_time(const ASN1_TIME *ctm)
int X509_cmp_time(const ASN1_TIME *ctm, time_t *cmp_time)
{
- char *str;
- ASN1_TIME atm;
- long offset;
- char buff1[24], buff2[24], *p;
- int i, j, remaining;
+ static const size_t utctime_length = sizeof("YYMMDDHHMMSSZ") - 1;
+ static const size_t generalizedtime_length = sizeof("YYYYMMDDHHMMSSZ") - 1;
+ ASN1_TIME *asn1_cmp_time = NULL;
+ int i, day, sec, ret = 0;
- p = buff1;
- remaining = ctm->length;
- str = (char *)ctm->data;
/*
- * Note that the following (historical) code allows much more slack in the
- * time format than RFC5280. In RFC5280, the representation is fixed:
+ * Note that ASN.1 allows much more slack in the time format than RFC5280.
+ * In RFC5280, the representation is fixed:
* UTCTime: YYMMDDHHMMSSZ
* GeneralizedTime: YYYYMMDDHHMMSSZ
+ *
+ * We do NOT currently enforce the following RFC 5280 requirement:
+ * "CAs conforming to this profile MUST always encode certificate
+ * validity dates through the year 2049 as UTCTime; certificate validity
+ * dates in 2050 or later MUST be encoded as GeneralizedTime."
*/
- if (ctm->type == V_ASN1_UTCTIME) {
- /* YYMMDDHHMM[SS]Z or YYMMDDHHMM[SS](+-)hhmm */
- int min_length = sizeof("YYMMDDHHMMZ") - 1;
- int max_length = sizeof("YYMMDDHHMMSS+hhmm") - 1;
- if (remaining < min_length || remaining > max_length)
+ switch (ctm->type) {
+ case V_ASN1_UTCTIME:
+ if (ctm->length != (int)(utctime_length))
return 0;
- memcpy(p, str, 10);
- p += 10;
- str += 10;
- remaining -= 10;
- } else {
- /* YYYYMMDDHHMM[SS[.fff]]Z or YYYYMMDDHHMM[SS[.f[f[f]]]](+-)hhmm */
- int min_length = sizeof("YYYYMMDDHHMMZ") - 1;
- int max_length = sizeof("YYYYMMDDHHMMSS.fff+hhmm") - 1;
- if (remaining < min_length || remaining > max_length)
+ break;
+ case V_ASN1_GENERALIZEDTIME:
+ if (ctm->length != (int)(generalizedtime_length))
return 0;
- memcpy(p, str, 12);
- p += 12;
- str += 12;
- remaining -= 12;
+ break;
+ default:
+ return 0;
}
- if ((*str == 'Z') || (*str == '-') || (*str == '+')) {
- *(p++) = '0';
- *(p++) = '0';
- } else {
- /* SS (seconds) */
- if (remaining < 2)
+ /**
+ * Verify the format: the ASN.1 functions we use below allow a more
+ * flexible format than what's mandated by RFC 5280.
+ * Digit and date ranges will be verified in the conversion methods.
+ */
+ for (i = 0; i < ctm->length - 1; i++) {
+ if (!isdigit(ctm->data[i]))
return 0;
- *(p++) = *(str++);
- *(p++) = *(str++);
- remaining -= 2;
- /*
- * Skip any (up to three) fractional seconds...
- * TODO(emilia): in RFC5280, fractional seconds are forbidden.
- * Can we just kill them altogether?
- */
- if (remaining && *str == '.') {
- str++;
- remaining--;
- for (i = 0; i < 3 && remaining; i++, str++, remaining--) {
- if (*str < '0' || *str > '9')
- break;
- }
- }
-
}
- *(p++) = 'Z';
- *(p++) = '\0';
-
- /* We now need either a terminating 'Z' or an offset. */
- if (!remaining)
+ if (ctm->data[ctm->length - 1] != 'Z')
return 0;
- if (*str == 'Z') {
- if (remaining != 1)
- return 0;
- offset = 0;
- } else {
- /* (+-)HHMM */
- if ((*str != '+') && (*str != '-'))
- return 0;
- /* Historical behaviour: the (+-)hhmm offset is forbidden in RFC5280. */
- if (remaining != 5)
- return 0;
- if (str[1] < '0' || str[1] > '9' || str[2] < '0' || str[2] > '9' ||
- str[3] < '0' || str[3] > '9' || str[4] < '0' || str[4] > '9')
- return 0;
- offset = ((str[1] - '0') * 10 + (str[2] - '0')) * 60;
- offset += (str[3] - '0') * 10 + (str[4] - '0');
- if (*str == '-')
- offset = -offset;
- }
- atm.type = ctm->type;
- atm.flags = 0;
- atm.length = sizeof(buff2);
- atm.data = (unsigned char *)buff2;
- if (X509_time_adj(&atm, offset * 60, cmp_time) == NULL)
- return 0;
+ /*
+ * There is ASN1_UTCTIME_cmp_time_t but no
+ * ASN1_GENERALIZEDTIME_cmp_time_t or ASN1_TIME_cmp_time_t,
+ * so we go through ASN.1
+ */
+ asn1_cmp_time = X509_time_adj(NULL, 0, cmp_time);
+ if (asn1_cmp_time == NULL)
+ goto err;
+ if (!ASN1_TIME_diff(&day, &sec, ctm, asn1_cmp_time))
+ goto err;
- if (ctm->type == V_ASN1_UTCTIME) {
- i = (buff1[0] - '0') * 10 + (buff1[1] - '0');
- if (i < 50)
- i += 100; /* cf. RFC 2459 */
- j = (buff2[0] - '0') * 10 + (buff2[1] - '0');
- if (j < 50)
- j += 100;
-
- if (i < j)
- return -1;
- if (i > j)
- return 1;
- }
- i = strcmp(buff1, buff2);
- if (i == 0) /* wait a second then return younger :-) */
- return -1;
- else
- return i;
+ /*
+ * X509_cmp_time comparison is <=.
+ * The return value 0 is reserved for errors.
+ */
+ ret = (day >= 0 && sec >= 0) ? -1 : 1;
+
+ err:
+ ASN1_TIME_free(asn1_cmp_time);
+ return ret;
}
ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj)
diff --git a/doc/man3/X509_cmp_time.pod b/doc/man3/X509_cmp_time.pod
new file mode 100644
index 0000000..31826ad
--- /dev/null
+++ b/doc/man3/X509_cmp_time.pod
@@ -0,0 +1,39 @@
+=pod
+
+=head1 NAME
+
+X509_cmp_time - X509 time functions
+
+=head1 SYNOPSIS
+
+ X509_cmp_time(const ASN1_TIME *asn1_time, time_t *cmp_time);
+
+=head1 DESCRIPTION
+
+X509_cmp_time() compares the ASN1_TIME in B<asn1_time> with the time in
+<cmp_time>.
+
+B<asn1_time> must satisfy the ASN1_TIME format mandated by RFC 5280, i.e.,
+its format must be either YYMMDDHHMMSSZ or YYYYMMDDHHMMSSZ.
+
+If B<cmp_time> is NULL the current time is used.
+
+=head1 BUGS
+
+Unlike many standard comparison functions, X509_cmp_time returns 0 on error.
+
+=head1 RETURN VALUES
+
+X509_cmp_time() returns -1 if B<asn1_time> is earlier than, or equal to,
+B<cmp_time>, and 1 otherwise. It returns 0 on error.
+
+=head1 COPYRIGHT
+
+Copyright 2017 The OpenSSL Project Authors. All Rights Reserved.
+
+Licensed under the OpenSSL license (the "License"). You may not use
+this file except in compliance with the License. You can obtain a copy
+in the file LICENSE in the source distribution or at
+L<https://www.openssl.org/source/license.html>.
+
+=cut
diff --git a/test/build.info b/test/build.info
index 7c1a055..443848b 100644
--- a/test/build.info
+++ b/test/build.info
@@ -28,7 +28,7 @@ IF[{- !$disabled{tests} -}]
dtlsv1listentest ct_test threadstest afalgtest d2i_test \
ssl_test_ctx_test ssl_test x509aux cipherlist_test asynciotest \
bioprinttest sslapitest dtlstest sslcorrupttest bio_enc_test \
- pkey_meth_test uitest cipherbytes_test
+ pkey_meth_test uitest cipherbytes_test x509_time_test
SOURCE[aborttest]=aborttest.c
INCLUDE[aborttest]=../include
@@ -295,6 +295,10 @@ IF[{- !$disabled{tests} -}]
INCLUDE[pkey_meth_test]=../include
DEPEND[pkey_meth_test]=../libcrypto
+ SOURCE[x509_time_test]=x509_time_test.c testutil.c test_main.c
+ INCLUDE[x509_time_test]=.. ../include
+ DEPEND[x509_time_test]=../libcrypto
+
IF[{- !$disabled{psk} -}]
PROGRAMS_NO_INST=dtls_mtu_test
SOURCE[dtls_mtu_test]=dtls_mtu_test.c ssltestlib.c
diff --git a/test/recipes/90-test_bioprint.t b/test/recipes/60-test_x509_time.t
similarity index 72%
copy from test/recipes/90-test_bioprint.t
copy to test/recipes/60-test_x509_time.t
index b86e828..8b311ad 100644
--- a/test/recipes/90-test_bioprint.t
+++ b/test/recipes/60-test_x509_time.t
@@ -1,5 +1,5 @@
#! /usr/bin/env perl
-# Copyright 2016 The OpenSSL Project Authors. All Rights Reserved.
+# Copyright 2017 The OpenSSL Project Authors. All Rights Reserved.
#
# Licensed under the OpenSSL license (the "License"). You may not use
# this file except in compliance with the License. You can obtain a copy
@@ -9,4 +9,4 @@
use OpenSSL::Test::Simple;
-simple_test("test_bioprint", "bioprinttest");
+simple_test("test_x509_time", "x509_time_test");
diff --git a/test/x509_time_test.c b/test/x509_time_test.c
new file mode 100644
index 0000000..5f69ebd
--- /dev/null
+++ b/test/x509_time_test.c
@@ -0,0 +1,201 @@
+/*
+ * Copyright 2017 The OpenSSL Project Authors. All Rights Reserved.
+ *
+ * Licensed under the OpenSSL license (the "License"). You may not use
+ * this file except in compliance with the License. You can obtain a copy
+ * in the file LICENSE in the source distribution or at
+ * https://www.openssl.org/source/license.html
+ */
+
+/* Tests for X509 time functions */
+
+#include <string.h>
+#include <time.h>
+
+#include <openssl/asn1.h>
+#include <openssl/x509.h>
+#include "testutil.h"
+#include "test_main.h"
+#include "e_os.h"
+
+typedef struct {
+ const char *data;
+ int type;
+ time_t cmp_time;
+ /* -1 if asn1_time <= cmp_time, 1 if asn1_time > cmp_time, 0 if error. */
+ int expected;
+} TESTDATA;
+
+static TESTDATA x509_cmp_tests[] = {
+ {
+ "20170217180154Z", V_ASN1_GENERALIZEDTIME,
+ /* The same in seconds since epoch. */
+ 1487354514, -1,
+ },
+ {
+ "20170217180154Z", V_ASN1_GENERALIZEDTIME,
+ /* One second more. */
+ 1487354515, -1,
+ },
+ {
+ "20170217180154Z", V_ASN1_GENERALIZEDTIME,
+ /* One second less. */
+ 1487354513, 1,
+ },
+ /* Same as UTC time. */
+ {
+ "170217180154Z", V_ASN1_UTCTIME,
+ /* The same in seconds since epoch. */
+ 1487354514, -1,
+ },
+ {
+ "170217180154Z", V_ASN1_UTCTIME,
+ /* One second more. */
+ 1487354515, -1,
+ },
+ {
+ "170217180154Z", V_ASN1_UTCTIME,
+ /* One second less. */
+ 1487354513, 1,
+ },
+ /* UTCTime from the 20th century. */
+ {
+ "990217180154Z", V_ASN1_UTCTIME,
+ /* The same in seconds since epoch. */
+ 919274514, -1,
+ },
+ {
+ "990217180154Z", V_ASN1_UTCTIME,
+ /* One second more. */
+ 919274515, -1,
+ },
+ {
+ "990217180154Z", V_ASN1_UTCTIME,
+ /* One second less. */
+ 919274513, 1,
+ },
+ /* Various invalid formats. */
+ {
+ /* No trailing Z. */
+ "20170217180154", V_ASN1_GENERALIZEDTIME, 0, 0,
+ },
+ {
+ /* No trailing Z, UTCTime. */
+ "170217180154", V_ASN1_UTCTIME, 0, 0,
+ },
+ {
+ /* No seconds. */
+ "201702171801Z", V_ASN1_GENERALIZEDTIME, 0, 0,
+ },
+ {
+ /* No seconds, UTCTime. */
+ "1702171801Z", V_ASN1_UTCTIME, 0, 0,
+ },
+ {
+ /* Fractional seconds. */
+ "20170217180154.001Z", V_ASN1_GENERALIZEDTIME, 0, 0,
+ },
+ {
+ /* Fractional seconds, UTCTime. */
+ "170217180154.001Z", V_ASN1_UTCTIME, 0, 0,
+ },
+ {
+ /* Timezone offset. */
+ "20170217180154+0100", V_ASN1_GENERALIZEDTIME, 0, 0,
+ },
+ {
+ /* Timezone offset, UTCTime. */
+ "170217180154+0100", V_ASN1_UTCTIME, 0, 0,
+ },
+ {
+ /* Extra digits. */
+ "2017021718015400Z", V_ASN1_GENERALIZEDTIME, 0, 0,
+ },
+ {
+ /* Extra digits, UTCTime. */
+ "17021718015400Z", V_ASN1_UTCTIME, 0, 0,
+ },
+ {
+ /* Non-digits. */
+ "2017021718015aZ", V_ASN1_GENERALIZEDTIME, 0, 0,
+ },
+ {
+ /* Non-digits, UTCTime. */
+ "17021718015aZ", V_ASN1_UTCTIME, 0, 0,
+ },
+ {
+ /* Trailing garbage. */
+ "20170217180154Zlongtrailinggarbage", V_ASN1_GENERALIZEDTIME, 0, 0,
+ },
+ {
+ /* Trailing garbage, UTCTime. */
+ "170217180154Zlongtrailinggarbage", V_ASN1_UTCTIME, 0, 0,
+ },
+ {
+ /* Swapped type. */
+ "20170217180154Z", V_ASN1_UTCTIME, 0, 0,
+ },
+ {
+ /* Swapped type. */
+ "170217180154Z", V_ASN1_GENERALIZEDTIME, 0, 0,
+ },
+ {
+ /* Bad type. */
+ "20170217180154Z", V_ASN1_OCTET_STRING, 0, 0,
+ },
+};
+
+static int test_x509_cmp_time(int idx)
+{
+ ASN1_TIME t;
+ int result;
+
+ memset(&t, 0, sizeof(t));
+ t.type = x509_cmp_tests[idx].type;
+ t.data = (unsigned char*)(x509_cmp_tests[idx].data);
+ t.length = strlen(x509_cmp_tests[idx].data);
+
+ result = X509_cmp_time(&t, &x509_cmp_tests[idx].cmp_time);
+ if (result != x509_cmp_tests[idx].expected) {
+ fprintf(stderr, "test_x509_cmp_time(%d) failed: expected %d, got %d\n",
+ idx, x509_cmp_tests[idx].expected, result);
+ return 0;
+ }
+ return 1;
+}
+
+static int test_x509_cmp_time_current()
+{
+ time_t now = time(NULL);
+ /* Pick a day earlier and later, relative to any system clock. */
+ ASN1_TIME *asn1_before = NULL, *asn1_after = NULL;
+ int cmp_result, failed = 0;
+
+ asn1_before = ASN1_TIME_adj(NULL, now, -1, 0);
+ asn1_after = ASN1_TIME_adj(NULL, now, 1, 0);
+
+ cmp_result = X509_cmp_time(asn1_before, NULL);
+ if (cmp_result != -1) {
+ fprintf(stderr, "test_x509_cmp_time_current failed: expected -1, got %d\n",
+ cmp_result);
+ failed = 1;
+ }
+
+ cmp_result = X509_cmp_time(asn1_after, NULL);
+ if (cmp_result != 1) {
+ fprintf(stderr, "test_x509_cmp_time_current failed: expected 1, got %d\n",
+ cmp_result);
+ failed = 1;
+ }
+
+ ASN1_TIME_free(asn1_before);
+ ASN1_TIME_free(asn1_after);
+
+ return failed == 0;
+}
+
+void register_tests()
+{
+ ADD_TEST(test_x509_cmp_time_current);
+ ADD_ALL_TESTS(test_x509_cmp_time, OSSL_NELEM(x509_cmp_tests));
+}
More information about the openssl-commits
mailing list