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.

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

Possible memory leak in ota_pal.c (CA-296) #185

Open
gonzzor opened this issue Jun 6, 2023 · 2 comments
Open

Possible memory leak in ota_pal.c (CA-296) #185

gonzzor opened this issue Jun 6, 2023 · 2 comments

Comments

@gonzzor
Copy link

gonzzor commented Jun 6, 2023

In the function otaPal_CheckFileSignature() there are two possible code paths that lead to memory leaks.

The call to CRYPTO_SignatureVerificationStart will allocate memory but it's not freed until CRYPTO_SignatureVerificationFinal() is called.

Between these two calls there are

Both these cases will cause a memory leak. Both code paths are rather unlikely but they still exist and shouldn't leak memory.

@github-actions github-actions bot changed the title Possible memory leak in ota_pal.c Possible memory leak in ota_pal.c (CA-296) Jun 6, 2023
@AntoineSX
Copy link

AntoineSX commented Oct 3, 2023

24c17ab is meant to fix this, but there's two new cases that will fail.

Both if the verification fails or succeeds here, it will free the memory block in CRYPTO_SignatureVerificationFinal and it will then try to free that memory block again causing the esp32 to crash.

if( CRYPTO_SignatureVerificationFinal( pvSigVerifyContext, ( char * ) pucSignerCert, ulSignerCertSize,
pFileContext->pSignature->data, pFileContext->pSignature->size ) == pdFALSE )
{
LogError( ( "Signature verification failed." ) );
result = OTA_PAL_COMBINE_ERR( OtaPalSignatureCheckFailed, 0 );
}
else
{
LogInfo( ( "Signature verification succeeded." ) );
result = OTA_PAL_COMBINE_ERR( OtaPalSuccess, 0 );
}

Line 472 and 477 should return.
@avsheth

EDIT: I assume the case where pvContext == NULL in CRYPTO_SignatureVerificationFinal is ok not to free the memory.

@avsheth
Copy link
Collaborator

avsheth commented Oct 3, 2023

Hi @AntoineSX
Yeah, guess I made a blunder :)
Thanks for bringing it to the notice. Will push the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants