diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 24d627f7..4a1bf926 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,34 +8,68 @@ on: workflow_dispatch: jobs: - unittest: + unittests-sanitizer: runs-on: ubuntu-latest steps: - name: Clone This Repo uses: actions/checkout@v2 - - name: Build + - name: Build with Sanitizers run: | - sudo apt-get install -y lcov - cmake -S test -B build/ \ + sudo apt-get install -y cmake lcov + CFLAGS=" -O0 -Wall -Wextra" + CFLAGS+=" -Werror -Wno-error=pedantic" + CFLAGS+=" -D_FORTIFY_SOURCE=2" + CFLAGS+=" -Wformat" + CLFAGS+=" -Wformat-security" + CFLAGS+=" -Warray-bounds" + CFLAGS+=" -fsanitize=address,undefined" + CFLAGS+=" -fsanitize=pointer-compare -fsanitize=pointer-subtract" + CFLAGS+=" -fsanitize-recover=undefined" + CFLAGS+=" -fsanitize-address-use-after-scope" + CFLAGS+=" -fsanitize-undefined-trap-on-error" + CFLAGS_=" -fstack-protector-all" + cmake -S test -B build \ -G "Unix Makefiles" \ -DCMAKE_BUILD_TYPE=Debug \ - -DCMAKE_C_FLAGS='--coverage -Wall -Wextra -DNDEBUG' - make -C build/ all - - name: Test + -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON \ + -DCMAKE_C_FLAGS="${CFLAGS}" + make -C build all + - name: Run Tests with Sanitizers run: | - cd build/ + cd build + make coverage ctest -E system --output-on-failure cd .. - - name: Run Coverage + unittests-coverage: + runs-on: ubuntu-latest + steps: + - name: Clone This Repo + uses: actions/checkout@v2 + - name: Build Tests for Coverage run: | - make -C build/ coverage + sudo apt-get install -y cmake lcov + CFLAGS=" --coverage -O0 -Wall -Wextra" + CFLAGS+=" -Werror -Wno-error=pedantic" + CFLAGS+=" -DNDEBUG" + cmake -S test -B build_cov \ + -G "Unix Makefiles" \ + -DCMAKE_BUILD_TYPE=Debug \ + -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON \ + -DCMAKE_C_FLAGS="${CFLAGS}" + make -C build_cov all + - name: Run Tests for Coverage + run: | + cd build_cov + make coverage + ctest -E system --output-on-failure + cd .. declare -a EXCLUDE=("\*test\*" "\*CMakeCCompilerId\*" "\*mocks\*" "\*3rdparty\*") - echo ${EXCLUDE[@]} | xargs lcov --rc lcov_branch_coverage=1 -r build/coverage.info -o build/coverage.info - lcov --rc lcov_branch_coverage=1 --list build/coverage.info + echo ${EXCLUDE[@]} | xargs lcov --rc lcov_branch_coverage=1 -r build_cov/coverage.info -o build_cov/coverage.info + lcov --rc lcov_branch_coverage=1 --list build_cov/coverage.info - name: Check Coverage uses: FreeRTOS/CI-CD-Github-Actions/coverage-cop@main with: - path: ./build/coverage.info + path: ./build_cov/coverage.info complexity: runs-on: ubuntu-latest steps: diff --git a/docs/doxygen/include/size_table.html b/docs/doxygen/include/size_table.html index c9873814..c98617e2 100644 --- a/docs/doxygen/include/size_table.html +++ b/docs/doxygen/include/size_table.html @@ -9,8 +9,8 @@ core_http_client.c -
3.1K
-
2.5K
+
3.2K
+
2.6K
http_parser.c (third-party utility) @@ -19,7 +19,7 @@ Total estimates -
18.8K
 -
15.5K
 +
18.9K
 +
