Fix SoftKeymaster handling of EC curve specification.

Keymaster2 should accept EC curve specification either by key size (as
done in KM1) or with the new KM_TAG_EC_CURVE, filling in the other value
if not specified, and validating that they match if both are
provided. SoftKeymaster doesn't correctly implement this KM2
requirement.

Bug: 28365747
Change-Id: I27d98b71730b69bb2f0c2543af6c027b1a5670f1

8 files changed, 154 insertions, 21 deletions

diff --git a/android_keymaster_test.cpp b/android_keymaster_test.cpp
index 1ea0c34..325a4c8 100644
--- a/android_keymaster_test.cpp
+++ b/android_keymaster_test.cpp
@@ -334,17 +334,17 @@ TEST_P(NewKeyGeneration, EcdsaDefaultSize) {
 }
 
 TEST_P(NewKeyGeneration, EcdsaInvalidSize) {
-    if (GetParam()->algorithm_in_km0_hardware(KM_ALGORITHM_EC))
-        ASSERT_EQ(
-            KM_ERROR_UNKNOWN_ERROR,
-            GenerateKey(AuthorizationSetBuilder().EcdsaSigningKey(190).Digest(KM_DIGEST_NONE)));
-    else
-        ASSERT_EQ(
-            KM_ERROR_UNSUPPORTED_KEY_SIZE,
-            GenerateKey(AuthorizationSetBuilder().EcdsaSigningKey(190).Digest(KM_DIGEST_NONE)));
+    ASSERT_EQ(KM_ERROR_UNSUPPORTED_KEY_SIZE,
+              GenerateKey(AuthorizationSetBuilder().EcdsaSigningKey(190).Digest(KM_DIGEST_NONE)));
+    EXPECT_EQ(0, GetParam()->keymaster0_calls());
+}
 
-    if (GetParam()->algorithm_in_km0_hardware(KM_ALGORITHM_EC))
-        EXPECT_EQ(1, GetParam()->keymaster0_calls());
+TEST_P(NewKeyGeneration, EcdsaMismatchKeySize) {
+    ASSERT_EQ(KM_ERROR_INVALID_ARGUMENT,
+              GenerateKey(AuthorizationSetBuilder()
+                              .EcdsaSigningKey(224)
+                              .Authorization(TAG_EC_CURVE, KM_EC_CURVE_P_256)
+                              .Digest(KM_DIGEST_NONE)));
 }
 
 TEST_P(NewKeyGeneration, EcdsaAllValidSizes) {
diff --git a/android_keymaster_utils.cpp b/android_keymaster_utils.cpp
index 053e72a..5e92745 100644
--- a/android_keymaster_utils.cpp
+++ b/android_keymaster_utils.cpp
@@ -42,4 +42,54 @@ int memcmp_s(const void* p1, const void* p2, size_t length) {
     return result == 0 ? 0 : 1;
 }
 
+keymaster_error_t EcKeySizeToCurve(uint32_t key_size_bits, keymaster_ec_curve_t* curve) {
+    switch (key_size_bits) {
+    default:
+        return KM_ERROR_UNSUPPORTED_KEY_SIZE;
+
+    case 224:
+        *curve = KM_EC_CURVE_P_224;
+        break;
+
+    case 256:
+        *curve = KM_EC_CURVE_P_256;
+        break;
+
+    case 384:
+        *curve = KM_EC_CURVE_P_384;
+        break;
+
+    case 521:
+        *curve = KM_EC_CURVE_P_521;
+        break;
+    }
+
+    return KM_ERROR_OK;
+}
+
+keymaster_error_t EcCurveToKeySize(keymaster_ec_curve_t curve, uint32_t* key_size_bits) {
+    switch (curve) {
+    default:
+        return KM_ERROR_UNSUPPORTED_EC_CURVE;
+
+    case KM_EC_CURVE_P_224:
+        *key_size_bits = 224;
+        break;
+
+    case KM_EC_CURVE_P_256:
+        *key_size_bits = 256;
+        break;
+
+    case KM_EC_CURVE_P_384:
+        *key_size_bits = 384;
+        break;
+
+    case KM_EC_CURVE_P_521:
+        *key_size_bits = 521;
+        break;
+    }
+
+    return KM_ERROR_OK;
+}
+
 }  // namespace keymaster
