Merge "Keystore 2.0: Fix shared secret negotiation for Keymaster 4.x"
diff --git a/keystore2/src/crypto/certificate_utils.cpp b/keystore2/src/crypto/certificate_utils.cpp
index d0508c8..64bf1d0 100644
--- a/keystore2/src/crypto/certificate_utils.cpp
+++ b/keystore2/src/crypto/certificate_utils.cpp
@@ -24,10 +24,13 @@
#include <functional>
#include <limits>
-#include <string>
#include <variant>
#include <vector>
+#ifndef __LP64__
+#include <time64.h>
+#endif
+
namespace keystore {
namespace {
@@ -168,45 +171,42 @@
return key_usage;
}
-template <typename Out, typename In> static Out saturate(In in) {
- if constexpr (std::is_signed_v<Out> == std::is_signed_v<In>) {
- if constexpr (sizeof(Out) >= sizeof(In)) {
- // Same sign, and In fits into Out. Cast is lossless.
- return static_cast<Out>(in);
- } else {
- // Out is smaller than In we may need to truncate.
- // We pick the smaller of `out::max()` and the greater of `out::min()` and `in`.
- return static_cast<Out>(
- std::min(static_cast<In>(std::numeric_limits<Out>::max()),
- std::max(static_cast<In>(std::numeric_limits<Out>::min()), in)));
- }
- } else {
- // So we have different signs. This puts the lower bound at 0 because either input or output
- // is unsigned. The upper bound is max of the smaller type or, if they are equal the max of
- // the signed type.
- if constexpr (std::is_signed_v<Out>) {
- if constexpr (sizeof(Out) > sizeof(In)) {
- return static_cast<Out>(in);
- } else {
- // Because `out` is the signed one, the lower bound of `in` is 0 and fits into
- // `out`. We just have to compare the maximum and we do it in type In because it has
- // a greater range than Out, so Out::max() is guaranteed to fit.
- return static_cast<Out>(
- std::min(static_cast<In>(std::numeric_limits<Out>::max()), in));
- }
- } else {
- // Out is unsigned. So we can return 0 if in is negative.
- if (in < 0) return 0;
- if constexpr (sizeof(Out) >= sizeof(In)) {
- // If Out is wider or equal we can assign lossless.
- return static_cast<Out>(in);
- } else {
- // Otherwise we have to take the minimum of Out::max() and `in`.
- return static_cast<Out>(
- std::min(static_cast<In>(std::numeric_limits<Out>::max()), in));
- }
- }
+// TODO Once boring ssl can take int64_t instead of time_t we can go back to using
+// ASN1_TIME_set: https://bugs.chromium.org/p/boringssl/issues/detail?id=416
+std::optional<std::array<char, 16>> toTimeString(int64_t timeMillis) {
+ struct tm time;
+ // If timeMillis is negative the rounding direction should still be to the nearest previous
+ // second.
+ if (timeMillis < 0 && __builtin_add_overflow(timeMillis, -999, &timeMillis)) {
+ return std::nullopt;
}
+#if defined(__LP64__)
+ time_t timeSeconds = timeMillis / 1000;
+ if (gmtime_r(&timeSeconds, &time) == nullptr) {
+ return std::nullopt;
+ }
+#else
+ time64_t timeSeconds = timeMillis / 1000;
+ if (gmtime64_r(&timeSeconds, &time) == nullptr) {
+ return std::nullopt;
+ }
+#endif
+ std::array<char, 16> buffer;
+ if (__builtin_add_overflow(time.tm_year, 1900, &time.tm_year)) {
+ return std::nullopt;
+ }
+ if (time.tm_year >= 1950 && time.tm_year < 2050) {
+ // UTCTime according to RFC5280 4.1.2.5.1.
+ snprintf(buffer.data(), buffer.size(), "%02d%02d%02d%02d%02d%02dZ", time.tm_year % 100,
+ time.tm_mon + 1, time.tm_mday, time.tm_hour, time.tm_min, time.tm_sec);
+ } else if (time.tm_year >= 0 && time.tm_year < 10000) {
+ // GeneralizedTime according to RFC5280 4.1.2.5.2.
+ snprintf(buffer.data(), buffer.size(), "%04d%02d%02d%02d%02d%02dZ", time.tm_year,
+ time.tm_mon + 1, time.tm_mday, time.tm_hour, time.tm_min, time.tm_sec);
+ } else {
+ return std::nullopt;
+ }
+ return buffer;
}
// Creates a rump certificate structure with serial, subject and issuer names, as well as
@@ -260,19 +260,24 @@
return std::get<CertUtilsError>(subjectName);
}
- time_t notBeforeTime = saturate<time_t>(activeDateTimeMilliSeconds / 1000);
+ auto notBeforeTime = toTimeString(activeDateTimeMilliSeconds);
+ if (!notBeforeTime) {
+ return CertUtilsError::TimeError;
+ }
// Set activation date.
ASN1_TIME_Ptr notBefore(ASN1_TIME_new());
- if (!notBefore || !ASN1_TIME_set(notBefore.get(), notBeforeTime) ||
+ if (!notBefore || !ASN1_TIME_set_string(notBefore.get(), notBeforeTime->data()) ||
!X509_set_notBefore(certificate.get(), notBefore.get() /* Don't release; copied */))
return CertUtilsError::BoringSsl;
// Set expiration date.
- time_t notAfterTime;
- notAfterTime = saturate<time_t>(usageExpireDateTimeMilliSeconds / 1000);
+ auto notAfterTime = toTimeString(usageExpireDateTimeMilliSeconds);
+ if (!notAfterTime) {
+ return CertUtilsError::TimeError;
+ }
ASN1_TIME_Ptr notAfter(ASN1_TIME_new());
- if (!notAfter || !ASN1_TIME_set(notAfter.get(), notAfterTime) ||
+ if (!notAfter || !ASN1_TIME_set_string(notAfter.get(), notAfterTime->data()) ||
!X509_set_notAfter(certificate.get(), notAfter.get() /* Don't release; copied */)) {
return CertUtilsError::BoringSsl;
}
diff --git a/keystore2/src/crypto/include/certificate_utils.h b/keystore2/src/crypto/include/certificate_utils.h
index b43a830..cad82b6 100644
--- a/keystore2/src/crypto/include/certificate_utils.h
+++ b/keystore2/src/crypto/include/certificate_utils.h
@@ -54,6 +54,7 @@
InvalidArgument,
UnexpectedNullPointer,
SignatureFailed,
+ TimeError,
};
private:
@@ -138,6 +139,16 @@
};
/**
+ * Takes an int64_t representing UNIX epoch time in milliseconds and turns it into a UTCTime
+ * or GeneralizedTime string depending on whether the year is in the interval [1950 .. 2050).
+ * Note: The string returned in the array buffer is NUL terminated and of length 13 (UTCTime)
+ * or 15 (GeneralizedTime).
+ * @param timeMillis
+ * @return UTCTime or GeneralizedTime string.
+ */
+std::optional<std::array<char, 16>> toTimeString(int64_t timeMillis);
+
+/**
* Sets the signature specifier of the certificate and the signature according to the parameters
* c. Then it signs the certificate with the `sign` callback.
* IMPORTANT: The parameters `algo`, `padding`, and `digest` do not control the actual signing
diff --git a/keystore2/src/crypto/tests/certificate_utils_test.cpp b/keystore2/src/crypto/tests/certificate_utils_test.cpp
index 119c3fa..bd94928 100644
--- a/keystore2/src/crypto/tests/certificate_utils_test.cpp
+++ b/keystore2/src/crypto/tests/certificate_utils_test.cpp
@@ -315,3 +315,23 @@
EVP_PKEY_Ptr decoded_pkey(X509_get_pubkey(decoded_cert.get()));
ASSERT_TRUE(X509_verify(decoded_cert.get(), decoded_pkey.get()));
}
+
+TEST(TimeStringTests, toTimeStringTest) {
+ // Two test vectors that need to result in UTCTime
+ ASSERT_EQ(std::string(toTimeString(1622758591000)->data()), std::string("210603221631Z"));
+ ASSERT_EQ(std::string(toTimeString(0)->data()), std::string("700101000000Z"));
+ // Two test vectors that need to result in GeneralizedTime.
+ ASSERT_EQ(std::string(toTimeString(16227585910000)->data()), std::string("24840325064510Z"));
+ ASSERT_EQ(std::string(toTimeString(-1622758591000)->data()), std::string("19180731014329Z"));
+
+ // Highest possible UTCTime
+ ASSERT_EQ(std::string(toTimeString(2524607999999)->data()), "491231235959Z");
+ // And one millisecond later must be GeneralizedTime.
+ ASSERT_EQ(std::string(toTimeString(2524608000000)->data()), "20500101000000Z");
+
+ // Earliest possible UTCTime
+ ASSERT_EQ(std::string(toTimeString(-631152000000)->data()), "500101000000Z");
+ // And one millisecond earlier must be GeneralizedTime.
+ // This also checks that the rounding direction does not flip when the input is negative.
+ ASSERT_EQ(std::string(toTimeString(-631152000001)->data()), "19491231235959Z");
+}
diff --git a/ondevice-signing/CertUtils.cpp b/ondevice-signing/CertUtils.cpp
index 14dd6d0..10abfe2 100644
--- a/ondevice-signing/CertUtils.cpp
+++ b/ondevice-signing/CertUtils.cpp
@@ -196,9 +196,9 @@
Result<std::vector<uint8_t>>
extractPublicKeyFromSubjectPublicKeyInfo(const std::vector<uint8_t>& keyData) {
auto keyDataBytes = keyData.data();
- EVP_PKEY* public_key = d2i_PUBKEY(nullptr, &keyDataBytes, keyData.size());
+ bssl::UniquePtr<EVP_PKEY> public_key(d2i_PUBKEY(nullptr, &keyDataBytes, keyData.size()));
- return extractPublicKey(public_key);
+ return extractPublicKey(public_key.get());
}
Result<std::vector<uint8_t>> extractPublicKeyFromX509(const std::vector<uint8_t>& keyData) {
@@ -213,18 +213,19 @@
}
Result<std::vector<uint8_t>> extractPublicKeyFromX509(const std::string& path) {
- X509* cert;
+ X509* rawCert;
auto f = fopen(path.c_str(), "re");
if (f == nullptr) {
return Error() << "Failed to open " << path;
}
- if (!d2i_X509_fp(f, &cert)) {
+ if (!d2i_X509_fp(f, &rawCert)) {
fclose(f);
return Error() << "Unable to decode x509 cert at " << path;
}
+ bssl::UniquePtr<X509> cert(rawCert);
fclose(f);
- return extractPublicKey(X509_get_pubkey(cert));
+ return extractPublicKey(X509_get_pubkey(cert.get()));
}
Result<std::vector<uint8_t>> createPkcs7(const std::vector<uint8_t>& signed_digest) {
diff --git a/ondevice-signing/VerityUtils.cpp b/ondevice-signing/VerityUtils.cpp
index cab92e2..2c4dc6d 100644
--- a/ondevice-signing/VerityUtils.cpp
+++ b/ondevice-signing/VerityUtils.cpp
@@ -255,7 +255,7 @@
char* argv_child[argc + 1];
memcpy(argv_child, argv, argc * sizeof(char*));
argv_child[argc] = nullptr;
- execvp(argv_child[0], const_cast<char**>(argv_child));
+ execvp(argv_child[0], argv_child);
PLOG(ERROR) << "exec in ForkExecvp";
_exit(EXIT_FAILURE);
} else {