Skip to content

Make psa_close_key(0) and psa_destroy_key(0) succeed #292

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

Merged
gilles-peskine-arm merged 5 commits into from
Oct 14, 2019
Merged

Make psa_close_key(0) and psa_destroy_key(0) succeed #292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2493401
Document that psa_close_key(0) and psa_destroy_key(0) succeed
gilles-peskine-arm Oct 8, 2019
f102e4e
Test that psa_close_key(0) and psa_destroy_key(0) succeed
gilles-peskine-arm Oct 8, 2019
1841cf4
Make psa_close_key(0) and psa_destroy_key(0) succeed
gilles-peskine-arm Oct 8, 2019
04129a0
Update slot management tests now that {close,destroy}_key(0) succeed
gilles-peskine-arm Oct 9, 2019
b8cde4e
Consolidate invalid-handle tests
gilles-peskine-arm Oct 11, 2019
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
9 changes: 8 additions & 1 deletion include/psa/crypto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,9 +459,12 @@ psa_status_t psa_open_key(psa_key_id_t id,
* maintain the key handle until after the multipart operation has finished.
*
* \param handle The key handle to close.
* If this is \c 0, do nothing and return \c PSA_SUCCESS.
*
* \retval #PSA_SUCCESS
* \p handle was a valid handle or \c 0. It is now closed.
* \retval #PSA_ERROR_INVALID_HANDLE
* \p handle is not a valid handle nor \c 0.
* \retval #PSA_ERROR_COMMUNICATION_FAILURE
* \retval #PSA_ERROR_CORRUPTION_DETECTED
* \retval #PSA_ERROR_BAD_STATE
Expand Down Expand Up @@ -579,13 +582,17 @@ psa_status_t psa_copy_key(psa_key_handle_t source_handle,
* key will cause the multipart operation to fail.
*
* \param handle Handle to the key to erase.
* If this is \c 0, do nothing and return \c PSA_SUCCESS.
*
* \retval #PSA_SUCCESS
* The key material has been erased.
* \p handle was a valid handle and the key material that it
* referred to has been erased.
* Alternatively, \p handle is \c 0.
* \retval #PSA_ERROR_NOT_PERMITTED
* The key cannot be erased because it is
* read-only, either due to a policy or due to physical restrictions.
* \retval #PSA_ERROR_INVALID_HANDLE
* \p handle is not a valid handle nor \c 0.
* \retval #PSA_ERROR_COMMUNICATION_FAILURE
* There was an failure in communication with the cryptoprocessor.
* The key material may still be present in the cryptoprocessor.
Expand Down
3 changes: 3 additions & 0 deletions library/psa_crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,9 @@ psa_status_t psa_destroy_key( psa_key_handle_t handle )
psa_se_drv_table_entry_t *driver;
#endif /* MBEDTLS_PSA_CRYPTO_SE_C */

if( handle == 0 )
return( PSA_SUCCESS );

status = psa_get_key_slot( handle, &slot );
if( status != PSA_SUCCESS )
return( status );
Expand Down
3 changes: 3 additions & 0 deletions library/psa_crypto_slot_management.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,9 @@ psa_status_t psa_close_key( psa_key_handle_t handle )
psa_status_t status;
psa_key_slot_t *slot;

if( handle == 0 )
return( PSA_SUCCESS );

status = psa_get_key_slot( handle, &slot );
if( status != PSA_SUCCESS )
return( status );
Expand Down
9 changes: 0 additions & 9 deletions tests/suites/test_suite_psa_crypto.data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,6 @@ PSA import/export AES-256
depends_on:MBEDTLS_AES_C
import_export:"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef":PSA_KEY_TYPE_AES:PSA_KEY_USAGE_EXPORT:PSA_ALG_CTR:256:0:PSA_SUCCESS:1

PSA invalid handle (0)
invalid_handle:0

PSA invalid handle (smallest plausible handle)
invalid_handle:1

PSA invalid handle (largest plausible handle)
invalid_handle:-1

PSA import: bad usage flag
import_with_policy:PSA_KEY_TYPE_RAW_DATA:0x40000000:0:PSA_ERROR_INVALID_ARGUMENT

Expand Down
14 changes: 0 additions & 14 deletions tests/suites/test_suite_psa_crypto.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1103,9 +1103,6 @@ static int test_operations_on_invalid_handle( psa_key_handle_t handle )
buffer, sizeof( buffer ), &length ),
PSA_ERROR_INVALID_HANDLE );

