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

mbedtls_md_setup memory leak if allocation fails #3486

Closed
guidovranken opened this issue Jul 8, 2020 · 0 comments · Fixed by #3578
Closed

mbedtls_md_setup memory leak if allocation fails #3486

guidovranken opened this issue Jul 8, 2020 · 0 comments · Fixed by #3578
Assignees
Labels
bug component-crypto Crypto primitives and low-level interfaces

Comments

@guidovranken
Copy link
Contributor

Description

  • Type: Bug
  • Priority: Minor

Bug

The following code produces a memory leak. This happens because I've modified the allocator to return NULL on the second allocation request.

#include <mbedtls/md.h>
#include <mbedtls/platform.h>

#include <string.h>
#include <stdint.h>
#include <stdlib.h>
#include <stddef.h>

#define CF_CHECK_EQ(expr, res) if ( (expr) != (res) ) { goto end; }
#define CF_CHECK_NE(expr, res) if ( (expr) == (res) ) { goto end; }

static void* mbedTLS_custom_calloc(size_t A, size_t B) {
    static int i;
    i++;
    if ( i == 2 ) return NULL;
    const size_t size = A*B;
    void* p = malloc(size);
    if ( size ) {
        memset(p, 0x00, size);
    }
    return p;
}

static void mbedTLS_custom_free(void* ptr) {
    free(ptr);
}

int main(void)
{
    if ( mbedtls_platform_set_calloc_free(mbedTLS_custom_calloc, mbedTLS_custom_free) != 0 ) {
        abort();
    }

    mbedtls_md_info_t const* md_info = NULL;
    mbedtls_md_context_t md_ctx;

    mbedtls_md_init(&md_ctx);

    CF_CHECK_NE(md_info = mbedtls_md_info_from_type(MBEDTLS_MD_SHA512), NULL);
    CF_CHECK_EQ(mbedtls_md_setup(&md_ctx, md_info, 1), 0 );

end:
    mbedtls_md_free(&md_ctx);

    return 0;
}

Fix it by moving

https://github.com/ARMmbed/mbedtls/blob/3ee91f47f44d4133d3f155b113abfdf7bef98c4e/library/md.c#L471

to before line 461

@danh-arm danh-arm added the bug label Jul 20, 2020
@gilles-peskine-arm gilles-peskine-arm added this to the August 2020 Sprint milestone Aug 10, 2020
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Aug 19, 2020
mbedtls_md_setup() allocates a hash-specific context and then, if
requested, an extra HMAC context. If the second allocation failed, the
hash context was not freed.

Fix this by ensuring that the mbedtls_md_context_t object is always in
a consistent state, in particular, that the md_info field is always
set. For robustness, ensure that the object is in a consistent state
even on errors (other than BAD_INPUT_DATA if the object was not in a
consistent state on entry).

Fix Mbed-TLS#3486

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added component-crypto Crypto primitives and low-level interfaces fix available labels Aug 19, 2020
@mpg mpg closed this as completed in #3578 Aug 21, 2020
carlaageamundsen pushed a commit to carlaageamundsen/mbedtls that referenced this issue Aug 25, 2020
mbedtls_md_setup() allocates a hash-specific context and then, if
requested, an extra HMAC context. If the second allocation failed, the
hash context was not freed.

Fix this by ensuring that the mbedtls_md_context_t object is always in
a consistent state, in particular, that the md_info field is always
set. For robustness, ensure that the object is in a consistent state
even on errors (other than BAD_INPUT_DATA if the object was not in a
consistent state on entry).

Fix Mbed-TLS#3486

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Carl Aage Amundsen <Carl.Amundsen@silabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants