Commit 7b6cfcd6 authored by Emilia Kasper's avatar Emilia Kasper Committed by Dr. Matthias St. Pierre
Browse files

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 80770da3

Reviewed-by: default avatarRich Salz <rsalz@openssl.org>
Reviewed-by: default avatarMatthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from https://github.com/openssl/openssl/pull/6182)
parent 8dd55d9e

CHANGES

+4 −1
Original line number Diff line number Diff line
@@ -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]

crypto/x509/x509_vfy.c

+44 −96
Original line number Diff line number Diff line
@@ -56,6 +56,7 @@ 
 * [including the GNU Public Licence.]

 */



#include <ctype.h>

#include <stdio.h>

#include <time.h>

#include <errno.h>
@@ -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)

            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)

    switch (ctm->type) {

    case V_ASN1_UTCTIME:

        if (ctm->length != (int)(utctime_length))

            return 0;

        memcpy(p, str, 12);

        p += 12;

        str += 12;

        remaining -= 12;

    }



    if ((*str == 'Z') || (*str == '-') || (*str == '+')) {

        *(p++) = '0';

        *(p++) = '0';

    } else {

        /* SS (seconds) */

        if (remaining < 2)

        break;

    case V_ASN1_GENERALIZEDTIME:

        if (ctm->length != (int)(generalizedtime_length))

            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;

    default:

        return 0;

    }

        }



    }

    *(p++) = 'Z';

    *(p++) = '\0';



    /* We now need either a terminating 'Z' or an offset. */

    if (!remaining)

        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')

    /**

     * 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;

        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)

    if (ctm->data[ctm->length - 1] != 'Z')

        return 0;



    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;

    /*

     * 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 (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)

doc/man3/X509_cmp_time.pod

0 → 100644
+39 −0
Original line number Diff line number Diff line 
=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

test/Makefile

+29 −7
Original line number Diff line number Diff line
@@ -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

test/recipes/60-test_x509_time.t

0 → 100644
+12 −0
Original line number Diff line number Diff line 
#! /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");