Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix psa_generate_key(): return PSA_ERROR_INVALID_ARGUMENT for public key #5037

Merged
merged 5 commits into from
Oct 18, 2021
Merged

Fix psa_generate_key(): return PSA_ERROR_INVALID_ARGUMENT for public key #5037

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0fe820
psa_generate_key(): return PSA_ERROR_INVALID_ARGUMENT for public key
mprse Oct 7, 2021
770153e
Add change-log entry
mprse Oct 7, 2021
d9d630c
Addapt psa_generate_key() tests
mprse Oct 8, 2021
25f7063
enerate_psa_tests.py fix format
mprse Oct 8, 2021
b576c7b
Address review comments
mprse Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ChangeLog.d/fix-psa_gen_key-status.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Bugfix
* Fix status ret by psa_generate_key() for public key. Fixes #4551.
gilles-peskine-arm marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions library/psa_crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5703,6 +5703,10 @@ psa_status_t psa_generate_key( const psa_key_attributes_t *attributes,
if( psa_get_key_bits( attributes ) == 0 )
return( PSA_ERROR_INVALID_ARGUMENT );

/* Reject any attempt to create a public key. */
if( PSA_KEY_TYPE_IS_PUBLIC_KEY(attributes->core.type) )
return( PSA_ERROR_INVALID_ARGUMENT );

status = psa_start_key_creation( PSA_KEY_CREATION_GENERATE, attributes,
&slot, &driver );
if( status != PSA_SUCCESS )
Expand Down
17 changes: 11 additions & 6 deletions tests/scripts/generate_psa_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def read_psa_interface(self) -> macro_collector.PSAMacroEnumerator:
return constructors


def test_case_for_key_type_not_supported(
def test_case_for_key_type_not_supported_invalid_arg(
verb: str, key_type: str, bits: int,
dependencies: List[str],
*args: str,
Expand All @@ -148,10 +148,15 @@ def test_case_for_key_type_not_supported(
adverb = 'not' if dependencies else 'never'
if param_descr:
adverb = param_descr + ' ' + adverb
tc.set_description('PSA {} {} {}-bit {} supported'
.format(verb, short_key_type, bits, adverb))
if (verb == "generate") and ("PUBLIC" in short_key_type):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In NotSupported.test_cases_for_key_type_not_supported, the test for is-a-public-key is a bit different (kt.name.endswith('_PUBLIC_KEY')). I'm working on a follow-up that adds a method kt.is_public(). It would be better to have a single place that implements the test “is this key type for a public key?”, otherwise they risk diverging. Can you arrange to pass the information “is public key” down to this function?

Or maybe it would be better to have a completely separate function for the invalid-argument-for-public key case? I find this function rather messy. That's unavoidable to some extent because we want to produce output that has a lot of tweakable parts. Here, I suspect that keeping test_case_for_key_type_not_supported as it originally was, and making a separate function for the new case invalid-argument-for-public key, would make the code easier to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to confirm if I understand correctly your proposition:

  • restore original version of test_case_for_key_type_not_supported()
  • add test_case_for_key_type_invalid_argument()

In this solution we would also require to modify NotSupported class (change it to NotSupported_InvalidArgumentor addInvalidArgument class". What option you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I meant.

If we want to make this really clean, taking a high-level view, the job of NotSupported is to enumerate combinations of (key type, algorithm, operation) and generate test cases for when one of them is not supported. If a combination is inherently impossible, rather than not supported by the current configuration of the library, then it's not NotSupported's job to handle this. #4444 will introduce the generation of negative test cases for impossible combinations, but only for operations on existing keys, not for key generation (and not for key import either, but there are no impossible combinations for import). This leaves a gap of impossible combinations for key generation, which are just the cases where the key type is a public key. So, to be clean, there should be a separate CreationFail class (distinct from NotSupported and OpFail).

However, this is an evolving test script, we shouldn't expend too much energy on it. Since NotSupported already does much of the job of CreationFail, we can keep it doing that, with a comment explaining that it's doing a little extra job on the side.

Despite my saing we shouldn't expend too much energy on this, I do still prefer to restore the original test_case_for_key_type_not_supported and make a separate function for the invalid-generate case. I see renaming NotSupported as optional: not-supported is still its main job.

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've filed #5060 as a follow-up. It's not a top priority, but it is something we'll need eventually and it's a well-defined task that both adds a feature to generate_psa_tests.py and, I think, will make it a bit better structured. So @mprse if you don't have a more urgent task, I suggest you pick it up next.

tc.set_description('PSA {} {} {}-bit invalid argument'
.format(verb, short_key_type, bits))
tc.set_function(verb + '_invalid_arg')
else:
tc.set_description('PSA {} {} {}-bit {} supported'
.format(verb, short_key_type, bits, adverb))
tc.set_function(verb + '_not_supported')
tc.set_dependencies(dependencies)
tc.set_function(verb + '_not_supported')
tc.set_arguments([key_type] + list(args))
return tc

Expand Down Expand Up @@ -192,7 +197,7 @@ def test_cases_for_key_type_not_supported(
else:
generate_dependencies = import_dependencies
for bits in kt.sizes_to_test():
yield test_case_for_key_type_not_supported(
yield test_case_for_key_type_not_supported_invalid_arg(
'import', kt.expression, bits,
finish_family_dependencies(import_dependencies, bits),
test_case.hex_string(kt.key_material(bits)),
Expand All @@ -203,7 +208,7 @@ def test_cases_for_key_type_not_supported(
# supported or not depending on implementation capabilities,
# only generate the test case once.
continue
yield test_case_for_key_type_not_supported(
yield test_case_for_key_type_not_supported_invalid_arg(
'generate', kt.expression, bits,
finish_family_dependencies(generate_dependencies, bits),
str(bits),
Expand Down
2 changes: 1 addition & 1 deletion tests/suites/test_suite_psa_crypto.data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4705,7 +4705,7 @@ generate_random:2 * MBEDTLS_CTR_DRBG_MAX_REQUEST + 1

PSA generate key: bad type (RSA public key)
depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY
generate_key:PSA_KEY_TYPE_RSA_PUBLIC_KEY:512:PSA_KEY_USAGE_EXPORT:0:PSA_ERROR_NOT_SUPPORTED:0
generate_key:PSA_KEY_TYPE_RSA_PUBLIC_KEY:512:PSA_KEY_USAGE_EXPORT:0:PSA_ERROR_INVALID_ARGUMENT:0

PSA generate key: raw data, 0 bits: invalid argument
# The spec allows either INVALID_ARGUMENT or NOT_SUPPORTED
Expand Down
19 changes: 19 additions & 0 deletions tests/suites/test_suite_psa_crypto_not_supported.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,22 @@ exit:
PSA_DONE( );
}
/* END_CASE */

/* BEGIN_CASE */
void generate_invalid_arg( int key_type, int bits )
gilles-peskine-arm marked this conversation as resolved.
Show resolved Hide resolved
{
psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
mbedtls_svc_key_id_t key_id = INVALID_KEY_ID;

PSA_ASSERT( psa_crypto_init( ) );
psa_set_key_type( &attributes, key_type );
psa_set_key_bits( &attributes, bits );
TEST_EQUAL( psa_generate_key( &attributes, &key_id ),
PSA_ERROR_INVALID_ARGUMENT );
TEST_ASSERT( mbedtls_svc_key_id_equal( key_id, MBEDTLS_SVC_KEY_ID_INIT ) );

exit:
psa_destroy_key( key_id );
PSA_DONE( );
}
/* END_CASE */