Add `X509_V_FLAG_ALLOW_TIMEZONE_OFFSET`. This relaxes X.509 comparisons to allow for a time zone offset in `notBefore` and `notAfter` fields. Bug: 537625888 Change-Id: I1f51b59816ed4ceaff6ac9e9411d8cf86a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/101247 Auto-Submit: Rudolf Polzer <rpolzer@google.com> Commit-Queue: Rudolf Polzer <rpolzer@google.com> Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 694b6dd..c126f14 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -27,6 +27,7 @@ #include <utility> #include <vector> +#include <gmock/gmock.h> #include <gtest/gtest.h> #include <openssl/asn1.h> @@ -5255,6 +5256,36 @@ X509_VERIFY_PARAM_clear_flags(param, X509_V_FLAG_USE_CHECK_TIME); })); + + // Time zone offsets in notBefore and notAfter fields are rejected by default, + // but allowed with X509_V_FLAG_ALLOW_TIMEZONE_OFFSET. + UniquePtr<X509> leaf_not_before_tz = + MakeTestCert("Intermediate", "Leaf", key.get(), /*is_ca=*/false); + ASSERT_TRUE(leaf_not_before_tz); + ASSERT_TRUE(ASN1_STRING_set(X509_getm_notBefore(leaf_not_before_tz.get()), + "160926010000+0100", 17)); + ASSERT_TRUE(X509_sign(leaf_not_before_tz.get(), key.get(), EVP_sha256())); + + EXPECT_EQ(X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD, + Verify(leaf_not_before_tz.get(), {root.valid.get()}, + {intermediate.valid.get()}, {})); + EXPECT_EQ(X509_V_OK, Verify(leaf_not_before_tz.get(), {root.valid.get()}, + {intermediate.valid.get()}, {}, + X509_V_FLAG_ALLOW_TIMEZONE_OFFSET)); + + UniquePtr<X509> leaf_not_after_tz = + MakeTestCert("Intermediate", "Leaf", key.get(), /*is_ca=*/false); + ASSERT_TRUE(leaf_not_after_tz); + ASSERT_TRUE(ASN1_STRING_set(X509_getm_notAfter(leaf_not_after_tz.get()), + "160928010000+0100", 17)); + ASSERT_TRUE(X509_sign(leaf_not_after_tz.get(), key.get(), EVP_sha256())); + + EXPECT_EQ(X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD, + Verify(leaf_not_after_tz.get(), {root.valid.get()}, + {intermediate.valid.get()}, {})); + EXPECT_EQ(X509_V_OK, Verify(leaf_not_after_tz.get(), {root.valid.get()}, + {intermediate.valid.get()}, {}, + X509_V_FLAG_ALLOW_TIMEZONE_OFFSET)); } TEST(X509Test, SignatureVerification) {
diff --git a/crypto/x509/x509_time_test.cc b/crypto/x509/x509_time_test.cc index ada534e..e741910 100644 --- a/crypto/x509/x509_time_test.cc +++ b/crypto/x509/x509_time_test.cc
@@ -150,13 +150,6 @@ 0, }, { - // Timezone offset, UTCTime. - "170217180154+0100", - V_ASN1_UTCTIME, - 0, - 0, - }, - { // Extra digits. "2017021718015400Z", V_ASN1_GENERALIZEDTIME, @@ -221,13 +214,15 @@ }, // Test limits and unusual cases. { - "99991231235959Z", V_ASN1_GENERALIZEDTIME, + "99991231235959Z", + V_ASN1_GENERALIZEDTIME, // Test a very large positive time with the largest representable time 253402300799, -1, // TODO(bbe): This is *technically* wrong by rfc5280. }, { - "99991231235959Z", V_ASN1_GENERALIZEDTIME, + "99991231235959Z", + V_ASN1_GENERALIZEDTIME, // one second after the largest possible time should still compare // correctly 253402300800, @@ -297,7 +292,6 @@ -62167219199, -1, }, - }; TEST(X509TimeTest, TestCmpTime) { @@ -309,6 +303,50 @@ ASSERT_TRUE(ASN1_STRING_set(t.get(), test.data, strlen(test.data))); EXPECT_EQ(test.expected, X509_cmp_time_posix(t.get(), test.cmp_time)); + EXPECT_EQ(test.expected, + X509_cmp_time_posix_nonstandard(t.get(), test.cmp_time)); + } +} + +struct NonStandardTestData { + const char *data; + int type; + int64_t cmp_time; + // -1 if asn1_time <= cmp_time, 1 if asn1_time > cmp_time, 0 if error. + int expected; + int expected_nonstandard; +}; + +static NonStandardTestData kX509CmpNonStandardTests[] = { + { + // Timezone offset, UTCTime. + "170217180154+0100", + V_ASN1_UTCTIME, + 1487350913, + 0, + 1, + }, + { + // Timezone offset, UTCTime. + "170217180154+0100", + V_ASN1_UTCTIME, + 1487350915, + 0, + -1, + }, +}; + +TEST(X509TimeTest, TestCmpTimeNonStandard) { + for (auto &test : kX509CmpNonStandardTests) { + SCOPED_TRACE(test.data); + + bssl::UniquePtr<ASN1_STRING> t(ASN1_STRING_type_new(test.type)); + ASSERT_TRUE(t); + ASSERT_TRUE(ASN1_STRING_set(t.get(), test.data, strlen(test.data))); + + EXPECT_EQ(test.expected, X509_cmp_time_posix(t.get(), test.cmp_time)); + EXPECT_EQ(test.expected_nonstandard, + X509_cmp_time_posix_nonstandard(t.get(), test.cmp_time)); } }
diff --git a/crypto/x509/x509_vfy.cc b/crypto/x509/x509_vfy.cc index fedc4a0..2b071bc 100644 --- a/crypto/x509/x509_vfy.cc +++ b/crypto/x509/x509_vfy.cc
@@ -1202,7 +1202,9 @@ ptime = time(nullptr); } - int i = X509_cmp_time_posix(X509_get_notBefore(x), ptime); + int i = (ctx->param->flags & X509_V_FLAG_ALLOW_TIMEZONE_OFFSET) + ? X509_cmp_time_posix_nonstandard(X509_get_notBefore(x), ptime) + : X509_cmp_time_posix(X509_get_notBefore(x), ptime); if (i == 0) { ctx->error = X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD; ctx->current_cert = x; @@ -1219,7 +1221,9 @@ } } - i = X509_cmp_time_posix(X509_get_notAfter(x), ptime); + i = (ctx->param->flags & X509_V_FLAG_ALLOW_TIMEZONE_OFFSET) + ? X509_cmp_time_posix_nonstandard(X509_get_notAfter(x), ptime) + : X509_cmp_time_posix(X509_get_notAfter(x), ptime); if (i == 0) { ctx->error = X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD; ctx->current_cert = x; @@ -1333,6 +1337,15 @@ return (ctm_time - cmp_time <= 0) ? -1 : 1; } +int X509_cmp_time_posix_nonstandard(const ASN1_TIME *ctm, int64_t cmp_time) { + int64_t ctm_time; + if (!ASN1_TIME_to_posix_nonstandard(ctm, &ctm_time)) { + return 0; + } + // The return value 0 is reserved for errors. + return (ctm_time - cmp_time <= 0) ? -1 : 1; +} + ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long offset_sec) { return X509_time_adj(s, offset_sec, nullptr); }
diff --git a/include/openssl/prefix_symbols.h b/include/openssl/prefix_symbols.h index e60131a..54cd557 100644 --- a/include/openssl/prefix_symbols.h +++ b/include/openssl/prefix_symbols.h
@@ -2813,6 +2813,7 @@ #pragma redefine_extname X509_cmp_current_time BORINGSSL_ADD_USER_LABEL_AND_PREFIX(X509_cmp_current_time) #pragma redefine_extname X509_cmp_time BORINGSSL_ADD_USER_LABEL_AND_PREFIX(X509_cmp_time) #pragma redefine_extname X509_cmp_time_posix BORINGSSL_ADD_USER_LABEL_AND_PREFIX(X509_cmp_time_posix) +#pragma redefine_extname X509_cmp_time_posix_nonstandard BORINGSSL_ADD_USER_LABEL_AND_PREFIX(X509_cmp_time_posix_nonstandard) #pragma redefine_extname X509_delete_ext BORINGSSL_ADD_USER_LABEL_AND_PREFIX(X509_delete_ext) #pragma redefine_extname X509_digest BORINGSSL_ADD_USER_LABEL_AND_PREFIX(X509_digest) #pragma redefine_extname X509_dup BORINGSSL_ADD_USER_LABEL_AND_PREFIX(X509_dup) @@ -5951,6 +5952,7 @@ #define X509_cmp_current_time BORINGSSL_ADD_PREFIX(X509_cmp_current_time) #define X509_cmp_time BORINGSSL_ADD_PREFIX(X509_cmp_time) #define X509_cmp_time_posix BORINGSSL_ADD_PREFIX(X509_cmp_time_posix) +#define X509_cmp_time_posix_nonstandard BORINGSSL_ADD_PREFIX(X509_cmp_time_posix_nonstandard) #define X509_delete_ext BORINGSSL_ADD_PREFIX(X509_delete_ext) #define X509_digest BORINGSSL_ADD_PREFIX(X509_digest) #define X509_dup BORINGSSL_ADD_PREFIX(X509_dup)
diff --git a/include/openssl/x509.h b/include/openssl/x509.h index f1b21e2..75ada6c 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h
@@ -3286,6 +3286,9 @@ // X509_V_FLAG_NO_CHECK_TIME disables all time checks in certificate // verification. #define X509_V_FLAG_NO_CHECK_TIME 0x200000 +// X509_V_FLAG_ALLOW_TIMEZONE_OFFSET allows `notBefore` and `notAfter` fields +// to contain a time zone offset. +#define X509_V_FLAG_ALLOW_TIMEZONE_OFFSET 0x1000000 // X509_VERIFY_PARAM_set_flags enables all values in `flags` in `param`'s // verification flags and returns one. `flags` should be a combination of @@ -4349,10 +4352,24 @@ // negative number if `s` <= `t` and a positive number if `s` > `t`. On error, // it returns zero. // +// If `s` has a time zone offset, it returns an error (0). +// // WARNING: Unlike most comparison functions, this function returns zero on // error, not equality. OPENSSL_EXPORT int X509_cmp_time_posix(const ASN1_TIME *s, int64_t t); +// X509_cmp_time_posix_nonstandard compares `s` against `t`. On success, it +// returns a negative number if `s` <= `t` and a positive number if `s` > `t`. +// On error, it returns zero. +// +// If `s` has a time zone offset, it applies it before comparing to `t`. See +// `ASN1_TIME_to_posix_nonstandard` for more details. +// +// WARNING: Unlike most comparison functions, this function returns zero on +// error, not equality. +OPENSSL_EXPORT int X509_cmp_time_posix_nonstandard(const ASN1_TIME *s, + int64_t t); + // X509_cmp_current_time behaves like `X509_cmp_time` but compares `s` against // the current time. OPENSSL_EXPORT int X509_cmp_current_time(const ASN1_TIME *s);