TEST_EQUAL( psa_close_key( handle ), PSA_ERROR_INVALID_HANDLE );
TEST_EQUAL( psa_destroy_key( handle ), PSA_ERROR_INVALID_HANDLE );

ok = 1;

exit:
Expand Down Expand Up @@ -1535,17 +1532,6 @@ exit:
}
/* END_CASE */

/* BEGIN_CASE */
void invalid_handle( int handle )
{
PSA_ASSERT( psa_crypto_init( ) );
test_operations_on_invalid_handle( handle );

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

/* BEGIN_CASE */
void import_export_public_key( data_t *data,
int type_arg,
Expand Down
13 changes: 11 additions & 2 deletions tests/suites/test_suite_psa_crypto_slot_management.data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,17 @@ Copy persistent to same
depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C
copy_to_occupied:PSA_KEY_LIFETIME_PERSISTENT:1:PSA_KEY_USAGE_EXPORT | PSA_KEY_USAGE_COPY:PSA_ALG_CTR:PSA_KEY_TYPE_AES:"404142434445464748494a4b4c4d4e4f":PSA_KEY_LIFETIME_PERSISTENT:1:PSA_KEY_USAGE_EXPORT:PSA_ALG_CTR:PSA_KEY_TYPE_AES:"404142434445464748494a4b4c4d4e4f"

Close/destroy invalid handle
invalid_handle:
invalid handle: 0
invalid_handle:INVALID_HANDLE_0:PSA_SUCCESS:PSA_ERROR_INVALID_HANDLE

invalid handle: never opened
invalid_handle:INVALID_HANDLE_UNOPENED:PSA_ERROR_INVALID_HANDLE:PSA_ERROR_INVALID_HANDLE

invalid handle: already closed
invalid_handle:INVALID_HANDLE_CLOSED:PSA_ERROR_INVALID_HANDLE:PSA_ERROR_INVALID_HANDLE

invalid handle: huge
invalid_handle:INVALID_HANDLE_HUGE:PSA_ERROR_INVALID_HANDLE:PSA_ERROR_INVALID_HANDLE

Open many transient handles
many_transient_handles:42
65 changes: 52 additions & 13 deletions tests/suites/test_suite_psa_crypto_slot_management.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ typedef enum
CLOSE_AFTER,
} reopen_policy_t;

typedef enum
{
INVALID_HANDLE_0,
INVALID_HANDLE_UNOPENED,
INVALID_HANDLE_CLOSED,
INVALID_HANDLE_HUGE,
} invalid_handle_construction_t;

/* All test functions that create persistent keys must call
* `TEST_USES_KEY_ID( key_id )` before creating a persistent key with this
* identifier, and must call psa_purge_key_storage() in their cleanup
Expand Down Expand Up @@ -625,9 +633,13 @@ exit:
/* END_CASE */

/* BEGIN_CASE */
void invalid_handle( )
void invalid_handle( int handle_construction,
int close_status_arg, int usage_status_arg )
{
psa_key_handle_t handle1 = 0;
psa_key_handle_t valid_handle = 0;
psa_key_handle_t invalid_handle = 0;
psa_status_t close_status = close_status_arg;
psa_status_t usage_status = usage_status_arg;
psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
uint8_t material[1] = "a";

Expand All @@ -639,23 +651,50 @@ void invalid_handle( )
psa_set_key_algorithm( &attributes, 0 );
PSA_ASSERT( psa_import_key( &attributes,
material, sizeof( material ),
&handle1 ) );
TEST_ASSERT( handle1 != 0 );
&valid_handle ) );
TEST_ASSERT( valid_handle != 0 );

