mbedtls_ecp_muladd hangs with oversized point coordinates

[guidovranken]

Summary

mbedtls_ecp_muladd hangs with oversized point coordinates (>= curve order).

System information

Mbed TLS version (number or commit id): acc74b8
Operating system and version: Ubuntu Linux 64 bit
Configuration (if not default, please attach mbedtls_config.h):
Compiler and options (if you used a pre-built binary, please indicate how you obtained it):

export CC=clang
export CXX=clang++
git clone --depth 1 -b development_2.x https://github.com/ARMmbed/mbedtls.git
cd mbedtls/
scripts/config.pl set MBEDTLS_PLATFORM_MEMORY
scripts/config.pl set MBEDTLS_CMAC_C
scripts/config.pl set MBEDTLS_NIST_KW_C
scripts/config.pl set MBEDTLS_ARIA_C
scripts/config.pl set MBEDTLS_MD2_C
scripts/config.pl set MBEDTLS_MD4_C
mkdir build/
cd build/
cmake .. -DENABLE_PROGRAMS=0 -DENABLE_TESTING=0
make -j$(nproc)

Expected behavior

No hang

Actual behavior

Hang

Steps to reproduce

#include <mbedtls/ecp.h>

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

int main(void)
{
    mbedtls_ecp_group grp;
    mbedtls_ecp_point a, b, res;
    mbedtls_mpi scalar;
    const mbedtls_ecp_curve_info* curve_info = NULL;

    mbedtls_ecp_group_init(&grp);
    mbedtls_ecp_point_init(&a);
    mbedtls_ecp_point_init(&b);
    mbedtls_mpi_init(&scalar);
    mbedtls_ecp_point_init(&res);

    /* secp256r1 */
    CF_CHECK_NE(curve_info = mbedtls_ecp_curve_info_from_tls_id(23), NULL);

    CF_CHECK_EQ(mbedtls_ecp_group_load(&grp, curve_info->grp_id), 0);

    /* Load point a */
    CF_CHECK_EQ(mbedtls_mpi_read_string(&a.X, 10, "48439561293906451759052585252797914202762949526041747995844080717082404635286"), 0);
    CF_CHECK_EQ(mbedtls_mpi_read_string(&a.Y, 10, "36134250956749795798585127919587881956611106672985015071877198253568414405109"), 0);
    CF_CHECK_EQ(mbedtls_mpi_lset(&a.Z, 1), 0);

    /* Load point b */
    CF_CHECK_EQ(mbedtls_mpi_read_string(&b.X, 10, "330527984395124299475957654016385519914202341482140609642324395022880711289249191050673258457777458014096366590617731358670"), 0);
    CF_CHECK_EQ(mbedtls_mpi_read_string(&b.Y, 10, "109665173580781193289131306340629307257455236857523409490593279367549320823692"), 0);
    CF_CHECK_EQ(mbedtls_mpi_lset(&b.Z, 1), 0);

    CF_CHECK_EQ(mbedtls_mpi_lset(&scalar, 1), 0);

    /* res = a + b */
    /* hangs */
    CF_CHECK_EQ(mbedtls_ecp_muladd(&grp, &res, &scalar, &b, &scalar, &a), 0);
end:
    mbedtls_ecp_group_free(&grp);
    mbedtls_ecp_point_free(&a);
    mbedtls_ecp_point_free(&b);
    mbedtls_mpi_free(&scalar);
    mbedtls_ecp_point_free(&res);

    return 0;
}

Additional information

This happens when the point coordinates are >= curve order, which is the case in the reproducer for the b point.

This is not necessarily a bug but the documentation does not mandate passing only valid points to mbedtls_ecp_muladd. Moreover, mbedtls_ecp_muladd does not hang when given other invalid points (not on curve, infinity). Additionally, mbedtls_ecp_mul does not hang with oversized points.

[hanno-becker]

(Speaking for myself) @guidovranken I might have missed something, but directly accessing point coordinates is not supported by the public API, is it? See https://github.com/ARMmbed/mbedtls/blob/development/include/mbedtls/ecp.h#L174 I agree, though, that if the ECP module makes assumptions about the size of coordinates, those should be checked at the entry of top-level API calls (at negligible cost).

[gilles-peskine-arm]

@hanno-arm It was supported (since it wasn't explicitly forbidden) in Mbed TLS 2. And it may be officially supported again in some 3.x version: we haven't settled yet on what fields we're going to add accessors for.

[hanno-becker]

Actually, the public API mbedtls_ecp_point_read_string() can be used in the reproducer to circumvent direct field access.

[daverodgman]

This was fixed by #6191