Backport 2.28: Fix the detection of not-supported mechanisms in systematically generated PSA tests

library/psa_crypto.c

@@ -2259,6 +2259,50 @@ psa_status_t psa_copy_key(mbedtls_svc_key_id_t source_key,
 /* Message digests */
 /****************************************************************/
 
+static int is_hash_supported(psa_algorithm_t alg)
+{
+    switch (alg) {
+#if defined(PSA_WANT_ALG_MD2)
+        case PSA_ALG_MD2:
+            return 1;
+#endif
+#if defined(PSA_WANT_ALG_MD4)
+        case PSA_ALG_MD4:
+            return 1;
+#endif
+#if defined(PSA_WANT_ALG_MD5)
+        case PSA_ALG_MD5:
+            return 1;
+#endif
+#if defined(PSA_WANT_ALG_RIPEMD160)
+        case PSA_ALG_RIPEMD160:
+            return 1;
+#endif
+#if defined(PSA_WANT_ALG_SHA_1)
+        case PSA_ALG_SHA_1:
+            return 1;
+#endif
+#if defined(PSA_WANT_ALG_SHA_224)
+        case PSA_ALG_SHA_224:
+            return 1;
+#endif
+#if defined(PSA_WANT_ALG_SHA_256)
+        case PSA_ALG_SHA_256:
+            return 1;
+#endif
+#if defined(PSA_WANT_ALG_SHA_384)
+        case PSA_ALG_SHA_384:
+            return 1;
+#endif
+#if defined(PSA_WANT_ALG_SHA_512)
+        case PSA_ALG_SHA_512:
+            return 1;
+#endif
+        default:
+            return 0;
+    }
+}
+
 psa_status_t psa_hash_abort(psa_hash_operation_t *operation)
 {
     /* Aborting a non-active operation is allowed */
@@ -2913,16 +2957,44 @@ static psa_status_t psa_sign_verify_check_alg(int input_is_message,
         if (!PSA_ALG_IS_SIGN_MESSAGE(alg)) {
             return PSA_ERROR_INVALID_ARGUMENT;
         }
+    }
 
-        if (PSA_ALG_IS_SIGN_HASH(alg)) {
-            if (!PSA_ALG_IS_HASH(PSA_ALG_SIGN_GET_HASH(alg))) {
-                return PSA_ERROR_INVALID_ARGUMENT;
-            }
-        }
-    } else {
-        if (!PSA_ALG_IS_SIGN_HASH(alg)) {
-            return PSA_ERROR_INVALID_ARGUMENT;
-        }
+    psa_algorithm_t hash_alg = 0;
+    if (PSA_ALG_IS_SIGN_HASH(alg)) {
+        hash_alg = PSA_ALG_SIGN_GET_HASH(alg);
+    }
+
+    /* Now hash_alg==0 if alg by itself doesn't need a hash.
+     * This is good enough for sign-hash, but a guaranteed failure for
+     * sign-message which needs to hash first for all algorithms
+     * supported at the moment. */
+
+    if (hash_alg == 0 && input_is_message) {
+        return PSA_ERROR_INVALID_ARGUMENT;
+    }
+    if (hash_alg == PSA_ALG_ANY_HASH) {
+        return PSA_ERROR_INVALID_ARGUMENT;
+    }
+    /* Give up immediately if the hash is not supported. This has
+     * several advantages:
+     * - For mechanisms that don't use the hash at all (e.g.
+     *   ECDSA verification, randomized ECDSA signature), without
+     *   this check, the operation would succeed even though it has
+     *   been given an invalid argument. This would not be insecure
+     *   since the hash was not necessary, but it would be weird.
+     * - For mechanisms that do use the hash, we avoid an error
+     *   deep inside the execution. In principle this doesn't matter,
+     *   but there is a little more risk of a bug in error handling
+     *   deep inside than in this preliminary check.
+     * - When calling a driver, the driver might be capable of using
+     *   a hash that the core doesn't support. This could potentially
+     *   result in a buffer overflow if the hash is larger than the
+     *   maximum hash size assumed by the core.
+     * - Returning a consistent error makes it possible to test
+     *   not-supported hashes in a consistent way.
+     */
+    if (hash_alg != 0 && !is_hash_supported(hash_alg)) {
+        return PSA_ERROR_NOT_SUPPORTED;
     }
 
     return PSA_SUCCESS;

scripts/mbedtls_dev/psa_information.py

@@ -6,9 +6,10 @@
 
 
 import re
-from typing import Dict, FrozenSet, List, Optional
+from typing import Dict, FrozenSet, Iterator, List, Optional, Set
 
 from . import macro_collector
+from . import test_case
 
 
 def psa_want_symbol(name: str) -> str:
@@ -53,26 +54,6 @@ def automatic_dependencies(*expressions: str) -> List[str]:
     used.difference_update(SYMBOLS_WITHOUT_DEPENDENCY)
     return sorted(psa_want_symbol(name) for name in used)
 
-# A temporary hack: at the time of writing, not all dependency symbols
-# are implemented yet. Skip test cases for which the dependency symbols are
-# not available. Once all dependency symbols are available, this hack must
-# be removed so that a bug in the dependency symbols properly leads to a test
-# failure.
-def read_implemented_dependencies(filename: str) -> FrozenSet[str]:
-    return frozenset(symbol
-                     for line in open(filename)
-                     for symbol in re.findall(r'\bPSA_WANT_\w+\b', line))
-_implemented_dependencies = None #type: Optional[FrozenSet[str]] #pylint: disable=invalid-name
-
-def hack_dependencies_not_implemented(dependencies: List[str]) -> None:
-    global _implemented_dependencies #pylint: disable=global-statement,invalid-name
-    if _implemented_dependencies is None:
-        _implemented_dependencies = \
-            read_implemented_dependencies('include/psa/crypto_config.h')
-    if not all((dep.lstrip('!') in _implemented_dependencies or
-                not dep.lstrip('!').startswith('PSA_WANT'))
-               for dep in dependencies):
-        dependencies.append('DEPENDENCY_NOT_IMPLEMENTED_YET')
 
 class Information:
     """Gather information about PSA constructors."""
@@ -84,8 +65,13 @@ def __init__(self) -> None:
     def remove_unwanted_macros(
             constructors: macro_collector.PSAMacroEnumerator
     ) -> None:
-        # Mbed TLS doesn't support finite-field DH yet and will not support
-        # finite-field DSA. Don't attempt to generate any related test case.
+        """Remove macros from consideration during value enumeration."""
+        # Remove some mechanisms that are declared but not implemented.
+        # The corresponding test cases would be commented out anyway
+        # thanks to the detect_not_implemented_dependencies mechanism,
+        # but for those particular key types, we don't even have enough
+        # support in the test scripts to construct test keys. So
+        # we arrange to not even attempt to generate test cases.
         constructors.key_types.discard('PSA_KEY_TYPE_DH_KEY_PAIR')
         constructors.key_types.discard('PSA_KEY_TYPE_DH_PUBLIC_KEY')
         constructors.key_types.discard('PSA_KEY_TYPE_DSA_KEY_PAIR')
@@ -104,3 +90,94 @@ def read_psa_interface(self) -> macro_collector.PSAMacroEnumerator:
         self.remove_unwanted_macros(constructors)
         constructors.gather_arguments()
         return constructors
+
+
+class TestCase(test_case.TestCase):
+    """A PSA test case with automatically inferred dependencies.
+
+    For mechanisms like ECC curves where the support status includes
+    the key bit-size, this class assumes that only one bit-size is
+    involved in a given test case.
+    """
+
+    # Use a class variable to cache the set of implemented dependencies.
+    # Call read_implemented_dependencies() to fill the cache.
+    _implemented_dependencies = None #type: Optional[FrozenSet[str]]
+
+    DEPENDENCY_SYMBOL_RE = re.compile(r'\bPSA_WANT_\w+\b')
+    @classmethod
+    def _yield_implemented_dependencies(cls) -> Iterator[str]:
+        for filename in ['include/psa/crypto_config.h',
+                         'include/mbedtls/config_psa.h']:
+            with open(filename) as inp:
+                content = inp.read()
+            yield from cls.DEPENDENCY_SYMBOL_RE.findall(content)
+
+    @classmethod
+    def read_implemented_dependencies(cls) -> FrozenSet[str]:
+        if cls._implemented_dependencies is None:
+            cls._implemented_dependencies = \
+                frozenset(cls._yield_implemented_dependencies())
+            # Redundant return to reassure pylint (mypy is fine without it).
+            # Known issue: https://github.com/pylint-dev/pylint/issues/3045
+            return cls._implemented_dependencies
+        return cls._implemented_dependencies
+
+    # We skip test cases for which the dependency symbols are not defined.
+    # We assume that this means that a required mechanism is not implemented.
+    # Note that if we erroneously skip generating test cases for
+    # mechanisms that are not implemented, this should be caught
+    # by the NOT_SUPPORTED test cases generated by generate_psa_tests.py
+    # in test_suite_psa_crypto_not_supported and test_suite_psa_crypto_op_fail:
+    # those emit negative tests, which will not be skipped here.
+    def detect_not_implemented_dependencies(self) -> None:
+        """Detect dependencies that are not implemented."""
+        all_implemented_dependencies = self.read_implemented_dependencies()
+        not_implemented = [dep
+                           for dep in self.dependencies
+                           if (dep.startswith('PSA_WANT') and
+                               dep not in all_implemented_dependencies)]
+        if not_implemented:
+            self.skip_because('not implemented: ' +
+                              ' '.join(not_implemented))
+
+    def __init__(self) -> None:
+        super().__init__()
+        self.key_bits = None #type: Optional[int]
+        self.negated_dependencies = set() #type: Set[str]
+
+    def assumes_not_supported(self, name: str) -> None:
+        """Negate the given mechanism for automatic dependency generation.
+
+        Call this function before set_arguments() for a test case that should
+        run if the given mechanism is not supported.
+
+        A mechanism is either a PSA_XXX symbol (e.g. PSA_KEY_TYPE_AES,
+        PSA_ALG_HMAC, etc.) or a PSA_WANT_XXX symbol.
+        """
+        symbol = name
+        if not symbol.startswith('PSA_WANT_'):
+            symbol = psa_want_symbol(name)
+        self.negated_dependencies.add(symbol)
+
+    def set_key_bits(self, key_bits: Optional[int]) -> None:
+        """Use the given key size for automatic dependency generation.
+
+        Call this function before set_arguments() if relevant.
+
+        This is only relevant for ECC and DH keys. For other key types,
+        this information is ignored.
+        """
+        self.key_bits = key_bits
+
+    def set_arguments(self, arguments: List[str]) -> None:
+        """Set test case arguments and automatically infer dependencies."""
+        super().set_arguments(arguments)
+        dependencies = automatic_dependencies(*arguments)
+        for i in range(len(dependencies)): #pylint: disable=consider-using-enumerate
+            if dependencies[i] in self.negated_dependencies:
+                dependencies[i] = '!' + dependencies[i]
+        if self.key_bits is not None:
+            dependencies = finish_family_dependencies(dependencies, self.key_bits)
+        self.dependencies += sorted(dependencies)
+        self.detect_not_implemented_dependencies()

scripts/mbedtls_dev/test_case.py

@@ -31,6 +31,7 @@ def __init__(self, description: Optional[str] = None):
         self.dependencies = [] #type: List[str]
         self.function = None #type: Optional[str]
         self.arguments = [] #type: List[str]
+        self.skip_reason = ''
 
     def add_comment(self, *lines: str) -> None:
         self.comments += lines
@@ -47,6 +48,23 @@ def set_function(self, function: str) -> None:
     def set_arguments(self, arguments: List[str]) -> None:
         self.arguments = arguments
 
+    def skip_because(self, reason: str) -> None:
+        """Skip this test case.
+
+        It will be included in the output, but commented out.
+
+        This is intended for test cases that are obtained from a
+        systematic enumeration, but that have dependencies that cannot
+        be fulfilled. Since we don't want to have test cases that are
+        never executed, we arrange not to have actual test cases. But
+        we do include comments to make it easier to understand the output
+        of test case generation.
+
+        reason must be a non-empty string explaining to humans why this
+        test case is skipped.
+        """
+        self.skip_reason = reason
+
     def check_completeness(self) -> None:
         if self.description is None:
             raise MissingDescription
@@ -67,10 +85,16 @@ def write(self, out: typing_util.Writable) -> None:
         out.write('\n')
         for line in self.comments:
             out.write('# ' + line + '\n')
-        out.write(self.description + '\n')
+        prefix = ''
+        if self.skip_reason:
+            prefix = '## '
+            out.write('## # skipped because: ' + self.skip_reason + '\n')
+        out.write(prefix + self.description + '\n')
         if self.dependencies:
-            out.write('depends_on:' + ':'.join(self.dependencies) + '\n')
-        out.write(self.function + ':' + ':'.join(self.arguments) + '\n')
+            out.write(prefix + 'depends_on:' +
+                      ':'.join(self.dependencies) + '\n')
+        out.write(prefix + self.function + ':' +
+                  ':'.join(self.arguments) + '\n')
 
 def write_data_file(filename: str,
                     test_cases: Iterable[TestCase],