/* Attempt to close and destroy some invalid handles. */
TEST_EQUAL( psa_close_key( 0 ), PSA_ERROR_INVALID_HANDLE );
TEST_EQUAL( psa_close_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE );
TEST_EQUAL( psa_close_key( handle1 + 1 ), PSA_ERROR_INVALID_HANDLE );
TEST_EQUAL( psa_destroy_key( 0 ), PSA_ERROR_INVALID_HANDLE );
TEST_EQUAL( psa_destroy_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE );
TEST_EQUAL( psa_destroy_key( handle1 + 1 ), PSA_ERROR_INVALID_HANDLE );
/* Construct an invalid handle as specified in the test case data. */
switch( handle_construction )
{
case INVALID_HANDLE_0:
invalid_handle = 0;
break;
case INVALID_HANDLE_UNOPENED:
/* We can't easily construct a handle that's never been opened
* without knowing how the implementation constructs handle
* values. The current test code assumes that valid handles
* are in a range between 1 and some maximum. */
if( valid_handle == 1 )
invalid_handle = 2;
else
invalid_handle = valid_handle - 1;
Copy link
Contributor

@Patater Patater Oct 11, 2019

Choose a reason for hiding this comment

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

Do we sometimes want to test invalid_handle = valid_handle + 1? We may need a INVALID_HANDLE_UNOPENED_ABOVE for this, renaming the - 1 case to INVALID_HANDLE_UNOPENED_BELOW.

Copy link
Collaborator Author

@gilles-peskine-arm gilles-peskine-arm Oct 11, 2019

Choose a reason for hiding this comment

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

I think it would be better to test both, but you objected to that.

Copy link
Contributor

@Patater Patater Oct 11, 2019

Choose a reason for hiding this comment

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

I objected to the tests silently only testing one way without letting us know we reduced test coverage.

Copy link
Collaborator Author

@gilles-peskine-arm gilles-peskine-arm Oct 11, 2019

Choose a reason for hiding this comment

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

So it's a misunderstanding then? The tests were testing both ways, skipping one of the ways if it was not applicable.

Copy link
Contributor

@Patater Patater Oct 14, 2019

Choose a reason for hiding this comment

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

I didn't like skipping if one wasn't applicable, as both might be silently skipped. I'd like to always see both ways tested (above and below). If this means we have to allocate multiple keys, we should do that.

Copy link
Collaborator Author

@gilles-peskine-arm gilles-peskine-arm Oct 14, 2019

Choose a reason for hiding this comment

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

No, it was not possible for both to be silently skipped. handle1 - 1 != 0 and handle1 + 1 != 0 can't both be false.

Copy link
Contributor

@Patater Patater Oct 14, 2019

Choose a reason for hiding this comment

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

OK

break;
case INVALID_HANDLE_CLOSED:
PSA_ASSERT( psa_import_key( &attributes,
material, sizeof( material ),
&invalid_handle ) );
PSA_ASSERT( psa_destroy_key( invalid_handle ) );
break;
case INVALID_HANDLE_HUGE:
invalid_handle = (psa_key_handle_t) ( -1 );
break;
default:
TEST_ASSERT( ! "unknown handle construction" );
}

/* Attempt to use the invalid handle. */
TEST_EQUAL( psa_get_key_attributes( invalid_handle, &attributes ),
usage_status );
TEST_EQUAL( psa_close_key( invalid_handle ), close_status );
TEST_EQUAL( psa_destroy_key( invalid_handle ), close_status );

/* After all this, check that the original handle is intact. */
PSA_ASSERT( psa_get_key_attributes( handle1, &attributes ) );
PSA_ASSERT( psa_get_key_attributes( valid_handle, &attributes ) );
TEST_EQUAL( psa_get_key_type( &attributes ), PSA_KEY_TYPE_RAW_DATA );
TEST_EQUAL( psa_get_key_bits( &attributes ),
PSA_BYTES_TO_BITS( sizeof( material ) ) );
PSA_ASSERT( psa_close_key( handle1 ) );
PSA_ASSERT( psa_close_key( valid_handle ) );

exit:
PSA_DONE( );
Expand Down