15.6K
 diff --git a/lexicon.txt b/lexicon.txt index 028e065e..624333ad 100644 --- a/lexicon.txt +++ b/lexicon.txt @@ -31,6 +31,7 @@ const contentlength convertint copybrief +copycharsuntilnull copydoc corehttp coverity @@ -70,7 +71,7 @@ html http httpclient httpheadernotfound -httpheaderstrncpy +httpheadercpy httpinsufficientmemory httpinvalidparameter httpinvalidresponse @@ -128,6 +129,7 @@ loginfo logwarn mainpage malloc +maxlen memcpy memmove methodlen diff --git a/source/core_http_client.c b/source/core_http_client.c index a3ba100a..4bd23f72 100644 --- a/source/core_http_client.c +++ b/source/core_http_client.c @@ -110,23 +110,37 @@ static HTTPStatus_t sendHttpBody( const TransportInterface_t * pTransport, size_t reqBodyBufLen ); /** - * @brief A strncpy replacement with HTTP header validation. + * @brief A strncpy replacement which works with char pointers and does not add + * a NULL terminator to the output buffer. + * + * @param[in] pDest The destination buffer to copy to. + * @param[in] pSrc The source buffer to copy from. + * @param[in] maxLen The maximum number of chars to copy from pSrc to @p pDest + * + * @return the number of chars written to @p pDest + */ +static size_t copyCharsUntilNull( char * pDest, + const char * pSrc, + size_t maxLen ); + +/** + * @brief A memcpy replacement with HTTP header validation. * * This function checks for `\r` and `\n` in the @p pSrc while copying. * This function checks for `:` in the @p pSrc, if @p isField is set to 1. * * @param[in] pDest The destination buffer to copy to. * @param[in] pSrc The source buffer to copy from. - * @param[in] len The length of @p pSrc to copy to pDest. + * @param[in] len The maximum number of character to copy from pSrc to @p pDest * @param[in] isField Set to 0 to indicate that @p pSrc is a field. Set to 1 to * indicate that @p pSrc is a value. * - * @return @p pDest if the copy was successful, NULL otherwise. + * @return the number of chars written to @p pDest */ -static char * httpHeaderStrncpy( char * pDest, - const char * pSrc, - size_t len, - uint8_t isField ); +static size_t httpHeaderCpy( char * pDest, + const char * pSrc, + size_t len, + uint8_t isField ); /** * @brief Write header based on parameters. This method also adds a trailing @@ -221,7 +235,7 @@ static HTTPStatus_t sendHttpRequest( const TransportInterface_t * pTransport, uint32_t sendFlags ); /** - * @brief Converts an integer value to its ASCII representation in the passed + * @brief Converts an integer value to its ASCII representation in the given * buffer. * * @param[in] value The value to convert to ASCII. @@ -638,7 +652,7 @@ static int httpParserOnStatusCallback( http_parser * pHttpParser, assert( pResponse != NULL ); /* Set the location of what to parse next. */ - pParsingContext->pBufferCur = pLoc + length; + pParsingContext->pBufferCur = &pLoc[ length ]; /* Initialize the first header field and value to be passed to the user * callback. */ @@ -686,7 +700,7 @@ static int httpParserOnHeaderFieldCallback( http_parser * pHttpParser, } /* Set the location of what to parse next. */ - pParsingContext->pBufferCur = pLoc + length; + pParsingContext->pBufferCur = &pLoc[ length ]; /* The httpParserOnHeaderFieldCallback() always follows the * httpParserOnHeaderValueCallback() if there is another header field. When @@ -705,7 +719,7 @@ static int httpParserOnHeaderFieldCallback( http_parser * pHttpParser, } else { - assert( pParsingContext->lastHeaderFieldLen <= SIZE_MAX - length ); + assert( pParsingContext->lastHeaderFieldLen <= ( SIZE_MAX - length ) ); pParsingContext->lastHeaderFieldLen += length; } @@ -732,7 +746,7 @@ static int httpParserOnHeaderValueCallback( http_parser * pHttpParser, pParsingContext = ( HTTPParsingContext_t * ) ( pHttpParser->data ); /* Set the location of what to parse next. */ - pParsingContext->pBufferCur = pLoc + length; + pParsingContext->pBufferCur = &pLoc[ length ]; /* If httpParserOnHeaderValueCallback() is invoked in succession, then the * last time http_parser_execute() was called only part of the header field @@ -920,7 +934,7 @@ static int httpParserOnBodyCallback( http_parser * pHttpParser, pResponse->bodyLen += length; /* Set the next location of parsing. */ - pParsingContext->pBufferCur = pLoc + length; + pParsingContext->pBufferCur = &pLoc[ length ]; LogDebug( ( "Response parsing: Found the response body: " "BodyLength=%lu", @@ -1170,7 +1184,7 @@ static HTTPStatus_t parseHttpResponse( HTTPParsingContext_t * pParsingContext, /* The next location to parse will always be after what has already * been parsed. */ - pParsingContext->pBufferCur = parsingStartLoc + bytesParsed; + pParsingContext->pBufferCur = &parsingStartLoc[ bytesParsed ]; LogDebug( ( "Parsed HTTP Response buffer: BytesParsed=%lu, " "ExpectedBytesParsed=%lu", @@ -1237,21 +1251,52 @@ static uint8_t convertInt32ToAscii( int32_t value, /*-----------------------------------------------------------*/ -static char * httpHeaderStrncpy( char * pDest, - const char * pSrc, - size_t len, - uint8_t isField ) +static size_t copyCharsUntilNull( char * pDest, + const char * pSrc, + size_t maxLen ) { size_t i = 0U; - char * pRet = pDest; + size_t charsWritten = 0U; + + assert( pDest != NULL ); + assert( pSrc != NULL ); + + for( i = 0U; i < maxLen; i++ ) + { + if( pSrc[ i ] == NULL_CHARACTER ) + { + break; + } + + pDest[ i ] = pSrc[ i ]; + charsWritten++; + } + + return charsWritten; +} + +/*-----------------------------------------------------------*/ + +static size_t httpHeaderCpy( char * pDest, + const char * pSrc, + size_t len, + uint8_t isField ) +{ + size_t i = 0U; + size_t charsWritten = 0U; uint8_t hasError = 0U; + uint8_t endOfInput = 0U; assert( pDest != NULL ); assert( pSrc != NULL ); - for( ; i < len; i++ ) + for( i = 0; i < len; i++ ) { - if( pSrc[ i ] == CARRIAGE_RETURN_CHARACTER ) + if( pSrc[ i ] == NULL_CHARACTER ) + { + endOfInput = 1U; + } + else if( pSrc[ i ] == CARRIAGE_RETURN_CHARACTER ) { LogError( ( "Invalid character '\r' found in %.*s", ( int ) len, pSrc ) ); @@ -1263,7 +1308,7 @@ static char * httpHeaderStrncpy( char * pDest, ( int ) len, pSrc ) ); hasError = 1U; } - else if( ( isField == 1U ) && ( pSrc[ i ] == COLON_CHARACTER ) ) + else if( ( pSrc[ i ] == COLON_CHARACTER ) && ( isField == 1U ) ) { LogError( ( "Invalid character ':' found in %.*s", ( int ) len, pSrc ) ); @@ -1272,16 +1317,21 @@ static char * httpHeaderStrncpy( char * pDest, else { pDest[ i ] = pSrc[ i ]; + charsWritten++; } - if( hasError == 1U ) + if( ( endOfInput == 1U ) || ( hasError == 1U ) ) { - pRet = NULL; break; } } - return pRet; + if( hasError == 1U ) + { + charsWritten = 0U; + } + + return charsWritten; } /*-----------------------------------------------------------*/ @@ -1293,9 +1343,9 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, size_t valueLen ) { HTTPStatus_t returnStatus = HTTPSuccess; - char * pBufferCur = NULL; - size_t toAddLen = 0U; - size_t backtrackHeaderLen = 0U; + char * pBufferStart = NULL; + ptrdiff_t toAddLen = 0U; + size_t bufferBacktrackLen = 0; assert( pRequestHeaders != NULL ); assert( pRequestHeaders->pBuffer != NULL ); @@ -1304,66 +1354,72 @@ static HTTPStatus_t addHeader( HTTPRequestHeaders_t * pRequestHeaders, assert( fieldLen != 0U ); assert( valueLen != 0U ); - pBufferCur = ( char * ) ( pRequestHeaders->pBuffer + pRequestHeaders->headersLen ); - backtrackHeaderLen = pRequestHeaders->headersLen; - /* Backtrack before trailing "\r\n" (HTTP header end) if it's already written. * Note that this method also writes trailing "\r\n" before returning. * The first condition prevents reading before start of the header. */ - if( ( HTTP_HEADER_END_INDICATOR_LEN <= pRequestHeaders->headersLen ) && - ( strncmp( ( char * ) pBufferCur - HTTP_HEADER_END_INDICATOR_LEN, - HTTP_HEADER_END_INDICATOR, HTTP_HEADER_END_INDICATOR_LEN ) == 0 ) ) + if( ( pRequestHeaders->headersLen >= HTTP_HEADER_END_INDICATOR_LEN ) && + ( strncmp( ( char * ) &pRequestHeaders->pBuffer[ pRequestHeaders->headersLen - HTTP_HEADER_END_INDICATOR_LEN ], + HTTP_HEADER_END_INDICATOR, + HTTP_HEADER_END_INDICATOR_LEN ) == 0 ) ) { - backtrackHeaderLen -= HTTP_HEADER_LINE_SEPARATOR_LEN; - pBufferCur -= HTTP_HEADER_LINE_SEPARATOR_LEN; + /* Decrease the headersLen field */ + pBufferStart = ( char * ) &pRequestHeaders->pBuffer[ pRequestHeaders->headersLen - HTTP_HEADER_LINE_SEPARATOR_LEN ]; + bufferBacktrackLen = HTTP_HEADER_LINE_SEPARATOR_LEN; + } + else + { + pBufferStart = ( char * ) &pRequestHeaders->pBuffer[ pRequestHeaders->headersLen ]; } - /* Check if there is enough space in buffer for additional header. */ - toAddLen = fieldLen + HTTP_HEADER_FIELD_SEPARATOR_LEN + valueLen + + /* Check if there is enough space in buffer for the additional header. */ + toAddLen = ( ptrdiff_t ) fieldLen + + HTTP_HEADER_FIELD_SEPARATOR_LEN + + ( ptrdiff_t ) valueLen + HTTP_HEADER_LINE_SEPARATOR_LEN + HTTP_HEADER_LINE_SEPARATOR_LEN; - /* If we have enough room for the new header line, then write it to the - * header buffer. */ - if( ( backtrackHeaderLen + toAddLen ) <= pRequestHeaders->bufferLen ) + assert(sizeof(ptrdiff_t) == 64 ); + assert( toAddLen > fieldLen ); + assert( toAddLen > valueLen ); + assert( toAddLen > ( HTTP_HEADER_LINE_SEPARATOR_LEN + HTTP_HEADER_LINE_SEPARATOR_LEN + HTTP_HEADER_LINE_SEPARATOR_LEN ) ); + + /* If we have enough room for the additional header, then write it to the + * buffer. */ + if( ( toAddLen + pRequestHeaders->headersLen - bufferBacktrackLen ) <= pRequestHeaders->bufferLen ) { + ptrdiff_t outBytesWritten = 0U; /* Write ": \r\n" to the headers buffer. */ /* Copy the header name into the buffer. */ - if( httpHeaderStrncpy( pBufferCur, pField, fieldLen, HTTP_HEADER_STRNCPY_IS_FIELD ) == NULL ) - { - returnStatus = HTTPSecurityAlertInvalidCharacter; - } - - if( returnStatus == HTTPSuccess ) + outBytesWritten += (ptrdiff_t) httpHeaderCpy( &pBufferStart[ outBytesWritten ], + pField, + fieldLen, + HTTP_HEADER_STRNCPY_IS_FIELD ); + + /* Copy the field separator, ": ", into the buffer. */ + outBytesWritten += (ptrdiff_t) copyCharsUntilNull( &pBufferStart[ outBytesWritten ], + HTTP_HEADER_FIELD_SEPARATOR, + HTTP_HEADER_FIELD_SEPARATOR_LEN ); + + /* Copy the header value into the buffer. */ + outBytesWritten += (ptrdiff_t) httpHeaderCpy( &pBufferStart[ outBytesWritten ], + pValue, + valueLen, + HTTP_HEADER_STRNCPY_IS_VALUE ); + + /* Copy the header end indicator, "\r\n\r\n" into the buffer. */ + outBytesWritten += (ptrdiff_t) copyCharsUntilNull( &pBufferStart[ outBytesWritten ], + HTTP_HEADER_END_INDICATOR, + HTTP_HEADER_END_INDICATOR_LEN ); + + if( outBytesWritten == toAddLen ) { - pBufferCur += fieldLen; - - /* Copy the field separator, ": ", into the buffer. */ - ( void ) strncpy( pBufferCur, - HTTP_HEADER_FIELD_SEPARATOR, - HTTP_HEADER_FIELD_SEPARATOR_LEN ); - - pBufferCur += HTTP_HEADER_FIELD_SEPARATOR_LEN; - - /* Copy the header value into the buffer. */ - if( httpHeaderStrncpy( pBufferCur, pValue, valueLen, HTTP_HEADER_STRNCPY_IS_VALUE ) == NULL ) - { - returnStatus = HTTPSecurityAlertInvalidCharacter; - } + /* Update the headers length value only when everything is successful. */ + pRequestHeaders->headersLen += ( outBytesWritten - bufferBacktrackLen ); } - - if( returnStatus == HTTPSuccess ) + else { - pBufferCur += valueLen; - - /* Copy the header end indicator, "\r\n\r\n" into the buffer. */ - ( void ) strncpy( pBufferCur, - HTTP_HEADER_END_INDICATOR, - HTTP_HEADER_END_INDICATOR_LEN ); - - /* Update the headers length value only when everything is successful. */ - pRequestHeaders->headersLen = backtrackHeaderLen + toAddLen; + returnStatus = HTTPSecurityAlertInvalidCharacter; } } else @@ -1387,7 +1443,7 @@ static HTTPStatus_t addRangeHeader( HTTPRequestHeaders_t * pRequestHeaders, { HTTPStatus_t returnStatus = HTTPSuccess; char rangeValueBuffer[ HTTP_MAX_RANGE_REQUEST_VALUE_LEN ]; - size_t rangeValueLength = 0U; + ptrdiff_t rangeValueLength = 0U; assert( pRequestHeaders != NULL ); @@ -1399,35 +1455,42 @@ static HTTPStatus_t addRangeHeader( HTTPRequestHeaders_t * pRequestHeaders, /* Generate the value data for the Range Request header.*/ /* Write the range value prefix in the buffer. */ - ( void ) strncpy( rangeValueBuffer, - HTTP_RANGE_REQUEST_HEADER_VALUE_PREFIX, - HTTP_RANGE_REQUEST_HEADER_VALUE_PREFIX_LEN ); - rangeValueLength += HTTP_RANGE_REQUEST_HEADER_VALUE_PREFIX_LEN; + rangeValueLength += copyCharsUntilNull( rangeValueBuffer, + HTTP_RANGE_REQUEST_HEADER_VALUE_PREFIX, + HTTP_RANGE_REQUEST_HEADER_VALUE_PREFIX_LEN ); + + assert( rangeValueLength <= HTTP_MAX_RANGE_REQUEST_VALUE_LEN ); /* Write the range start value in the buffer. */ rangeValueLength += convertInt32ToAscii( rangeStartOrlastNbytes, - rangeValueBuffer + rangeValueLength, + &rangeValueBuffer[ rangeValueLength ], sizeof( rangeValueBuffer ) - rangeValueLength ); + assert( rangeValueLength <= HTTP_MAX_RANGE_REQUEST_VALUE_LEN ); + /* Add remaining value data depending on the range specification type. */ /* Add rangeEnd value if request is for [rangeStart, rangeEnd] byte range */ if( rangeEnd != HTTP_RANGE_REQUEST_END_OF_FILE ) { /* Write the "-" character to the buffer.*/ - *( rangeValueBuffer + rangeValueLength ) = DASH_CHARACTER; + rangeValueBuffer[ rangeValueLength ] = DASH_CHARACTER; rangeValueLength += DASH_CHARACTER_LEN; + assert( rangeValueLength <= HTTP_MAX_RANGE_REQUEST_VALUE_LEN ); + /* Write the rangeEnd value of the request range to the buffer. */ rangeValueLength += convertInt32ToAscii( rangeEnd, - rangeValueBuffer + rangeValueLength, + &rangeValueBuffer[ rangeValueLength ], sizeof( rangeValueBuffer ) - rangeValueLength ); + + assert( rangeValueLength <= HTTP_MAX_RANGE_REQUEST_VALUE_LEN ); } /* Case when request is for bytes in the range [rangeStart, EoF). */ else if( rangeStartOrlastNbytes >= 0 ) { /* Write the "-" character to the buffer.*/ - *( rangeValueBuffer + rangeValueLength ) = DASH_CHARACTER; + rangeValueBuffer[ rangeValueLength ] = DASH_CHARACTER; rangeValueLength += DASH_CHARACTER_LEN; } else @@ -1455,17 +1518,18 @@ static HTTPStatus_t writeRequestLine( HTTPRequestHeaders_t * pRequestHeaders, { HTTPStatus_t returnStatus = HTTPSuccess; char * pBufferCur = NULL; - size_t toAddLen = 0U; + ptrdiff_t toAddLen = 0U; + ptrdiff_t outBytesWritten = 0U; assert( pRequestHeaders != NULL ); assert( pRequestHeaders->pBuffer != NULL ); assert( pMethod != NULL ); assert( methodLen != 0U ); - toAddLen = methodLen + \ - SPACE_CHARACTER_LEN + \ - SPACE_CHARACTER_LEN + \ - HTTP_PROTOCOL_VERSION_LEN + \ + toAddLen = ( ptrdiff_t ) methodLen + + SPACE_CHARACTER_LEN + + SPACE_CHARACTER_LEN + + HTTP_PROTOCOL_VERSION_LEN + HTTP_HEADER_LINE_SEPARATOR_LEN; pBufferCur = ( char * ) ( pRequestHeaders->pBuffer ); @@ -1479,38 +1543,43 @@ static HTTPStatus_t writeRequestLine( HTTPRequestHeaders_t * pRequestHeaders, if( returnStatus == HTTPSuccess ) { /* Write " HTTP/1.1\r\n" to start the HTTP header. */ - ( void ) strncpy( pBufferCur, pMethod, methodLen ); - pBufferCur += methodLen; - *pBufferCur = SPACE_CHARACTER; - pBufferCur += SPACE_CHARACTER_LEN; + outBytesWritten += copyCharsUntilNull( &pBufferCur[ outBytesWritten ], pMethod, methodLen ); + + pBufferCur[ outBytesWritten ] = SPACE_CHARACTER; + outBytesWritten += SPACE_CHARACTER_LEN; /* Use "/" as default value if is NULL. */ if( ( pPath == NULL ) || ( pathLen == 0U ) ) { - ( void ) strncpy( pBufferCur, - HTTP_EMPTY_PATH, - HTTP_EMPTY_PATH_LEN ); - pBufferCur += HTTP_EMPTY_PATH_LEN; + outBytesWritten += copyCharsUntilNull( &pBufferCur[ outBytesWritten ], + HTTP_EMPTY_PATH, + HTTP_EMPTY_PATH_LEN ); } else { - ( void ) strncpy( pBufferCur, pPath, pathLen ); - pBufferCur += pathLen; + outBytesWritten += copyCharsUntilNull( &pBufferCur[ outBytesWritten ], pPath, pathLen ); } - *pBufferCur = SPACE_CHARACTER; - pBufferCur += SPACE_CHARACTER_LEN; + pBufferCur[ outBytesWritten ] = SPACE_CHARACTER; + outBytesWritten += SPACE_CHARACTER_LEN; + + outBytesWritten += copyCharsUntilNull( &pBufferCur[ outBytesWritten ], + HTTP_PROTOCOL_VERSION, + HTTP_PROTOCOL_VERSION_LEN ); - ( void ) strncpy( pBufferCur, - HTTP_PROTOCOL_VERSION, - HTTP_PROTOCOL_VERSION_LEN ); - pBufferCur += HTTP_PROTOCOL_VERSION_LEN; + outBytesWritten += copyCharsUntilNull( &pBufferCur[ outBytesWritten ], + HTTP_HEADER_LINE_SEPARATOR, + HTTP_HEADER_LINE_SEPARATOR_LEN ); - ( void ) strncpy( pBufferCur, - HTTP_HEADER_LINE_SEPARATOR, - HTTP_HEADER_LINE_SEPARATOR_LEN ); - pRequestHeaders->headersLen = toAddLen; + if( outBytesWritten == toAddLen ) + { + pRequestHeaders->headersLen = outBytesWritten; + } + else + { + returnStatus = HTTPInvalidParameter; + } } return returnStatus; @@ -1746,10 +1815,10 @@ static HTTPStatus_t sendHttpData( const TransportInterface_t * pTransport, size_t dataLen ) { HTTPStatus_t returnStatus = HTTPSuccess; - const uint8_t * pIndex = pData; + size_t dataIndex = 0U; int32_t bytesSent = 0; - size_t bytesRemaining = dataLen; - uint32_t lastSendTimeMs = 0U, timeSinceLastSendMs = 0U; + uint32_t lastSendTimeMs = 0U; + uint32_t timeSinceLastSendMs = 0U; uint32_t retryTimeoutMs = HTTP_SEND_RETRY_TIMEOUT_MS; assert( pTransport != NULL ); @@ -1768,11 +1837,11 @@ static HTTPStatus_t sendHttpData( const TransportInterface_t * pTransport, lastSendTimeMs = getTimestampMs(); /* Loop until all data is sent. */ - while( ( bytesRemaining > 0UL ) && ( returnStatus != HTTPNetworkError ) ) + while( ( ( dataLen - dataIndex ) > 0UL ) && ( returnStatus != HTTPNetworkError ) ) { bytesSent = pTransport->send( pTransport->pNetworkContext, - pIndex, - bytesRemaining ); + &pData[ dataIndex ], + ( dataLen - dataIndex ) ); /* BytesSent less than zero is an error. */ if( bytesSent < 0 ) @@ -1785,20 +1854,20 @@ static HTTPStatus_t sendHttpData( const TransportInterface_t * pTransport, { /* It is a bug in the application's transport send implementation if * more bytes than expected are sent. To avoid a possible overflow - * in converting bytesRemaining from unsigned to signed, this assert - * must exist after the check for bytesSent being negative. */ - assert( ( size_t ) bytesSent <= bytesRemaining ); + * in converting the third argument from unsigned to signed, this + * assert must exist after the check for bytesSent overflowing when + * cast to a size_t. */ + assert( ( size_t ) bytesSent <= ( dataLen - dataIndex ) ); /* Record the most recent time of successful transmission. */ lastSendTimeMs = getTimestampMs(); - bytesRemaining -= ( size_t ) bytesSent; - pIndex += bytesSent; + dataIndex += ( size_t ) bytesSent; LogDebug( ( "Sent data over the transport: " "BytesSent=%ld, BytesRemaining=%lu, TotalBytesSent=%lu", ( long int ) bytesSent, - ( unsigned long ) bytesRemaining, - ( unsigned long ) ( dataLen - bytesRemaining ) ) ); + ( unsigned long ) ( dataLen - dataIndex ), + ( unsigned long ) dataIndex ) ); } else { @@ -1917,8 +1986,8 @@ static HTTPStatus_t getFinalResponseStatus( HTTPParsingState_t parsingState, { HTTPStatus_t returnStatus = HTTPSuccess; - assert( parsingState >= HTTP_PARSING_NONE && - parsingState <= HTTP_PARSING_COMPLETE ); + assert( ( parsingState >= HTTP_PARSING_NONE ) && + ( parsingState <= HTTP_PARSING_COMPLETE ) ); assert( totalReceived <= responseBufferLen ); /* If no parsing occurred, that means network data was never received. */ @@ -1966,8 +2035,11 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT size_t totalReceived = 0U; int32_t currentReceived = 0; HTTPParsingContext_t parsingContext = { 0 }; - uint8_t shouldRecv = 1U, shouldParse = 1U, timeoutReached = 0U; - uint32_t lastRecvTimeMs = 0U, timeSinceLastRecvMs = 0U; + uint8_t shouldRecv = 1U; + uint8_t shouldParse = 1U; + uint8_t timeoutReached = 0U; + uint32_t lastRecvTimeMs = 0U; + uint32_t timeSinceLastRecvMs = 0U; uint32_t retryTimeoutMs = HTTP_RECV_RETRY_TIMEOUT_MS; assert( pTransport != NULL ); @@ -2017,7 +2089,7 @@ static HTTPStatus_t receiveAndParseHttpResponse( const TransportInterface_t * pT * Because we cannot know how large the HTTP response will be in * total, parsing will tell us if the end of the message is reached.*/ shouldParse = 1U; - totalReceived += currentReceived; + totalReceived += ( size_t ) currentReceived; } else { @@ -2084,7 +2156,7 @@ static HTTPStatus_t sendHttpRequest( const TransportInterface_t * pTransport, assert( pTransport != NULL ); assert( pRequestHeaders != NULL ); assert( ( pRequestBodyBuf != NULL ) || - ( ( pRequestBodyBuf == NULL ) && ( reqBodyBufLen == 0 ) ) ); + ( ( pRequestBodyBuf == NULL ) && ( reqBodyBufLen == 0U ) ) ); assert( getTimestampMs != NULL ); /* Send the headers, which are at one location in memory. */ diff --git a/source/include/core_http_client_private.h b/source/include/core_http_client_private.h index d87a7ddf..a3340304 100644 --- a/source/include/core_http_client_private.h +++ b/source/include/core_http_client_private.h @@ -59,15 +59,16 @@ #define CARRIAGE_RETURN_CHARACTER '\r' /**< A carriage return character to help with header validation. */ #define LINEFEED_CHARACTER '\n' /**< A linefeed character to help with header validation. */ #define COLON_CHARACTER ':' /**< A colon character to help with header validation. */ +#define NULL_CHARACTER '\0' /**< A NULL character to help with header validation. */ /** - * @brief Indicator for function #httpHeaderStrncpy that the pSrc parameter is a + * @brief Indicator for function #httpHeaderCpy that the pSrc parameter is a * header value. */ #define HTTP_HEADER_STRNCPY_IS_VALUE 0U /** - * @brief Indicator for function #httpHeaderStrncpy that the pSrc parameter is a + * @brief Indicator for function #httpHeaderCpy that the pSrc parameter is a * header field. */ #define HTTP_HEADER_STRNCPY_IS_FIELD 1U diff --git a/test/cbmc/proofs/HTTPClient_AddHeader/Makefile b/test/cbmc/proofs/HTTPClient_AddHeader/Makefile index 7394ec1f..6a16665f 100644 --- a/test/cbmc/proofs/HTTPClient_AddHeader/Makefile +++ b/test/cbmc/proofs/HTTPClient_AddHeader/Makefile @@ -44,8 +44,8 @@ REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_httpParserOnMess # This decreases the run-time of the proof. This is safe to do for this proof # because HTTPClient_AddHeader does not use the results of the copy later in the # function. -REMOVE_FUNCTION_BODY += strncpy -REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_httpHeaderStrncpy +REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_copyCharsUntilNull +REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_httpHeaderCpy # strncmp is used to find if there exists "\r\n\r\n" at the end of the header # buffer. Therefore, we need to unwind strncmp the length of "\r\n\r\n" + 1. @@ -53,8 +53,8 @@ UNWINDSET += strncmp.0:5 PROOF_SOURCES += $(PROOFDIR)/$(HARNESS_FILE).c PROOF_SOURCES += $(SRCDIR)/test/cbmc/sources/http_cbmc_state.c -PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/strncpy.c -PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/httpHeaderStrncpy.c +PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/copyCharsUntilNull.c +PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/httpHeaderCpy.c PROJECT_SOURCES += $(SRCDIR)/source/core_http_client.c diff --git a/test/cbmc/proofs/HTTPClient_AddRangeHeader/Makefile b/test/cbmc/proofs/HTTPClient_AddRangeHeader/Makefile index 71fcf7c5..952465fb 100644 --- a/test/cbmc/proofs/HTTPClient_AddRangeHeader/Makefile +++ b/test/cbmc/proofs/HTTPClient_AddRangeHeader/Makefile @@ -31,8 +31,8 @@ INCLUDES += # This decreases the run-time of the proof. This is safe to do for this proof # because HTTPClient_AddRangeHeader does not use the results of the copy later # in the function. -REMOVE_FUNCTION_BODY += strncpy -REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_httpHeaderStrncpy +REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_copyCharsUntilNull +REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_httpHeaderCpy # strncmp is used to find if there exists "\r\n\r\n" at the end of the header # buffer. Therefore, we need to unwind strncmp the length of "\r\n\r\n" + 1. @@ -43,8 +43,8 @@ UNWINDSET += __CPROVER_file_local_core_http_client_c_convertInt32ToAscii.1:11 PROOF_SOURCES += $(PROOFDIR)/$(HARNESS_FILE).c PROOF_SOURCES += $(SRCDIR)/test/cbmc/sources/http_cbmc_state.c -PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/strncpy.c -PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/httpHeaderStrncpy.c +PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/copyCharsUntilNull.c +PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/httpHeaderCpy.c PROJECT_SOURCES += $(SRCDIR)/source/core_http_client.c diff --git a/test/cbmc/proofs/HTTPClient_InitializeRequestHeaders/Makefile b/test/cbmc/proofs/HTTPClient_InitializeRequestHeaders/Makefile index e8c8208d..7520cac2 100644 --- a/test/cbmc/proofs/HTTPClient_InitializeRequestHeaders/Makefile +++ b/test/cbmc/proofs/HTTPClient_InitializeRequestHeaders/Makefile @@ -43,8 +43,8 @@ REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_httpParserOnMess # This decreases the run-time of the proof. This is safe to do for this proof # because HTTPClient_InitializeRequestHeaders does not use the results of the # copy later in the function. -REMOVE_FUNCTION_BODY += strncpy -REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_httpHeaderStrncpy +REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_copyCharsUntilNull +REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_httpHeaderCpy # strncmp is used to find if there exists "\r\n\r\n" at the end of the header # buffer. Therefore, we need to unwind strncmp the length of "\r\n\r\n" + 1. @@ -52,8 +52,8 @@ UNWINDSET += strncmp.0:5 PROOF_SOURCES += $(PROOFDIR)/$(HARNESS_FILE).c PROOF_SOURCES += $(SRCDIR)/test/cbmc/sources/http_cbmc_state.c -PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/strncpy.c -PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/httpHeaderStrncpy.c +PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/copyCharsUntilNull.c +PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/httpHeaderCpy.c PROJECT_SOURCES += $(SRCDIR)/source/core_http_client.c diff --git a/test/cbmc/proofs/HTTPClient_Send/Makefile b/test/cbmc/proofs/HTTPClient_Send/Makefile index fdeb4daf..b72f4e8a 100644 --- a/test/cbmc/proofs/HTTPClient_Send/Makefile +++ b/test/cbmc/proofs/HTTPClient_Send/Makefile @@ -51,8 +51,8 @@ REMOVE_FUNCTION_BODY += http_parser_settings_init # This decreases the run-time of the proof. This is safe to do for this proof # because all of the functions, in HTTPClient_Send, that would have used the # results of the copy are stubbed out to be proven separately. -REMOVE_FUNCTION_BODY += strncpy -REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_httpHeaderStrncpy +REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_copyCharsUntilNull +REMOVE_FUNCTION_BODY += __CPROVER_file_local_core_http_client_c_httpHeaderCpy # There is a total of 10 digits in INT32_MAX. These loops are unwound once more # than the total possible iterations in the int32_t to ASCII converation. @@ -70,8 +70,8 @@ PROOF_SOURCES += $(SRCDIR)/test/cbmc/sources/http_cbmc_state.c PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/HTTPClient_Send_http_parser_execute.c PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/transport_interface_stubs.c PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/get_time_stub.c -PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/strncpy.c -PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/httpHeaderStrncpy.c +PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/copyCharsUntilNull.c +PROOF_SOURCES += $(SRCDIR)/test/cbmc/stubs/httpHeaderCpy.c PROJECT_SOURCES += $(SRCDIR)/source/core_http_client.c diff --git a/test/cbmc/stubs/strncpy.c b/test/cbmc/stubs/copyCharsUntilNull.c similarity index 51% rename from test/cbmc/stubs/strncpy.c rename to test/cbmc/stubs/copyCharsUntilNull.c index 353a51ab..413bcada 100644 --- a/test/cbmc/stubs/strncpy.c +++ b/test/cbmc/stubs/copyCharsUntilNull.c @@ -21,37 +21,22 @@ */ /** - * @file strncpy.c - * @brief Creates a stub for strncpy so that the proofs for HTTPClient_AddHeader, - * HTTPClient_AddRangeHeader, and HTTPClient_InitializeRequestHeaders, run much - * faster. This stub checks if, for the input copy length, the destination and - * source are valid accessible memory. + * @file copyCharsUntilNull.c + * @brief Creates a stub for copyCharsUntilNull so that the proofs for + * HTTPClient_AddHeader, HTTPClient_AddRangeHeader, and + * HTTPClient_InitializeRequestHeaders run much faster. This stub checks if, for + * the input copy length, the destination and source are valid accessible + * memory. */ #include +#include -/* This is a clang macro not available on linux */ -#ifndef __has_builtin - #define __has_builtin( x ) 0 -#endif - -#if __has_builtin( __builtin___strncpy_chk ) - void * __builtin___strncpy_chk( void * dest, - const void * src, - size_t n, - size_t os ) - { - __CPROVER_assert( __CPROVER_w_ok( dest, n ), "write" ); - __CPROVER_assert( __CPROVER_r_ok( src, n ), "read" ); - return dest; - } -#else - void * strncpy( void * dest, - const void * src, - size_t n ) - { - __CPROVER_assert( __CPROVER_w_ok( dest, n ), "write" ); - __CPROVER_assert( __CPROVER_r_ok( src, n ), "read" ); - return dest; - } -#endif /* if __has_builtin( __builtin___strncpy_chk ) */ +size_t copyCharsUntilNull( char * pDest, + const char * pSrc, + size_t maxLen ) +{ + __CPROVER_assert( __CPROVER_w_ok( pDest, maxLen ), "write" ); + __CPROVER_assert( __CPROVER_r_ok( pSrc, maxLen ), "read" ); + return maxLen; +} diff --git a/test/cbmc/stubs/httpHeaderStrncpy.c b/test/cbmc/stubs/httpHeaderCpy.c similarity index 84% rename from test/cbmc/stubs/httpHeaderStrncpy.c rename to test/cbmc/stubs/httpHeaderCpy.c index ce6aa796..0ceafac3 100644 --- a/test/cbmc/stubs/httpHeaderStrncpy.c +++ b/test/cbmc/stubs/httpHeaderCpy.c @@ -21,8 +21,8 @@ */ /** - * @file httpHeaderStrncpy.c - * @brief Creates a stub for httpHeaderStrncpy so that the proofs for + * @file httpHeaderCpy.c + * @brief Creates a stub for httpHeaderCpy so that the proofs for * HTTPClient_AddHeader, HTTPClient_AddRangeHeader, and * HTTPClient_InitializeRequestHeaders run much faster. This stub checks if, for * the input copy length, the destination and source are valid accessible @@ -32,12 +32,12 @@ #include #include -void * httpHeaderStrncpy( char * pDest, - const char * pSrc, - size_t len, - uint8_t isField ) +size_t httpHeaderCpy( char * pDest, + const char * pSrc, + size_t len, + uint8_t isField ) { __CPROVER_assert( __CPROVER_w_ok( pDest, len ), "write" ); __CPROVER_assert( __CPROVER_r_ok( pSrc, len ), "read" ); - return pDest; + return len; } diff --git a/test/unit-test/core_http_utest.c b/test/unit-test/core_http_utest.c index b3e8ff24..4a57096d 100644 --- a/test/unit-test/core_http_utest.c +++ b/test/unit-test/core_http_utest.c @@ -592,6 +592,33 @@ void test_Http_InitializeRequestHeaders_Insufficient_Memory() HTTP_TEST_REQUEST_LINE_LEN ) != 0 ); } +/** + * @brief Test HTTPInsufficientMemory with a path containing a null byte. + */ +void test_Http_InitializeRequestHeaders_NullByteInPath() +{ + HTTPStatus_t httpStatus = HTTPSuccess; + HTTPRequestHeaders_t requestHeaders = { 0 }; + HTTPRequestInfo_t requestInfo = { 0 }; + const char * const pathWithNullByte = "/AB\0CDEF/"; + + expectedHeaders.dataLen = HTTP_TEST_PREFIX_HEADER_LEN - + HTTP_TEST_REQUEST_PATH_LEN + + HTTP_EMPTY_PATH_LEN; + + setupRequestInfo( &requestInfo ); + setupBuffer( &requestHeaders ); + + requestInfo.pPath = pathWithNullByte; + requestInfo.pathLen = ( sizeof( pathWithNullByte ) - 1 ); + requestInfo.reqFlags = 0U; + + requestHeaders.pBuffer = testBuffer; + requestHeaders.bufferLen = expectedHeaders.dataLen; + httpStatus = HTTPClient_InitializeRequestHeaders( &requestHeaders, &requestInfo ); + TEST_ASSERT_EQUAL( HTTPInvalidParameter, httpStatus ); +} + /* ===================== Testing HTTPClient_AddHeader ======================= */ /** @@ -827,6 +854,7 @@ void test_Http_AddHeader_Invalid_Fields() const char * colonInField = "head:er-field"; const char * linefeedInField = "head\ner-field"; const char * carriageReturnInField = "head\rer-field"; + const char * nullInField = "head\0er-field"; setupBuffer( &requestHeaders ); @@ -851,6 +879,11 @@ void test_Http_AddHeader_Invalid_Fields() carriageReturnInField, strlen( carriageReturnInField ), HTTP_TEST_HEADER_VALUE, HTTP_TEST_HEADER_VALUE_LEN ); TEST_ASSERT_EQUAL( HTTPSecurityAlertInvalidCharacter, httpStatus ); + + httpStatus = HTTPClient_AddHeader( &requestHeaders, + nullInField, sizeof( nullInField ), + HTTP_TEST_HEADER_VALUE, HTTP_TEST_HEADER_VALUE_LEN ); + TEST_ASSERT_EQUAL( HTTPSecurityAlertInvalidCharacter, httpStatus ); } /** @@ -865,6 +898,7 @@ void test_Http_AddHeader_Invalid_Values() const char * colonInValue = "head:er-value"; const char * linefeedInValue = "head\ner-Value"; const char * carriageReturnInValue = "head\rer-Value"; + const char * nullInValue = "head\0er-Value"; setupBuffer( &requestHeaders ); @@ -909,6 +943,11 @@ void test_Http_AddHeader_Invalid_Values() HTTP_TEST_HEADER_FIELD, HTTP_TEST_HEADER_FIELD_LEN, carriageReturnInValue, strlen( carriageReturnInValue ) ); TEST_ASSERT_EQUAL( HTTPSecurityAlertInvalidCharacter, httpStatus ); + + httpStatus = HTTPClient_AddHeader( &requestHeaders, + HTTP_TEST_HEADER_FIELD, HTTP_TEST_HEADER_FIELD_LEN, + carriageReturnInValue, sizeof( nullInValue ) - 1 ); + TEST_ASSERT_EQUAL( HTTPSecurityAlertInvalidCharacter, httpStatus ); } /* ============== Testing HTTPClient_AddRangeHeader ================== */ @@ -996,6 +1035,7 @@ void test_Http_AddRangeHeader_Insufficient_Memory( void ) &expectedHeaders, PREEXISTING_HEADER_DATA ); size_t preHeadersLen = testHeaders.headersLen; + testRangeStart = 5; testRangeEnd = 10;