Fix magic ID selection in NCryptExportKey.

Add algorithm-based magic ID selection, instead of hardcoding the EC-P256 ID. Interesting that signtool did not throw any errors or warnings. Bug: b/289095059 Change-Id: Ic430f1fd94892ed007847541a7f08787ae0691ea
GoogleCloudPlatform · Oct 4, 2023 · 2ef7fa4 · 2ef7fa4
1 parent 23a1331
commit 2ef7fa4
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 1 deletion.
diff --git a/kmscng/operation/sign_utils.cc b/kmscng/operation/sign_utils.cc
@@ -102,6 +102,20 @@ absl::StatusOr<int> CurveIdForAlgorithm(
   }
 }
 
+absl::StatusOr<uint32_t> MagicIdForAlgorithm(
+    kms_v1::CryptoKeyVersion::CryptoKeyVersionAlgorithm algorithm) {
+  switch (algorithm) {
+    case kms_v1::CryptoKeyVersion::EC_SIGN_P256_SHA256:
+      return BCRYPT_ECDSA_PUBLIC_P256_MAGIC;
+    case kms_v1::CryptoKeyVersion::EC_SIGN_P384_SHA384:
+      return BCRYPT_ECDSA_PUBLIC_P384_MAGIC;
+    default:
+      return NewInternalError(
+          absl::StrFormat("cannot get magic ID for algorithm: %d", algorithm),
+          SOURCE_LOCATION);
+  }
+}
+
 absl::Status ValidateKeyPreconditions(Object* object) {
   RETURN_IF_ERROR(IsValidSigningAlgorithm(object->algorithm()));
   ASSIGN_OR_RETURN(AlgorithmDetails details, GetDetails(object->algorithm()));
@@ -161,7 +175,7 @@ absl::StatusOr<std::vector<uint8_t>> SerializePublicKey(Object* object) {
   }
   BCRYPT_ECCKEY_BLOB* header =
       reinterpret_cast<BCRYPT_ECCKEY_BLOB*>(result.data());
-  header->dwMagic = BCRYPT_ECDSA_PUBLIC_P256_MAGIC;
+  ASSIGN_OR_RETURN(header->dwMagic, MagicIdForAlgorithm(object->algorithm()));
   header->cbKey = uncompressed_length / 2;
   return result;
 }

diff --git a/kmscng/operation/sign_utils.h b/kmscng/operation/sign_utils.h
@@ -38,6 +38,10 @@ absl::StatusOr<const EVP_MD*> DigestForAlgorithm(
 absl::StatusOr<int> CurveIdForAlgorithm(
     kms_v1::CryptoKeyVersion::CryptoKeyVersionAlgorithm algorithm);
 
+// Returns the right magic ID for the provided KMS algorithm.
+absl::StatusOr<uint32_t> MagicIdForAlgorithm(
+    kms_v1::CryptoKeyVersion::CryptoKeyVersionAlgorithm algorithm);
+
 // Checks the object properties against the expected properties defined in the
 // relevant AlgorithmDetails struct.
 absl::Status ValidateKeyPreconditions(Object* object);

diff --git a/kmscng/operation/sign_utils_test.cc b/kmscng/operation/sign_utils_test.cc
@@ -62,6 +62,16 @@ TEST(CurveIdForAlgorithmTest, InvalidAlgoritmhm) {
       StatusIs(absl::StatusCode::kInternal, HasSubstr("cannot get curve")));
 }
 
+TEST(MagicIdForAlgorithmTest, Success) {
+  EXPECT_OK(MagicIdForAlgorithm(kms_v1::CryptoKeyVersion::EC_SIGN_P384_SHA384));
+}
+
+TEST(MagicIdForAlgorithmTest, InvalidAlgoritmhm) {
+  EXPECT_THAT(
+      MagicIdForAlgorithm(kms_v1::CryptoKeyVersion::RSA_DECRYPT_OAEP_2048_SHA1),
+      StatusIs(absl::StatusCode::kInternal, HasSubstr("cannot get magic")));
+}
+
 class SignUtilsTest : public testing::Test {
  protected:
   void SetUp() override {