diff --git a/authorization_set.cpp b/authorization_set.cpp
index b241b78..9f6810d 100644
--- a/authorization_set.cpp
+++ b/authorization_set.cpp
@@ -630,4 +630,11 @@ bool AuthorizationSet::ContainsEnumValue(keymaster_tag_t tag, uint32_t value) co
     return false;
 }
 
+bool AuthorizationSet::ContainsIntValue(keymaster_tag_t tag, uint32_t value) const {
+    for (auto& entry : *this)
+        if (entry.tag == tag && entry.integer == value)
+            return true;
+    return false;
+}
+
 }  // namespace keymaster
diff --git a/ec_key_factory.cpp b/ec_key_factory.cpp
index 14c8327..36ec433 100644
--- a/ec_key_factory.cpp
+++ b/ec_key_factory.cpp
@@ -40,6 +40,37 @@ OperationFactory* EcKeyFactory::GetOperationFactory(keymaster_purpose_t purpose)
     }
 }
 
+/* static */
+keymaster_error_t EcKeyFactory::GetCurveAndSize(const AuthorizationSet& key_description,
+                                                keymaster_ec_curve_t* curve,
+                                                uint32_t* key_size_bits) {
+    if (!key_description.GetTagValue(TAG_EC_CURVE, curve)) {
+        // Curve not specified. Fall back to deducing curve from key size.
+        if (!key_description.GetTagValue(TAG_KEY_SIZE, key_size_bits)) {
+            LOG_E("%s", "No curve or key size specified for EC key generation");
+            return KM_ERROR_UNSUPPORTED_KEY_SIZE;
+        }
+        keymaster_error_t error = EcKeySizeToCurve(*key_size_bits, curve);
+        if (error != KM_ERROR_OK) {
+            return KM_ERROR_UNSUPPORTED_KEY_SIZE;
+        }
+    } else {
+        keymaster_error_t error = EcCurveToKeySize(*curve, key_size_bits);
+        if (error != KM_ERROR_OK) {
+            return error;
+        }
+        uint32_t tag_key_size_bits;
+        if (key_description.GetTagValue(TAG_KEY_SIZE, &tag_key_size_bits) &&
+            *key_size_bits != tag_key_size_bits) {
+            LOG_E("Curve key size %d and specified key size %d don't match", key_size_bits,
+                  tag_key_size_bits);
+            return KM_ERROR_INVALID_ARGUMENT;
+        }
+    }
+
+    return KM_ERROR_OK;
+}
+
 keymaster_error_t EcKeyFactory::GenerateKey(const AuthorizationSet& key_description,
                                             KeymasterKeyBlob* key_blob,
                                             AuthorizationSet* hw_enforced,
@@ -49,10 +80,15 @@ keymaster_error_t EcKeyFactory::GenerateKey(const AuthorizationSet& key_descript
 
     AuthorizationSet authorizations(key_description);
 
+    keymaster_ec_curve_t ec_curve;
     uint32_t key_size;
-    if (!authorizations.GetTagValue(TAG_KEY_SIZE, &key_size)) {
-        LOG_E("%s", "No key size specified for EC key generation");
-        return KM_ERROR_UNSUPPORTED_KEY_SIZE;
+    keymaster_error_t error = GetCurveAndSize(authorizations, &ec_curve, &key_size);
+    if (error != KM_ERROR_OK) {
+        return error;
+    } else if (!authorizations.Contains(TAG_KEY_SIZE, key_size)) {
+        authorizations.push_back(TAG_KEY_SIZE, key_size);
+    } else if (!authorizations.Contains(TAG_EC_CURVE, ec_curve)) {
+        authorizations.push_back(TAG_EC_CURVE, ec_curve);
     }
 
     UniquePtr<EC_KEY, EC_KEY_Delete> ec_key(EC_KEY_new());
@@ -60,9 +96,9 @@ keymaster_error_t EcKeyFactory::GenerateKey(const AuthorizationSet& key_descript
     if (ec_key.get() == NULL || pkey.get() == NULL)
         return KM_ERROR_MEMORY_ALLOCATION_FAILED;
 
-    UniquePtr<EC_GROUP, EC_GROUP_Delete> group(choose_group(key_size));
+    UniquePtr<EC_GROUP, EC_GROUP_Delete> group(ChooseGroup(ec_curve));
     if (group.get() == NULL) {
-        LOG_E("Unable to get EC group for key of size %d", key_size);
+        LOG_E("Unable to get EC group for curve %d", ec_curve);
         return KM_ERROR_UNSUPPORTED_KEY_SIZE;
     }
 
@@ -80,7 +116,7 @@ keymaster_error_t EcKeyFactory::GenerateKey(const AuthorizationSet& key_descript
         return TranslateLastOpenSslError();
 
     KeymasterKeyBlob key_material;
-    keymaster_error_t error = EvpKeyToKeyMaterial(pkey.get(), &key_material);
+    error = EvpKeyToKeyMaterial(pkey.get(), &key_material);
     if (error != KM_ERROR_OK)
         return error;
 
@@ -149,7 +185,7 @@ keymaster_error_t EcKeyFactory::UpdateImportKeyDescription(const AuthorizationSe
 }
 
 /* static */
-EC_GROUP* EcKeyFactory::choose_group(size_t key_size_bits) {
+EC_GROUP* EcKeyFactory::ChooseGroup(size_t key_size_bits) {
     switch (key_size_bits) {
     case 224:
         return EC_GROUP_new_by_curve_name(NID_secp224r1);
@@ -169,6 +205,27 @@ EC_GROUP* EcKeyFactory::choose_group(size_t key_size_bits) {
     }
 }
 
+/* static */
+EC_GROUP* EcKeyFactory::ChooseGroup(keymaster_ec_curve_t ec_curve) {
+    switch (ec_curve) {
+    case KM_EC_CURVE_P_224:
+        return EC_GROUP_new_by_curve_name(NID_secp224r1);
+        break;
+    case KM_EC_CURVE_P_256:
+        return EC_GROUP_new_by_curve_name(NID_X9_62_prime256v1);
+        break;
+    case KM_EC_CURVE_P_384:
+        return EC_GROUP_new_by_curve_name(NID_secp384r1);
+        break;
+    case KM_EC_CURVE_P_521:
+        return EC_GROUP_new_by_curve_name(NID_secp521r1);
+        break;
+    default:
+        return nullptr;
+        break;
+    }
+}
+
 keymaster_error_t EcKeyFactory::CreateEmptyKey(const AuthorizationSet& hw_enforced,
                                                const AuthorizationSet& sw_enforced,
                                                UniquePtr<AsymmetricKey>* key) const {
diff --git a/ec_keymaster0_key.cpp b/ec_keymaster0_key.cpp
index 705ee73..e11c887 100644
--- a/ec_keymaster0_key.cpp
+++ b/ec_keymaster0_key.cpp
@@ -43,10 +43,11 @@ keymaster_error_t EcdsaKeymaster0KeyFactory::GenerateKey(const AuthorizationSet&
     if (!engine_ || !engine_->supports_ec())
         return super::GenerateKey(key_description, key_blob, hw_enforced, sw_enforced);
 
+    keymaster_ec_curve_t ec_curve;
     uint32_t key_size;
-    if (!key_description.GetTagValue(TAG_KEY_SIZE, &key_size)) {
-        LOG_E("%s", "No key size specified for EC key generation");
-        return KM_ERROR_UNSUPPORTED_KEY_SIZE;
+    keymaster_error_t error = GetCurveAndSize(key_description, &ec_curve, &key_size);
+    if (error != KM_ERROR_OK) {
+        return error;
     }
 
     KeymasterKeyBlob key_material;
@@ -57,6 +58,7 @@ keymaster_error_t EcdsaKeymaster0KeyFactory::GenerateKey(const AuthorizationSet&
     // context_->CreateKeyBlob doesn't put them in sw_enforced.
     hw_enforced->push_back(TAG_ALGORITHM, KM_ALGORITHM_EC);
     hw_enforced->push_back(TAG_KEY_SIZE, key_size);
+    hw_enforced->push_back(TAG_EC_CURVE, ec_curve);
     hw_enforced->push_back(TAG_ORIGIN, KM_ORIGIN_UNKNOWN);
 
     return context_->CreateKeyBlob(key_description, KM_ORIGIN_UNKNOWN, key_material, key_blob,
diff --git a/include/keymaster/android_keymaster_utils.h b/include/keymaster/android_keymaster_utils.h
index c190e04..17688a6 100644
--- a/include/keymaster/android_keymaster_utils.h
+++ b/include/keymaster/android_keymaster_utils.h
@@ -325,6 +325,9 @@ struct CertificateChainDelete {
     }
 };
 
+keymaster_error_t EcKeySizeToCurve(uint32_t key_size_bits, keymaster_ec_curve_t* curve);
+keymaster_error_t EcCurveToKeySize(keymaster_ec_curve_t curve, uint32_t* key_size_bits);
+
 }  // namespace keymaster
 
 #endif  // SYSTEM_KEYMASTER_ANDROID_KEYMASTER_UTILS_H_
diff --git a/include/keymaster/authorization_set.h b/include/keymaster/authorization_set.h
index 5be5d83..ec68de0 100644
--- a/include/keymaster/authorization_set.h
+++ b/include/keymaster/authorization_set.h
@@ -231,6 +231,14 @@ class AuthorizationSet : public Serializable, public keymaster_key_param_set_t {
     }
 
     /**
+     * Returns true if the set contains the specified tag and value.
+     */
+    template <keymaster_tag_t Tag>
+    bool Contains(TypedTag<KM_UINT, Tag> tag, uint32_t val) const {
+        return ContainsIntValue(tag, val);
+    }
+
+    /**
      * If the specified integer-typed \p tag exists, places its value in \p val and returns true.
      * If \p tag is not present, leaves \p val unmodified and returns false.
      */
@@ -442,6 +450,7 @@ class AuthorizationSet : public Serializable, public keymaster_key_param_set_t {
     bool GetTagValueBool(keymaster_tag_t tag) const;
 
     bool ContainsEnumValue(keymaster_tag_t tag, uint32_t val) const;
+    bool ContainsIntValue(keymaster_tag_t tag, uint32_t val) const;
 
     // Define elems_ and elems_size_ as aliases to params and length, respectively.  This is to
     // avoid using the variables without the trailing underscore in the implementation.
diff --git a/include/keymaster/ec_key_factory.h b/include/keymaster/ec_key_factory.h
index 2715e79..91c491c 100644
--- a/include/keymaster/ec_key_factory.h
+++ b/include/keymaster/ec_key_factory.h
@@ -52,7 +52,12 @@ class EcKeyFactory : public AsymmetricKeyFactory {
 
     OperationFactory* GetOperationFactory(keymaster_purpose_t purpose) const override;
 
-    static EC_GROUP* choose_group(size_t key_size_bits);
+  protected:
+    static EC_GROUP* ChooseGroup(size_t key_size_bits);
+    static EC_GROUP* ChooseGroup(keymaster_ec_curve_t ec_curve);
+
+    static keymaster_error_t GetCurveAndSize(const AuthorizationSet& key_description,
+                                             keymaster_ec_curve_t* curve, uint32_t* key_size_bits);
 };
 
 }  // namespace keymaster