From a9bbd8271db419d87a44466ee0156e36f8031115 Mon Sep 17 00:00:00 2001 From: Archit Aggarwal Date: Sun, 15 Aug 2021 19:37:31 +0000 Subject: [PATCH] Fix complexity and hygiene improvements --- source/include/sigv4_internal.h | 2 -- source/sigv4.c | 24 ++++++++----------- source/sigv4_quicksort.c | 16 ++++++------- test/unit-test/sigv4_utest.c | 42 +++++++-------------------------- 4 files changed, 27 insertions(+), 57 deletions(-) diff --git a/source/include/sigv4_internal.h b/source/include/sigv4_internal.h index d1448d63..98dc3760 100644 --- a/source/include/sigv4_internal.h +++ b/source/include/sigv4_internal.h @@ -28,8 +28,6 @@ #ifndef SIGV4_INTERNAL_H_ #define SIGV4_INTERNAL_H_ -#include - /* SIGV4_DO_NOT_USE_CUSTOM_CONFIG allows building of the SigV4 library without a * config file. If a config file is provided, the SIGV4_DO_NOT_USE_CUSTOM_CONFIG * macro must not be defined. diff --git a/source/sigv4.c b/source/sigv4.c index 795e0f2d..8fd20cf8 100644 --- a/source/sigv4.c +++ b/source/sigv4.c @@ -2272,6 +2272,11 @@ static int32_t hmacAddKey( HmacContext_t * pHmacContext, } else { + /* To reduce the key length to less than the hash block size, this branch performs + * hash operations. We want to perform hash operations only when we have received the + * entire key. */ + assert( isKeyPrefix == false ); + returnStatus = pCryptoInterface->hashInit( pCryptoInterface->pHashContext ); /* Has part of the key that is cached in the HMAC context. */ @@ -2813,6 +2818,7 @@ static SigV4Status_t generateSigningKey( const SigV4Parameters_t * pSigV4Params, SIGV4_HMAC_SIGNING_KEY_PREFIX, SIGV4_HMAC_SIGNING_KEY_PREFIX_LEN, true /* Is key prefix. */ ); + /* The above call should always succeed as it only populates the HMAC key cache. */ assert( hmacStatus == 0 ); } @@ -2988,20 +2994,9 @@ SigV4Status_t SigV4_GenerateHTTPAuthorization( const SigV4Parameters_t * pParams if( returnStatus == SigV4Success ) { - authPrefixLen = *authBufLen; - - /* Default arguments. */ - if( ( pParams->pAlgorithm == NULL ) || ( pParams->algorithmLen == 0U ) ) - { - /* The default algorithm is AWS4-HMAC-SHA256. */ - pAlgorithm = SIGV4_AWS4_HMAC_SHA256; - algorithmLen = SIGV4_AWS4_HMAC_SHA256_LENGTH; - } - else - { - pAlgorithm = pParams->pAlgorithm; - algorithmLen = pParams->algorithmLen; - } + /* If the SigV4 algorithm is not specified, use "AWS4-HMAC-256" as the default algorithm. */ + pAlgorithm = ( pParams->pAlgorithm == NULL ) ? SIGV4_AWS4_HMAC_SHA256 : pParams->pAlgorithm; + algorithmLen = ( pParams->pAlgorithm == NULL ) ? SIGV4_AWS4_HMAC_SHA256_LENGTH : pParams->algorithmLen; } if( returnStatus == SigV4Success ) @@ -3025,6 +3020,7 @@ SigV4Status_t SigV4_GenerateHTTPAuthorization( const SigV4Parameters_t * pParams /* Write the prefix of the Authorizaton header value. */ if( returnStatus == SigV4Success ) { + authPrefixLen = *authBufLen; returnStatus = generateAuthorizationValuePrefix( pParams, pAlgorithm, algorithmLen, pSignedHeaders, signedHeadersLen, diff --git a/source/sigv4_quicksort.c b/source/sigv4_quicksort.c index f117b269..9a637e2e 100644 --- a/source/sigv4_quicksort.c +++ b/source/sigv4_quicksort.c @@ -147,8 +147,7 @@ static void quickSortHelper( void * pArray, /* Calculate length of the left partition containing items smaller * than the pivot element. * The length is zero if either: - * 1. The pivoted item is the smallest in the - * the array before partitioning. + * 1. The pivoted item is the smallest in the the array before partitioning. * OR * 2. The left partition is only of single length which can be treated as * sorted, and thus, of zero length for avoided adding to the stack. */ @@ -157,7 +156,7 @@ static void quickSortHelper( void * pArray, /* Calculate length of the right partition containing items greater than * or equal to the pivot item. * The calculated length is zero if either: - * 1. The pivoted item is the smallest in the the array before partitioning. + * 1. The pivoted item is the greatest in the the array before partitioning. * OR * 2. The right partition contains only a single length which can be treated as * sorted, and thereby, of zero length to avoid adding to the stack. */ @@ -202,14 +201,15 @@ static size_t partition( void * pArray, size_t itemSize, ComparisonFunc_t comparator ) { - void * pivot; + uint8_t * pivot; + uint8_t * pArrayLocal = ( uint8_t * ) pArray; size_t i = low - 1U, j = low; assert( pArray != NULL ); assert( comparator != NULL ); /* Choose pivot as the highest indexed item in the current partition. */ - pivot = pArray + ( high * itemSize ); + pivot = pArrayLocal + ( high * itemSize ); /* Iterate over all elements of the current array to partition it * in comparison to the chosen pivot with smaller items on the left @@ -217,16 +217,16 @@ static size_t partition( void * pArray, for( ; j < high; j++ ) { /* Use comparator function to check current element is smaller than the pivot */ - if( comparator( pArray + ( j * itemSize ), pivot ) < 0 ) + if( comparator( pArrayLocal + ( j * itemSize ), pivot ) < 0 ) { ++i; - swap( pArray + ( i * itemSize ), pArray + ( j * itemSize ), itemSize ); + swap( pArrayLocal + ( i * itemSize ), pArrayLocal + ( j * itemSize ), itemSize ); } } /* Place the pivot between the smaller and larger item chunks of * the array. This represents the 2 partitions of the array. */ - swap( pArray + ( ( i + 1U ) * itemSize ), pivot, itemSize ); + swap( pArrayLocal + ( ( i + 1U ) * itemSize ), pivot, itemSize ); /* Return the pivot item's index. */ return i + 1U; diff --git a/test/unit-test/sigv4_utest.c b/test/unit-test/sigv4_utest.c index 9dbd0f4a..dfdc377c 100644 --- a/test/unit-test/sigv4_utest.c +++ b/test/unit-test/sigv4_utest.c @@ -245,37 +245,9 @@ static int32_t valid_sha256_final( void * pHashContext, return -1; } -/*==================== Echo Implementation of Crypto Interface ===================== */ - -static uint8_t hashEchoBuffer[ SIGV4_HASH_MAX_BLOCK_LENGTH ]; -static size_t hashInputLen; - -/* These hash functions simply take the input and write it back to the output. - * The purpose of which is make it possible to write tests without having to - * know the computed hash of the string to sign. */ -static int32_t echo_hash_init( void * pHashContext ) -{ - return 0; -} - -static int32_t echo_hash_update( void * pHashContext, - const uint8_t * pInput, - size_t inputLen ) -{ - hashInputLen = inputLen; - ( void ) memcpy( hashEchoBuffer, pInput, inputLen ); -} - -static int32_t echo_hash_final( void * pHashContext, - uint8_t * pOutput, - size_t outputLen ) -{ - ( void ) memcpy( pOutput, hashEchoBuffer, hashInputLen ); -} - /*==================== Failable Implementation of Crypto Interface ===================== */ -#define HAPPY_PATH_HASH_ITERATIONS 12U +#define HASH_ERROR_BRANCH_COVERAGE_ITERATIONS 12U static size_t hashInitCalledCount = 0U, hashInitCallToFail = SIZE_MAX; static size_t updateHashCalledCount = 0U, updateHashCallToFail = SIZE_MAX; @@ -1186,24 +1158,28 @@ void test_SigV4_GenerateHTTPAuthorization_Hash_Errors() params.pCredentials->pSecretAccessKey = SECRET_KEY_LONGER_THAN_HASH_BLOCK; params.pCredentials->secretAccessKeyLen = strlen( SECRET_KEY_LONGER_THAN_HASH_BLOCK ); + char failureMessage[ 250 ]; - for( i = 0U; i < HAPPY_PATH_HASH_ITERATIONS; i++ ) + for( i = 0U; i < HASH_ERROR_BRANCH_COVERAGE_ITERATIONS; i++ ) { resetFailableHashParams(); hashInitCallToFail = i; params.pCryptoInterface->hashInit = hash_init_failable; returnStatus = SigV4_GenerateHTTPAuthorization( ¶ms, authBuf, &authBufLen, &signature, &signatureLen ); - TEST_ASSERT_EQUAL( SigV4HashError, returnStatus ); + snprintf( failureMessage, sizeof( failureMessage ), "Expected SigV4HashError from hashInit failure at call count %ld", i ); + TEST_ASSERT_EQUAL_MESSAGE( SigV4HashError, returnStatus, failureMessage ); resetFailableHashParams(); updateHashCallToFail = i; returnStatus = SigV4_GenerateHTTPAuthorization( ¶ms, authBuf, &authBufLen, &signature, &signatureLen ); - TEST_ASSERT_EQUAL( SigV4HashError, returnStatus ); + snprintf( failureMessage, sizeof( failureMessage ), "Expected SigV4HashError from hashUpdate failure at call count %ld", i ); + TEST_ASSERT_EQUAL_MESSAGE( SigV4HashError, returnStatus, failureMessage ); resetFailableHashParams(); finalHashCallToFail = i; returnStatus = SigV4_GenerateHTTPAuthorization( ¶ms, authBuf, &authBufLen, &signature, &signatureLen ); - TEST_ASSERT_EQUAL( SigV4HashError, returnStatus ); + snprintf( failureMessage, sizeof( failureMessage ), "Expected SigV4HashError from hashFinal failure at call count %ld", i ); + TEST_ASSERT_EQUAL_MESSAGE( SigV4HashError, returnStatus, failureMessage ); } }