Fix enforcement of block mode and MAC length on AES ops

Bug: 22301168
Change-Id: I54b4efffa1786b08704dd6e785360870f155ed80

2 files changed, 27 insertions, 7 deletions

diff --git a/aes_operation.cpp b/aes_operation.cpp
index face408..bf18041 100644
--- a/aes_operation.cpp
+++ b/aes_operation.cpp
@@ -70,23 +70,30 @@ Operation* AesOperationFactory::CreateOperation(const Key& key,
     if (!begin_params.GetTagValue(TAG_BLOCK_MODE, &block_mode)) {
         LOG_E("%d block modes specified in begin params", begin_params.GetTagCount(TAG_BLOCK_MODE));
         *error = KM_ERROR_UNSUPPORTED_BLOCK_MODE;
+        return nullptr;
     } else if (!supported(block_mode)) {
         LOG_E("Block mode %d not supported", block_mode);
         *error = KM_ERROR_UNSUPPORTED_BLOCK_MODE;
+        return nullptr;
     } else if (!key.authorizations().Contains(TAG_BLOCK_MODE, block_mode)) {
         LOG_E("Block mode %d was specified, but not authorized by key", block_mode);
         *error = KM_ERROR_INCOMPATIBLE_BLOCK_MODE;
+        return nullptr;
     }
 
     size_t tag_length = 0;
     if (block_mode == KM_MODE_GCM) {
         uint32_t tag_length_bits;
-        if (!begin_params.GetTagValue(TAG_MAC_LENGTH, &tag_length_bits))
+        if (!begin_params.GetTagValue(TAG_MAC_LENGTH, &tag_length_bits)) {
             *error = KM_ERROR_MISSING_MAC_LENGTH;
+            return nullptr;
+        }
         tag_length = tag_length_bits / 8;
         if (tag_length_bits % 8 != 0 || tag_length > GCM_MAX_TAG_LENGTH ||
-            tag_length < GCM_MIN_TAG_LENGTH)
+            tag_length < GCM_MIN_TAG_LENGTH) {
             *error = KM_ERROR_UNSUPPORTED_MAC_LENGTH;
+            return nullptr;
+        }
     }
 
     keymaster_padding_t padding;
@@ -95,14 +102,12 @@ Operation* AesOperationFactory::CreateOperation(const Key& key,
     if (!allows_padding(block_mode) && padding != KM_PAD_NONE) {
         LOG_E("Mode does not support padding", 0);
         *error = KM_ERROR_INCOMPATIBLE_PADDING_MODE;
+        return nullptr;
     }
 
     bool caller_nonce = key.authorizations().GetTagValue(TAG_CALLER_NONCE);
 
-    if (*error != KM_ERROR_OK)
-        return nullptr;
-
-    Operation* op = NULL;
+    Operation* op = nullptr;
     switch (purpose()) {
     case KM_PURPOSE_ENCRYPT:
         op = new (std::nothrow)
@@ -116,7 +121,7 @@ Operation* AesOperationFactory::CreateOperation(const Key& key,
         break;
     default:
         *error = KM_ERROR_UNSUPPORTED_PURPOSE;
-        return NULL;
+        return nullptr;
     }
 
     if (!op)
diff --git a/android_keymaster_test.cpp b/android_keymaster_test.cpp
index 341183b..cb120a6 100644
--- a/android_keymaster_test.cpp
+++ b/android_keymaster_test.cpp
@@ -2076,6 +2076,21 @@ TEST_P(EncryptionOperationsTest, AesEcbRoundTripSuccess) {
     EXPECT_EQ(0, GetParam()->keymaster0_calls());
 }
 
+TEST_P(EncryptionOperationsTest, AesEcbNotAuthorized) {
+    ASSERT_EQ(KM_ERROR_OK, GenerateKey(AuthorizationSetBuilder()
+                                           .AesEncryptionKey(128)
+                                           .Authorization(TAG_BLOCK_MODE, KM_MODE_CBC)
+                                           .Padding(KM_PAD_NONE)));
+    // Two-block message.
+    string message = "12345678901234567890123456789012";
+    AuthorizationSet begin_params(client_params());
+    begin_params.push_back(TAG_BLOCK_MODE, KM_MODE_ECB);
+    begin_params.push_back(TAG_PADDING, KM_PAD_NONE);
+    EXPECT_EQ(KM_ERROR_INCOMPATIBLE_BLOCK_MODE, BeginOperation(KM_PURPOSE_ENCRYPT, begin_params));
+
+    EXPECT_EQ(0, GetParam()->keymaster0_calls());
+}
+
 TEST_P(EncryptionOperationsTest, AesEcbNoPaddingWrongInputSize) {
     ASSERT_EQ(KM_ERROR_OK, GenerateKey(AuthorizationSetBuilder()
                                            .AesEncryptionKey(128)