From 7b6cfcd6dd99a86ecc3a1c51eef539494e191754 Mon Sep 17 00:00:00 2001 From: Emilia Kasper Date: Fri, 17 Feb 2017 19:00:15 +0100 Subject: [PATCH] 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 Backported from 80770da39e Reviewed-by: Rich Salz Reviewed-by: Matthias St. Pierre (Merged from https://github.com/openssl/openssl/pull/6182) --- CHANGES | 5 +- crypto/x509/x509_vfy.c | 140 +++++++------------- doc/man3/X509_cmp_time.pod | 39 ++++++ test/Makefile | 36 +++++- test/recipes/60-test_x509_time.t | 12 ++ test/x509_time_test.c | 212 +++++++++++++++++++++++++++++++ 6 files changed, 340 insertions(+), 104 deletions(-) create mode 100644 doc/man3/X509_cmp_time.pod create mode 100644 test/recipes/60-test_x509_time.t create mode 100644 test/x509_time_test.c diff --git a/CHANGES b/CHANGES index 1da1a42f505ed..25b453e60406f 100644 --- a/CHANGES +++ b/CHANGES @@ -9,7 +9,10 @@ Changes between 1.0.2o and 1.0.2p [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] Changes between 1.0.2n and 1.0.2o [27 Mar 2018] diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index ff238331e6821..869460d7cdba7 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -56,6 +56,7 @@ * [including the GNU Public Licence.] */ +#include #include #include #include @@ -1937,120 +1938,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); /* * X509_cmp_time comparison is <=. * The return value 0 is reserved for errors. */ - return i > 0 ? 1 : -1; + 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 0000000000000..31826ad84d6c4 --- /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 with the time in +. + +B must satisfy the ASN1_TIME format mandated by RFC 5280, i.e., +its format must be either YYMMDDHHMMSSZ or YYYYMMDDHHMMSSZ. + +If B 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 is earlier than, or equal to, +B, 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. + +=cut diff --git a/test/Makefile b/test/Makefile index a1f7eeb0ddeb3..db407f711caa7 100644 --- a/test/Makefile +++ b/test/Makefile @@ -74,7 +74,7 @@ BADDTLSTEST= bad_dtls_test SSLV2CONFTEST = sslv2conftest DTLSTEST = dtlstest FATALERRTEST = fatalerrtest - +X509TIMETEST = x509_time_test TESTS= alltests EXE= $(BNTEST)$(EXE_EXT) $(ECTEST)$(EXE_EXT) $(ECDSATEST)$(EXE_EXT) $(ECDHTEST)$(EXE_EXT) $(IDEATEST)$(EXE_EXT) \ @@ -88,7 +88,7 @@ EXE= $(BNTEST)$(EXE_EXT) $(ECTEST)$(EXE_EXT) $(ECDSATEST)$(EXE_EXT) $(ECDHTEST) $(ASN1TEST)$(EXE_EXT) $(V3NAMETEST)$(EXE_EXT) $(HEARTBEATTEST)$(EXE_EXT) \ $(CONSTTIMETEST)$(EXE_EXT) $(VERIFYEXTRATEST)$(EXE_EXT) \ $(CLIENTHELLOTEST)$(EXE_EXT) $(SSLV2CONFTEST)$(EXE_EXT) $(DTLSTEST)$(EXE_EXT) \ - $(BADDTLSTEST)$(EXE_EXT) $(FATALERRTEST)$(EXE_EXT) + $(BADDTLSTEST)$(EXE_EXT) $(FATALERRTEST)$(EXE_EXT) $(X509TIMETEST)$(EXE_EXT) # $(METHTEST)$(EXE_EXT) @@ -103,7 +103,7 @@ OBJ= $(BNTEST).o $(ECTEST).o $(ECDSATEST).o $(ECDHTEST).o $(IDEATEST).o \ $(EVPTEST).o $(EVPEXTRATEST).o $(IGETEST).o $(JPAKETEST).o $(ASN1TEST).o $(V3NAMETEST).o \ $(HEARTBEATTEST).o $(CONSTTIMETEST).o $(VERIFYEXTRATEST).o \ $(CLIENTHELLOTEST).o $(SSLV2CONFTEST).o $(DTLSTEST).o ssltestlib.o \ - $(BADDTLSTEST).o $(FATALERRTEST).o + $(BADDTLSTEST).o $(FATALERRTEST).o $(X509TIMETEST).o SRC= $(BNTEST).c $(ECTEST).c $(ECDSATEST).c $(ECDHTEST).c $(IDEATEST).c \ $(MD2TEST).c $(MD4TEST).c $(MD5TEST).c \ @@ -115,7 +115,7 @@ SRC= $(BNTEST).c $(ECTEST).c $(ECDSATEST).c $(ECDHTEST).c $(IDEATEST).c \ $(EVPTEST).c $(EVPEXTRATEST).c $(IGETEST).c $(JPAKETEST).c $(SRPTEST).c $(ASN1TEST).c \ $(V3NAMETEST).c $(HEARTBEATTEST).c $(CONSTTIMETEST).c $(VERIFYEXTRATEST).c \ $(CLIENTHELLOTEST).c $(SSLV2CONFTEST).c $(DTLSTEST).c ssltestlib.c \ - $(BADDTLSTEST).c $(FATALERRTEST).c + $(BADDTLSTEST).c $(FATALERRTEST).c $(X509TIMETEST).c EXHEADER= HEADER= testutil.h ssltestlib.h $(EXHEADER) @@ -160,7 +160,7 @@ alltests: \ test_ss test_ca test_engine test_evp test_evp_extra test_ssl test_tsa test_ige \ test_jpake test_srp test_cms test_ocsp test_v3name test_heartbeat \ test_constant_time test_verify_extra test_clienthello test_sslv2conftest \ - test_dtls test_bad_dtls test_fatalerr + test_dtls test_bad_dtls test_fatalerr test_x509_time test_evp: $(EVPTEST)$(EXE_EXT) evptests.txt ../util/shlib_wrap.sh ./$(EVPTEST) evptests.txt @@ -378,6 +378,10 @@ test_fatalerr: $(FATALERRTEST)$(EXE_EXT) @echo $(START) $@ ../util/shlib_wrap.sh ./$(FATALERRTEST) ../apps/server.pem ../apps/server.pem +test_x509_time: $(X509TIMETEST)$(EXE_EXT) + @echo $(START) $@ + ../util/shlib_wrap.sh ./$(X509TIMETEST) + test_sslv2conftest: $(SSLV2CONFTEST)$(EXE_EXT) @echo $(START) $@ ../util/shlib_wrap.sh ./$(SSLV2CONFTEST) @@ -569,6 +573,9 @@ $(BADDTLSTEST)$(EXE_EXT): $(BADDTLSTEST).o $(FATALERRTEST)$(EXE_EXT): $(FATALERRTEST).o ssltestlib.o $(DLIBSSL) $(DLIBCRYPTO) @target=$(FATALERRTEST); exobj=ssltestlib.o; $(BUILD_CMD) +$(X509TIMETEST)$(EXE_EXT): $(X509TIMETEST).o + @target=$(X509TIMETEST) $(BUILD_CMD) + $(SSLV2CONFTEST)$(EXE_EXT): $(SSLV2CONFTEST).o @target=$(SSLV2CONFTEST) $(BUILD_CMD) @@ -832,8 +839,11 @@ hmactest.o: ../include/openssl/objects.h ../include/openssl/opensslconf.h hmactest.o: ../include/openssl/opensslv.h ../include/openssl/ossl_typ.h hmactest.o: ../include/openssl/safestack.h ../include/openssl/stack.h hmactest.o: ../include/openssl/symhacks.h hmactest.c -ideatest.o: ../e_os.h ../include/openssl/e_os2.h ../include/openssl/idea.h -ideatest.o: ../include/openssl/opensslconf.h ideatest.c +ideatest.o: ../include/openssl/buffer.h ../include/openssl/crypto.h +ideatest.o: ../include/openssl/e_os2.h ../include/openssl/opensslconf.h +ideatest.o: ../include/openssl/opensslv.h ../include/openssl/ossl_typ.h +ideatest.o: ../include/openssl/safestack.h ../include/openssl/stack.h +ideatest.o: ../include/openssl/symhacks.h ideatest.c igetest.o: ../include/openssl/aes.h ../include/openssl/e_os2.h igetest.o: ../include/openssl/opensslconf.h ../include/openssl/ossl_typ.h igetest.o: ../include/openssl/rand.h igetest.c @@ -1012,3 +1022,15 @@ wp_test.o: ../include/openssl/opensslconf.h ../include/openssl/opensslv.h wp_test.o: ../include/openssl/ossl_typ.h ../include/openssl/safestack.h wp_test.o: ../include/openssl/stack.h ../include/openssl/symhacks.h wp_test.o: ../include/openssl/whrlpool.h wp_test.c +x509_time_test.o: ../e_os.h ../include/openssl/asn1.h ../include/openssl/bio.h +x509_time_test.o: ../include/openssl/buffer.h ../include/openssl/crypto.h +x509_time_test.o: ../include/openssl/e_os2.h ../include/openssl/ec.h +x509_time_test.o: ../include/openssl/ecdh.h ../include/openssl/ecdsa.h +x509_time_test.o: ../include/openssl/evp.h ../include/openssl/lhash.h +x509_time_test.o: ../include/openssl/obj_mac.h ../include/openssl/objects.h +x509_time_test.o: ../include/openssl/opensslconf.h +x509_time_test.o: ../include/openssl/opensslv.h ../include/openssl/ossl_typ.h +x509_time_test.o: ../include/openssl/pkcs7.h ../include/openssl/safestack.h +x509_time_test.o: ../include/openssl/sha.h ../include/openssl/stack.h +x509_time_test.o: ../include/openssl/symhacks.h ../include/openssl/x509.h +x509_time_test.o: ../include/openssl/x509_vfy.h testutil.h x509_time_test.c diff --git a/test/recipes/60-test_x509_time.t b/test/recipes/60-test_x509_time.t new file mode 100644 index 0000000000000..8b311ad31405a --- /dev/null +++ b/test/recipes/60-test_x509_time.t @@ -0,0 +1,12 @@ +#! /usr/bin/env perl +# 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 + + +use OpenSSL::Test::Simple; + +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 0000000000000..ef277fe8f77df --- /dev/null +++ b/test/x509_time_test.c @@ -0,0 +1,212 @@ +/* + * 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 +#include + +#include +#include +#include "testutil.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; +} + +int main(int argc, char **argv) +{ + int ret = 0; + unsigned int idx; + + if (!test_x509_cmp_time_current()) + ret = 1; + + for (idx=0 ; idx < sizeof(x509_cmp_tests)/sizeof(x509_cmp_tests[0]) ; ++idx) { + if (!test_x509_cmp_time(idx)) + ret = 1; + } + + if (ret == 0) + printf("PASS\n"); + return ret; +}