Missing cleanup in ECP multiplication if RNG fails

[tiod4420]

Description

• Type: Bug
• Priority: Minor

---

Bug

In the fileecp.c, functions ecp_randomize_jac and ecp_randomize_mxz have a retry counter to generate a random number 1 < l < p. If the retry counter exceeds the limit, the function returns directly, without going through cleanup.

It leads to a memory leak when the RNG function fails within the loop.

These functions are called from mbedtls_ecp_mul, so also affects ECDH and ECDSA computations.

Steps to reproduce

Sample program that simulates a failing RNG function during the call to the function. Same bug occurs with a Montgomery curve (group id MBEDTLS_ECP_DP_CURVE25519)

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/random.h>

#include <mbedtls/ecp.h>

typedef struct {
    size_t cur;
    size_t max;
} entropy_ctx;

static int good_rng(void *ctx, unsigned char *data, size_t data_sz);
static int bad_rng(void *ctx, unsigned char *data, size_t data_sz);

int main(int argc, const char *argv[])
{
    int ret = EXIT_FAILURE;
    int res;
    mbedtls_ecp_group grp;
    mbedtls_ecp_point R, P;
    mbedtls_mpi m;
    entropy_ctx bad_ctx;

    mbedtls_ecp_group_init(&grp);
    mbedtls_ecp_point_init(&R);
    mbedtls_ecp_point_init(&P);
    mbedtls_mpi_init(&m);

    res = mbedtls_ecp_group_load(&grp, MBEDTLS_ECP_DP_SECP256R1);
    if (0 != res)
        goto cleanup;

    res = mbedtls_ecp_gen_keypair(&grp, &m, &P, good_rng, NULL);
    if (0 != res)
        goto cleanup;

    bad_ctx.cur = 0;
    bad_ctx.max = 32;

    res = mbedtls_ecp_mul(&grp, &R, &m, &P, bad_rng, &bad_ctx);
    if (MBEDTLS_ERR_ECP_RANDOM_FAILED != res)
        goto cleanup;

    ret = EXIT_SUCCESS;
cleanup:
    mbedtls_ecp_group_free(&grp);
    mbedtls_ecp_point_free(&R);
    mbedtls_ecp_point_free(&P);
    mbedtls_mpi_free(&m);

    return ret;
}

int good_rng(void *ctx, unsigned char *data, size_t data_sz)
{
    ssize_t read_sz;
    ((void)ctx);

    read_sz = getrandom(data, data_sz, 0);
    return -1 == read_sz || read_sz < data_sz;
}

int bad_rng(void *ctx, unsigned char *data, size_t data_sz)
{
    ssize_t read_sz;
    entropy_ctx *bad_ctx = ctx;

    if ((bad_ctx->cur + data_sz) > bad_ctx->max)
        return 1;

    read_sz = getrandom(data, data_sz, 0);
    if (-1 == read_sz || read_sz < data_sz)
        return 1;

    bad_ctx->cur += data_sz;

    return 0;
}

Valgrind report for memory leak check:

$ valgrind --leak-check=full ./main
==66890== Memcheck, a memory error detector
==66890== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==66890== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==66890== Command: ./main
==66890==
==66890==
==66890== HEAP SUMMARY:
==66890==     in use at exit: 32 bytes in 1 blocks
==66890==   total heap usage: 17,066 allocs, 17,065 frees, 632,560 bytes allocated
==66890==
==66890== 32 bytes in 1 blocks are definitely lost in loss record 1 of 1
==66890==    at 0x4838B65: calloc (vg_replace_malloc.c:762)
==66890==    by 0x10954C: mbedtls_mpi_grow (in /home/user/mbedtls/build/main)
==66890==    by 0x10E011: mbedtls_mpi_fill_random (in /home/user/mbedtls/build/main)
==66890==    by 0x111BF1: ecp_randomize_jac (in /home/user/mbedtls/build/main)
==66890==    by 0x11273C: ecp_mul_comb_after_precomp (in /home/user/mbedtls/build/main)
==66890==    by 0x1129AD: ecp_mul_comb (in /home/user/mbedtls/build/main)
==66890==    by 0x11351A: mbedtls_ecp_mul_restartable (in /home/user/mbedtls/build/main)
==66890==    by 0x113582: mbedtls_ecp_mul (in /home/user/mbedtls/build/main)
==66890==    by 0x1092FE: main (in /home/user/mbedtls/build/main)
==66890==
==66890== LEAK SUMMARY:
==66890==    definitely lost: 32 bytes in 1 blocks
==66890==    indirectly lost: 0 bytes in 0 blocks
==66890==      possibly lost: 0 bytes in 0 blocks
==66890==    still reachable: 0 bytes in 0 blocks
==66890==         suppressed: 0 bytes in 0 blocks
==66890==
==66890== For lists of detected and suppressed errors, rerun with: -s
==66890== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

[gilles-peskine-arm]

Fixed by #3318 and its backports.